RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause [v2]

[Jaikiran Pai]

On Wed, 11 May 2022 15:45:40 GMT, Christoph Langer <clanger at openjdk.org> wrote:

>> After https://bugs.openjdk.java.net/browse/JDK-8251329, javac throws errors when the classpath
>> contains jar files with . or .. in its name. The error message, however, does not help to find
>> the culprit. This could be improved.
>
> Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8286444
>  - Separate zips change from this PR
>  - JDK-8286444
>    
>    Improve error message in javac compilation when jar files with . or .. are in classpath

src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java line 567:

> 565:                     this.fileSystem = jarFSProvider.newFileSystem(archivePath, env);
> 566:                 } catch (ZipException ze) {
> 567:                     throw new IOException("ZipException opening \"" + archivePath + "\": " + ze.getMessage(), ze);

Hello Christoph, I'm sorry I'm a bit late to this discussion. One of the concerns raised in this PR is that we might end up leaking the file path of the jar being reported for error. The original example that you had in the JBS is something like this:


java -cp invalid.jar Hello.java

which after this change prints:


ZipException opening "invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name

This is fine, IMO.

However, consider a slightly modified case. Let's consider a case where you have a different `foo.jar` in that current directory and that `foo.jar` has a `Class-Path` entry which looks like:


Manifest-Version: 1.0
Class-Path: invalid.jar
Created-By: 19-internal (N/A)

So `foo.jar` is pointing to `invalid.jar`. Now let's run the following command by passing `foo.jar` as `-cp` value (which then brings in `invalid.jar` through the `Class-Path` entry in the Manifest file):


java -cp foo.jar Hello.java

With this PR, this now returns:


ZipException opening "/private/tmp/8286444/invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name

Notice that it has ended up leaking the file path of the jar that was pulled in through the `MANIFEST.MF` file. I think this is something that isn't allowed from a security point of view.

Perhaps an additional change needs to be done on top of this PR to prevent this, something like:


diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
index 1336fb74916..7c40afb62af 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
@@ -564,7 +564,7 @@ public class JavacFileManager extends BaseFileManager implements StandardJavaFil
                 try {
                     this.fileSystem = jarFSProvider.newFileSystem(archivePath, env);
                 } catch (ZipException ze) {
-                    throw new IOException("ZipException opening "" + archivePath + "": " + ze.getMessage(), ze);
+                    throw new IOException("ZipException opening "" + archivePath.getFileName() + "": " + ze.getMessage(), ze);
                 }
             } else {
                 this.fileSystem = FileSystems.newFileSystem(archivePath, (ClassLoader)null);



This then just prints the name of the jar in both cases:


java -cp invalid.jar Hello.java

ZipException opening "invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name


java -cp foo.jar Hello.java

ZipException opening "invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name

-------------

PR: https://git.openjdk.java.net/jdk/pull/8616