commit 78cf73473dda5ceee3eecda5169621f36b93c3db Author: Jiri Vanek Date: Tue Jun 18 15:37:47 2019 +0200 Fixed bug when relative path (..) could leak up (even out of cache) --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java +++ a/netx/net/sourceforge/jnlp/cache/CacheUtil.java @@ -696,46 +696,68 @@ path.append(location.getPort()); path.append(File.separatorChar); } - path.append(location.getPath().replace('/', File.separatorChar)); - if (location.getQuery() != null && !location.getQuery().trim().isEmpty()) { - path.append(".").append(location.getQuery()); - } - - File candidate = new File(FileUtils.sanitizePath(path.toString())); - if (candidate.getName().length() > 255) { - /** - * When filename is longer then 255 chars, then then various - * filesytems have issues to save it. By saving the file by its - * summ, we are trying to prevent collision of two files differs in - * suffixes (general suffix of name, not only 'filetype suffix') - * only. It is also preventing bug when truncate (files with 1000 - * chars hash in query) cuts to much. - */ + String locationPath = location.getPath().replace('/', File.separatorChar); + if (locationPath.contains("..")){ try { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] sum = md.digest(candidate.getName().getBytes(StandardCharsets.UTF_8)); - //convert the byte to hex format method 2 - StringBuilder hexString = new StringBuilder(); - for (int i = 0; i < sum.length; i++) { - hexString.append(Integer.toHexString(0xFF & sum[i])); - } - String extension = ""; - int i = candidate.getName().lastIndexOf('.'); - if (i > 0) { - extension = candidate.getName().substring(i);//contains dot - } - if (extension.length() < 10 && extension.length() > 1) { - hexString.append(extension); - } - candidate = new File(candidate.getParentFile(), hexString.toString()); + /** + * if path contains .. then it can harm lcoal system + * So without mercy, hash it + */ + String hexed = hex(new File(locationPath).getName(), locationPath); + return new File(path.toString(), hexed.toString()); } catch (NoSuchAlgorithmException ex) { - // should not occure, cite from javadoc: - // every java iomplementation should support + // should not occur, cite from javadoc: + // every java implementation should support // MD5 SHA-1 SHA-256 throw new RuntimeException(ex); } - } - return candidate; + } else { + path.append(locationPath); + if (location.getQuery() != null && !location.getQuery().trim().isEmpty()) { + path.append(".").append(location.getQuery()); + } + + File candidate = new File(FileUtils.sanitizePath(path.toString())); + try { + if (candidate.getName().length() > 255) { + /** + * When filename is longer then 255 chars, then then various + * filesystems have issues to save it. By saving the file by its + * sum, we are trying to prevent collision of two files differs in + * suffixes (general suffix of name, not only 'filetype suffix') + * only. It is also preventing bug when truncate (files with 1000 + * chars hash in query) cuts to much. + */ + String hexed = hex(candidate.getName(), candidate.getName()); + candidate = new File(candidate.getParentFile(), hexed.toString()); + } + } catch (NoSuchAlgorithmException ex) { + // should not occur, cite from javadoc: + // every java implementation should support + // MD5 SHA-1 SHA-256 + throw new RuntimeException(ex); + } + return candidate; + } + } + + private static String hex(String origName, String candidate) throws NoSuchAlgorithmException { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + byte[] sum = md.digest(candidate.getBytes(StandardCharsets.UTF_8)); + //convert the byte to hex format method 2 + StringBuilder hexString = new StringBuilder(); + for (int i = 0; i < sum.length; i++) { + hexString.append(Integer.toHexString(0xFF & sum[i])); + } + String extension = ""; + int i = origName.lastIndexOf('.'); + if (i > 0) { + extension = origName.substring(i);//contains dot + } + if (extension.length() < 10 && extension.length() > 1) { + hexString.append(extension); + } + return hexString.toString(); } /** diff --git a/netx/net/sourceforge/jnlp/util/FileUtils.java b/netx/net/sourceforge/jnlp/util/FileUtils.java index 89216375..a5356e08 100644 --- a/netx/net/sourceforge/jnlp/util/FileUtils.java +++ b/netx/net/sourceforge/jnlp/util/FileUtils.java @@ -183,6 +183,13 @@ */ public static void createParentDir(File f, String eMsg) throws IOException { File parent = f.getParentFile(); + // warning, linux and windows behave differently. Below snippet will pass on win(security hole), fail on linux + // warning mkdir is canonicaling, but exists/isDirectory is not. So where mkdirs return true, and really creates dir, isDirectory can still return false + // can be seen on this example + // mkdirs /a/b/../c + // where b do not exists will lead creation of /a/c + // but exists on /a/b/../c is false on linux even afterwards + // without hexing of .. paths, if (!parent.isDirectory() && !parent.mkdirs()) { throw new IOException(R("RCantCreateDir", eMsg == null ? parent : eMsg)); diff --git a/tests/netx/unit/net/sourceforge/jnlp/cache/CacheUtilTest.java b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheUtilTest.java index 6422246b..0d2d9811 100644 --- a/tests/netx/unit/net/sourceforge/jnlp/cache/CacheUtilTest.java +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheUtilTest.java @@ -88,6 +88,53 @@ public class CacheUtilTest { final File expected = new File("/tmp/https/example.com/5050/applet/e4f3cf11f86f5aa33f424bc3efe3df7a9d20837a6f1a5bbbc60c1f57f3780a4"); Assert.assertEquals(expected, CacheUtil.urlToPath(u, "/tmp")); } + + @Test + public void tesPathUpNoGoBasic() throws Exception { + final URL u = new URL("https://example.com/applet/../my.jar"); + final File expected = new File("/tmp/https/example.com/abca4723622ed60db3dea12cbe2402622a74f7a49b73e23b55988e4eee5ded.jar"); + File r = CacheUtil.urlToPath(u, "/tmp/"); + Assert.assertEquals(expected, r); + } + + @Test + public void tesPathUpNoGoBasicLong() throws Exception { + final URL u = new URL("https://example.com/applet/../my.jar.q_SlNFU1NJT05JRD02OUY1ODVCNkJBOTM1NThCQjdBMTA5RkQyNDZEQjEwRi5wcm9kX3RwdG9tY2F0MjE1X2p2bTsgRW50cnVzdFRydWVQYXNzUmVkaXJlY3RVcmw9Imh0dHBzOi8vZWZzLnVzcHRvLmdvdi9FRlNXZWJVSVJlZ2lzdGVyZWQvRUZTV2ViUmVnaXN0ZXJlZCI7IFRDUFJPRFBQQUlSc2Vzc2lvbj02MjIxMjk0MTguMjA0ODAuMDAwMA\""); + final File expected = new File("/tmp/https/example.com/ec97413e3f6eee8215ecc8375478cc1ae5f44f18241b9375361d5dfcd7b0ec"); + File r = CacheUtil.urlToPath(u, "/tmp/"); + Assert.assertEquals(expected, r); + } + + @Test + public void tesPathUpNoGoBasic2() throws Exception { + final URL u = new URL("https://example.com/../my.jar"); + final File expected = new File("/tmp/https/example.com/eb1a56bed34523dbe7ad84d893ebc31a8bbbba9ce3f370e42741b6a5f067c140.jar"); + File r = CacheUtil.urlToPath(u, "/tmp/"); + Assert.assertEquals(expected, r); + } + + @Test + public void tesPathUpNoGoBasicEvil() throws Exception { + final URL u = new URL("https://example.com/../../my.jar"); + final File expected = new File("/tmp/https/example.com/db464f11d68af73e37eefaef674517b6be23f0e4a5738aaee774ecf5b58f1bfc.jar"); + File r = CacheUtil.urlToPath(u, "/tmp/"); + Assert.assertEquals(expected, r); + } + + @Test + public void tesPathUpNoGoBasicEvil2() throws Exception { + final URL u = new URL("https://example.com:99/../../../my.jar"); + final File expected = new File("/tmp/https/example.com/99/95401524c345e0d554d4d77330e86c98a77b9bb58a0f93094204df446b356.jar"); + File r = CacheUtil.urlToPath(u, "/tmp/"); + Assert.assertEquals(expected, r); + } + @Test + public void tesPathUpNoGoBasicEvilest() throws Exception { + final URL u = new URL("https://example2.com/something/../../../../../../../../../../../my.jar"); + final File expected = new File("/tmp/https/example2.com/a8df64388f5b84d5f635e4d6dea5f4d2f692ae5381f8ec6736825ff8d6ff2c0.jar"); + File r = CacheUtil.urlToPath(u, "/tmp/"); + Assert.assertEquals(expected, r); + } @Test diff --git a/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java index 100d9150..7580d23b 100644 --- a/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java +++ b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java @@ -43,6 +43,8 @@ import java.io.File; import java.io.FileOutputStream; import java.io.InputStream; +import java.net.URL; +import java.nio.charset.Charset; import java.nio.file.Files; import java.util.Arrays; import java.util.List; @@ -55,6 +57,12 @@ import net.sourceforge.jnlp.browsertesting.browsers.firefox.FirefoxProfilesOperator; import net.sourceforge.jnlp.cache.UpdatePolicy; import net.sourceforge.jnlp.config.DeploymentConfiguration; +import net.sourceforge.jnlp.config.PathsAndFiles; +import net.sourceforge.jnlp.JNLPFile; +import net.sourceforge.jnlp.ServerAccess; +import net.sourceforge.jnlp.ServerLauncher; +import net.sourceforge.jnlp.util.StreamUtils; +import net.sourceforge.jnlp.cache.CacheUtil; import net.sourceforge.jnlp.mock.DummyJNLPFileWithJar; import net.sourceforge.jnlp.security.appletextendedsecurity.AppletSecurityLevel; import net.sourceforge.jnlp.security.appletextendedsecurity.AppletStartupSecuritySettings; @@ -65,6 +73,7 @@ import org.junit.BeforeClass; import org.junit.Test; +import org.junit.Ignore; public class JNLPClassLoaderTest extends NoStdOutErrTest { @@ -138,7 +147,8 @@ File tempDirectory = FileTestUtils.createTempDirectory(); File jarLocation = new File(tempDirectory, "test.jar"); - /* Test with main-class in manifest */ { + /* Test with main-class in manifest */ + { Manifest manifest = new Manifest(); manifest.getMainAttributes().put(Attributes.Name.MAIN_CLASS, "DummyClass"); FileTestUtils.createJarWithContents(jarLocation, manifest); @@ -156,8 +166,10 @@ } @Test + @Ignore public void getMainClassNameTestEmpty() throws Exception { - /* Test with-out any main-class specified */ { + /* Test with-out any main-class specified */ + { File tempDirectory = FileTestUtils.createTempDirectory(); File jarLocation = new File(tempDirectory, "test.jar"); FileTestUtils.createJarWithContents(jarLocation /* No contents */); @@ -363,4 +375,57 @@ } } + + @Test + public void testRelativePathInUrl() throws Exception { + CacheUtil.clearCache(); + int port = ServerAccess.findFreePort(); + File dir = FileTestUtils.createTempDirectory(); + dir.deleteOnExit(); + dir = new File(dir,"base"); + dir.mkdir(); + File jar = new File(dir,"j1.jar"); + File jnlp = new File(dir+"/a/b/up.jnlp"); + jnlp.getParentFile().mkdirs(); + InputStream is = ClassLoader.getSystemClassLoader().getResourceAsStream("net/sourceforge/jnlp/runtime/up.jnlp"); + String jnlpString = StreamUtils.readStreamAsString(is, true, "utf-8"); + is.close(); + jnlpString = jnlpString.replaceAll("8080", ""+port); + is = ClassLoader.getSystemClassLoader().getResourceAsStream("net/sourceforge/jnlp/runtime/j1.jar"); + StreamUtils.copyStream(is, new FileOutputStream(jar)); + Files.write(jnlp.toPath(),jnlpString.getBytes("utf-8")); + ServerLauncher as = ServerAccess.getIndependentInstance(jnlp.getParent(), port); + boolean verifyBackup = JNLPRuntime.isVerifying(); + boolean trustBackup= JNLPRuntime.isTrustAll(); + boolean securityBAckup= JNLPRuntime.isSecurityEnabled(); + boolean verbose= JNLPRuntime.isDebug(); + JNLPRuntime.setVerify(false); + JNLPRuntime.setTrustAll(true); + JNLPRuntime.setSecurityEnabled(false); + JNLPRuntime.setDebug(true); + try { + final JNLPFile jnlpFile1 = new JNLPFile(new URL("http://localhost:" + port + "/up.jnlp")); + final JNLPClassLoader classLoader1 = new JNLPClassLoader(jnlpFile1, UpdatePolicy.ALWAYS) { + @Override + protected void activateJars(List jars) { + super.activateJars(jars); + } + + }; + InputStream is1 = classLoader1.getResourceAsStream("Hello1.class"); + is1.close(); + is1 = classLoader1.getResourceAsStream("META-INF/MANIFEST.MF"); + is1.close(); + Assert.assertTrue(new File(PathsAndFiles.CACHE_DIR.getFullPath()+"/0/http/localhost/"+port+"/up.jnlp").exists()); + Assert.assertTrue(new File(PathsAndFiles.CACHE_DIR.getFullPath()+"/1/http/localhost/"+port+"/f812acb32c857fd916c842e2bf4fb32b9c3837ef63922b167a7e163305058b7.jar").exists()); + } finally { + JNLPRuntime.setVerify(verifyBackup); + JNLPRuntime.setTrustAll(trustBackup); + JNLPRuntime.setSecurityEnabled(securityBAckup); + JNLPRuntime.setDebug(verbose); + as.stop(); + } + + } + } diff --git a/tests/netx/unit/net/sourceforge/jnlp/runtime/up.jnlp b/tests/netx/unit/net/sourceforge/jnlp/runtime/up.jnlp new file mode 100644 index 00000000..b22fdfb7 --- /dev/null +++ b/tests/netx/unit/net/sourceforge/jnlp/runtime/up.jnlp @@ -0,0 +1,15 @@ + + + +1965Nemzeti Ado- es Vamhivatal + + + + + + + + + + +