RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

Tue Dec 11 15:38:43 UTC 2018

> File paths are, in general, always something that demands extra scrutiny
> as it can be the source of security issues (privacy leaks, traversal
> attacks, etc). It's not just me that thinks that way, you can do a
> search on the Internet and find lots of references.
...
> 
> It might be perfectly fine and your usage might be ok too. But I'll need
> some time to evaluate it further. I am not familiar with the code in
> this part of the JDK.
> 
> I would also see if you can think about the security issues as well.
> Where do these paths come from? What are the application use cases that
> invoke these lower methods? From application code or elsewhere? Are
> relative paths used? Are paths containing ".." canonicalized? Are they
> arbitrary paths or a fixed set of paths? Do they ever contain sensitive
> information like home directory user names, etc?
> 
> Once we understand if there are any security issues, then we can decide
> if allowing that via a system property is acceptable or not, and if so
> the security risks that we would have to document for that property.
> 

Hello, the file paths potentially printed in the enhanced exceptions  in *canonicalize0, *createFileExclusively,
Java_java_io_WinNTFileSystem_canonicalizeWithPrefix0 could potentially come from  more or less arbitrary paths.

This means, in some cases, there is a chance that information (like user-ids/user-names often found in paths below  HOME dirs )
might be in the printed paths that people do not want to have in log files or other output containing the exception messages.

For such potentially sensitive info in exception messages, we have already the  jdk.includeInExceptions  
security property, see the java.security file :

1064 # Enhanced exception message information
1065 #
1066 # By default, exception messages should not include potentially sensitive
1067 # information such as file names, host names, or port numbers. This property
1068 # accepts one or more comma separated values, each of which represents a
1069 # category of enhanced exception message information to enable. Values are
1070 # case-insensitive. Leading and trailing whitespaces, surrounding each value,
1071 # are ignored. Unknown values are ignored.

I think this approach fits well for 8211752 (and is used already for some other categories).

I created an updated webrev,  it uses the jdk.includeInExceptions security property 
(had to do it via JNI because it did not work from the static class initializers of UnixFileSystem/WinNTFileSystem) :

http://cr.openjdk.java.net/~mbaesken/webrevs/8211752.2/

Best regards, Matthias

> -----Original Message-----
> From: Sean Mullan <sean.mullan at oracle.com>
> Sent: Donnerstag, 8. November 2018 20:36
> To: Baesken, Matthias <matthias.baesken at sap.com>; Langer, Christoph
> <christoph.langer at sap.com>; Alan Bateman <Alan.Bateman at oracle.com>;
> security-dev at openjdk.java.net; core-libs-dev at openjdk.java.net
> Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
> enhance some IOExceptions with path causing the issue
> 
> On 11/7/18 3:52 AM, Baesken, Matthias wrote:
> >> Sorry, I haven't had time to look at this in more detail yet. But, let's
> >> take a step back first. Can you or Matthias explain in more detail why
> >> this fix is necessary? What are the use cases and motivation?
> >
> > Hello,
> >      adding paths  (or maybe more details)  to exception messages just
> makes analyzing errors easier.
> > You do not get just "Bad path",  but also the real bad path which gives you a
> hint where to look and analyze further .
> 
> File paths are, in general, always something that demands extra scrutiny
> as it can be the source of security issues (privacy leaks, traversal
> attacks, etc). It's not just me that thinks that way, you can do a
> search on the Internet and find lots of references.
> 
> > That's why we introduced it in our JVM ages ago.
> > I have to agree that additionally  printing cwd / user.dir is a bit special,  so I
> omit that from this revision of the patch.
> > This makes the patch more simple.
> > New revision :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8211752.1/
> >
> >
> > Unfortunately the usage of sun.security.util.SecurityProperties  (which was
> considered)  in the  static { ... }
> > class initializers (e.g. UnixFileSystem.java) just does not work.
> > It fails with already in the build (!) with :
> >
> > Error occurred during initialization of boot layer
> > java.lang.ExceptionInInitializerError
> > Caused by: java.lang.NullPointerException
> >
> > (seems it is too early in the game for SecurityProperties).
> > (btw. this is another  NOT very helpful exception error message)
> >
> >
> > So I unfortunately had to go back to using system properties.
> >
> >
> > Btw. another question regarding path output in exceptions  :
> > you seem to consider it a bad thing  to (unconditionally) print paths in the
> exception messages,
> > but then on the other hand we have  it  already in OpenJDK
> UnixFileSystem_md.c :
> >
> >   269 JNIEXPORT jboolean JNICALL
> >   270 Java_java_io_UnixFileSystem_createFileExclusively(JNIEnv *env,
> jclass cls,
> >   271                                                   jstring pathname)
> >   272 {
> >   .......
> >   277         /* The root directory always exists */
> >   278         if (strcmp (path, "/")) {
> >   279             fd = handleOpen(path, O_RDWR | O_CREAT | O_EXCL, 0666);
> >   280             if (fd < 0) {
> >   281                 if (errno != EEXIST)
> >   282                     JNU_ThrowIOExceptionWithLastError(env, path);
> >   283             } else {
> >   284                 if (close(fd) == -1)
> >   285                     JNU_ThrowIOExceptionWithLastError(env, path);
> >
> >
> > Why is it fine here for a long time , but considered harmful at the other
> places ?
> > If we want to be consistent, we should then  write  "Bad path" here (or
> allow the path output at the other places too ).
> 
> It might be perfectly fine and your usage might be ok too. But I'll need
> some time to evaluate it further. I am not familiar with the code in
> this part of the JDK.
> 
> I would also see if you can think about the security issues as well.
> Where do these paths come from? What are the application use cases that
> invoke these lower methods? From application code or elsewhere? Are
> relative paths used? Are paths containing ".." canonicalized? Are they
> arbitrary paths or a fixed set of paths? Do they ever contain sensitive
> information like home directory user names, etc?
> 
> Once we understand if there are any security issues, then we can decide
> if allowing that via a system property is acceptable or not, and if so
> the security risks that we would have to document for that property.
> 
> Thanks,
> Sean
> 