commit 78cf73473dda5ceee3eecda5169621f36b93c3db

Author: Jiri Vanek <jvanek@redhat.com>

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<JARDesc> 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 @@

+

+<jnlp spec="6.0+" codebase=".">

+

+<information><title>1965</title><vendor>Nemzeti Ado- es Vamhivatal</vendor><offline-allowed/></information>

+

+

+<resources>

+  <j2se href="http://java.sun.com/products/autodl/j2se" version="1.8+" />

+

+  <jar href="http://localhost:8080/../../../base/j1.jar" version="2.0"/>

+</resources>

+

+<application-desc main-class="Hello1" />

+

+</jnlp>