Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
Hi All, Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly. Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks! Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/ -Dan
Hi Dan; External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992 I would like to better understand why silently removing the garbage null characters is the right answer here. If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled. Mike On Feb 26 2013, at 16:10 , Dan Xu wrote:
Hi All,
Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly.
Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks!
Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/
-Dan
Filenames are a lot like environment variables, and we already have code that rejects NUL chars in environment variable names: // Check that name is suitable for insertion into Environment map private static void validateVariable(String name) { if (name.indexOf('=') != -1 || name.indexOf('\u0000') != -1) throw new IllegalArgumentException ("Invalid environment variable name: \"" + name + "\""); } On Tue, Feb 26, 2013 at 4:33 PM, Mike Duigou <mike.duigou@oracle.com> wrote:
Hi Dan;
External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992
I would like to better understand why silently removing the garbage null characters is the right answer here.
If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled.
Mike
On Feb 26 2013, at 16:10 , Dan Xu wrote:
Hi All,
Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly.
Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks!
Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/
-Dan
Thank you, Mike. The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found. -Dan On 02/26/2013 04:33 PM, Mike Duigou wrote:
Hi Dan;
External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992
I would like to better understand why silently removing the garbage null characters is the right answer here.
If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled.
Mike
On Feb 26 2013, at 16:10 , Dan Xu wrote:
Hi All,
Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly.
Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks!
Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/
-Dan
On 27/02/2013 12:31 PM, Dan Xu wrote:
Thank you, Mike.
The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found.
That still gives the choice of deleting the nul characters, or treating them as a terminator. And do we really want/need to maintain compatability in this case anyway? What reasonable expectations can anyone have if they have embedded nuls? David
-Dan
On 02/26/2013 04:33 PM, Mike Duigou wrote:
Hi Dan;
External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992
I would like to better understand why silently removing the garbage null characters is the right answer here.
If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled.
Mike
On Feb 26 2013, at 16:10 , Dan Xu wrote:
Hi All,
Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly.
Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks!
Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/
-Dan
On 02/27/2013 03:52 AM, David Holmes wrote:
On 27/02/2013 12:31 PM, Dan Xu wrote:
Thank you, Mike.
The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found.
That still gives the choice of deleting the nul characters, or treating them as a terminator.
And do we really want/need to maintain compatability in this case anyway? What reasonable expectations can anyone have if they have embedded nuls?
What does a FileInputStream for example do when trying to open a File with embedded NUL chars on UNIX/Windows ? Does it try to open a "truncated" path? If so, then perhaps "normalize" could do that beforehand... Regards, Peter
David
-Dan
On 02/26/2013 04:33 PM, Mike Duigou wrote:
Hi Dan;
External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992
I would like to better understand why silently removing the garbage null characters is the right answer here.
If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled.
Mike
On Feb 26 2013, at 16:10 , Dan Xu wrote:
Hi All,
Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly.
Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks!
Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/
-Dan
On 27/02/2013 12:07, Peter Levart wrote:
What does a FileInputStream for example do when trying to open a File with embedded NUL chars on UNIX/Windows ? Does it try to open a "truncated" path? If so, then perhaps "normalize" could do that beforehand...
Yes, it's truncated. Dan's fix covers FileInputStream and friends too as they go through the normalize code. -Alan
On 02/27/2013 01:15 PM, Alan Bateman wrote:
On 27/02/2013 12:07, Peter Levart wrote:
What does a FileInputStream for example do when trying to open a File with embedded NUL chars on UNIX/Windows ? Does it try to open a "truncated" path? If so, then perhaps "normalize" could do that beforehand...
Yes, it's truncated. Dan's fix covers FileInputStream and friends too as they go through the normalize code.
You should throw an exception. Embedded NUL characters have been used to bypass security checks. The canonical example is an upload to a web server directory. You check that the file ends with ".jpg", so it won't be interpreted by the web server, but the full extension is actually ".php\000.jpg", so you end up writing a ".php" file, which is. Furthermore, dropping the NUL character is *extremely* dangerous because it could be used to bypass security checks which look for ".." to prevent directory traversal attacks. -- Florian Weimer / Red Hat Product Security Team
On 03/03/2013 20:00, Florian Weimer wrote:
You check that the file ends with ".jpg", so it won't be interpreted by the web server, but the full extension is actually ".php\000.jpg", so you end up writing a ".php" file, which is. The application have have the path String ".php\000.jpg" but when you create the file (with FileOutputStream or other APIs) then it would be ".php.jpg". Another potential approach is to just fail when attempting to create the file but changing File's constructor to throw an exception would be an incompatible change.
-Alan
On 03/03/2013 10:01 PM, Alan Bateman wrote:
On 03/03/2013 20:00, Florian Weimer wrote:
You check that the file ends with ".jpg", so it won't be interpreted by the web server, but the full extension is actually ".php\000.jpg", so you end up writing a ".php" file, which is.
The application have have the path String ".php\000.jpg" but when you create the file (with FileOutputStream or other APIs) then it would be ".php.jpg".
Yes, that's the behavior with dropping, and it does help in this case. (I was arguing against truncation.) But dropping is unsafe, too, as I described in the second paragraph of my message.
Another potential approach is to just fail when attempting to create the file
I think this is what's required. It's what Python has been doing for some time.
but changing File's constructor to throw an exception would be an incompatible change.
I completely agree. I think I've written code myself which relies on the File(String) constructor not looking at the contents of the string. 8-/ -- Florian Weimer / Red Hat Product Security Team
Hi All, Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered. As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review! webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/ -Dan
On 03/05/2013 00:08, Dan Xu wrote:
Hi All,
Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered.
As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review!
webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/
-Dan
This looks much better. I guess a Boolean would have worked as well as adding the PathStatus enum but what you have seems okay. It would be good to get Sherman's confirmation that we don't need to be concerned about anything else encoding to include NUL. -Alan.
On 5/3/13 6:48 AM, Alan Bateman wrote:
On 03/05/2013 00:08, Dan Xu wrote:
Hi All,
Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered.
As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review!
webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/
-Dan
This looks much better. I guess a Boolean would have worked as well as adding the PathStatus enum but what you have seems okay.
It would be good to get Sherman's confirmation that we don't need to be concerned about anything else encoding to include NUL.
-Alan.
The change looks fine. I don't see any encoding issue, though this reminds me one possible use scenario. Do we want to make it path "valid", if the NUL is at the end of the path? This should not been an issue normally though, everything from the native does not have it. Just wonder if this scenario might work before (?), with no potential vuln risk (?), we may want to keep it? No, I don't know any real code has the NUL attached to the end, just a wild guess. It's definitely fine to wait for any complain, if there will be, and then react. -Sherman
On 05/03/2013 01:08 AM, Dan Xu wrote:
Hi All,
Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered.
As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review!
I like this approach, thanks. I think the additional parenthesis around the return expression and the ternary operator are not part of the usual coding standard. -- Florian Weimer / Red Hat Product Security Team
On 05/06/2013 06:59 AM, Florian Weimer wrote:
On 05/03/2013 01:08 AM, Dan Xu wrote:
Hi All,
Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered.
As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review!
I like this approach, thanks.
I think the additional parenthesis around the return expression and the ternary operator are not part of the usual coding standard.
Thank you for your good review. I will fix the format. As for the possible scenario, NUL appearing at the end of a path, I will regard it as the same kind of invalidity as other embedded NUL cases since there are no obvious use cases for that. I will push the fix today. Thanks! -Dan
On 27/02/2013 02:31, Dan Xu wrote:
Thank you, Mike.
The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found.
-Dan Right, we can't change the constructor to throw an exception, particularly if this fix is going to be back-ported to 7u. For NIO then it's not an issue because getPath was specified from the begining to throw the unexpected InvalidPathException when it is given garbage.
-Alan.
Ouch. That is unfortunate that File cannot reject bad input. Perhaps FileInputStream etc. should throw a specialized "Bad Filename" FnF for paths containing NUL if the underlying filesystem does not support NUL? Masking garbage input always really scares me. Mike On Feb 27 2013, at 02:40 , Alan Bateman wrote:
On 27/02/2013 02:31, Dan Xu wrote:
Thank you, Mike.
The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found.
-Dan Right, we can't change the constructor to throw an exception, particularly if this fix is going to be back-ported to 7u. For NIO then it's not an issue because getPath was specified from the begining to throw the unexpected InvalidPathException when it is given garbage.
-Alan.
Because we cannot change the behaviour of File constructorsto error out the bad input immediately, the changes will be scattered all over many java io methods, especially methods in File.java, if we choose to reject bad inputs. And due to the delay of the rejection, it also brings a little headache to developers to find out why the exception happens.Comparing with that, the proposed approach is simple and resilient. In addition, the currentnormalization logic already tolerates some invalid path-name formats and invalid characters. This approach can be regarded as a extension of the tolerance to theNUL character. It is consistent with what the native platform would do with a null and what/how we are currently normalizing the "slash".Thanks! -Dan On 02/27/2013 10:31 AM, Mike Duigou wrote:
Ouch. That is unfortunate that File cannot reject bad input.
Perhaps FileInputStream etc. should throw a specialized "Bad Filename" FnF for paths containing NUL if the underlying filesystem does not support NUL?
Masking garbage input always really scares me.
Mike
On Feb 27 2013, at 02:40 , Alan Bateman wrote:
On 27/02/2013 02:31, Dan Xu wrote:
Thank you, Mike.
The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found.
-Dan Right, we can't change the constructor to throw an exception, particularly if this fix is going to be back-ported to 7u. For NIO then it's not an issue because getPath was specified from the begining to throw the unexpected InvalidPathException when it is given garbage.
-Alan.
participants (8)
-
Alan Bateman
-
Dan Xu
-
David Holmes
-
Florian Weimer
-
Martin Buchholz
-
Mike Duigou
-
Peter Levart
-
Xueming Shen