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

[Christoph Langer]

On Sun, 15 May 2022 02:36:56 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

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

Hi Jai,

this is a good point. Regarding the potential leaking of the jar path in the manifest case pointed out by you, I would think that it is not problematic as the exception occurs in a (developer-) tool and there it might even be useful. However, for consistency purposes, it might be clearer to just print the jar name. I guess that's already helpful enough.

I'll open a follow up bug for this.

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

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