-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted #16893
Changes from 1 commit
a42a45c
176d516
29ee889
7aa3766
8a13b55
b0d4cb9
0e14c97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,10 +207,20 @@ public int readNBytes(byte[] b, int off, int len) { | |
public synchronized long transferTo(OutputStream out) throws IOException { | ||
int len = count - pos; | ||
if (len > 0) { | ||
byte[] tmp; | ||
if ("java.io".equals(out.getClass().getPackageName())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should trust all classes in out.getClass().getPackageName().startsWith("java.") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change in 176d516 to use |
||
tmp = null; | ||
else | ||
tmp = new byte[Integer.min(len, MAX_TRANSFER_SIZE)]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks okay, I'd probably rename tmp to something better, maybe tmpbuf. |
||
|
||
int nwritten = 0; | ||
while (nwritten < len) { | ||
int nbyte = Integer.min(len - nwritten, MAX_TRANSFER_SIZE); | ||
out.write(buf, pos, nbyte); | ||
if (tmp != null) { | ||
System.arraycopy(buf, pos, tmp, 0, nbyte); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume the overall performance of transferTo will be faster if we use System.arraycopy only once in line 215 to create a safe copy of the complete buf instead of calling it multiple times in a loop to create copies per slice. In that case we can omit the tmp == null case but simply use tmp = buf, making the code in the loop if-free. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a tradeoff here between number of invocations of |
||
out.write(tmp, 0, nbyte); | ||
} else | ||
out.write(buf, pos, nbyte); | ||
pos += nbyte; | ||
nwritten += nbyte; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/* | ||
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
/* | ||
* @test | ||
* @bug 8321053 | ||
* @summary Verify ByteArrayInputStream.buf is used directly by | ||
* ByteArrayInputStream.transferTo only when its OutputStream | ||
* parameter is trusted | ||
* @key randomness | ||
*/ | ||
|
||
import java.io.ByteArrayInputStream; | ||
import java.io.ByteArrayOutputStream; | ||
import java.io.IOException; | ||
import java.io.OutputStream; | ||
import java.util.Arrays; | ||
import java.util.Objects; | ||
import java.util.Random; | ||
|
||
public class TransferToTrusted { | ||
private static final Random RND = new Random(System.nanoTime()); | ||
|
||
private static class UntrustedOutputStream extends OutputStream { | ||
UntrustedOutputStream() { | ||
super(); | ||
} | ||
|
||
@Override | ||
public void write(byte[] b, int off, int len) throws IOException { | ||
Objects.checkFromIndexSize(off, len, b.length); | ||
byte[] tmp = new byte[len]; | ||
RND.nextBytes(tmp); | ||
System.arraycopy(tmp, 0, b, off, len); | ||
} | ||
|
||
@Override | ||
public void write(int b) throws IOException { | ||
write(new byte[] {(byte)b}); | ||
} | ||
} | ||
|
||
public static void main(String[] args) throws IOException { | ||
byte[] buf = new byte[128]; | ||
RND.nextBytes(buf); | ||
byte[] dup = Arrays.copyOf(buf, buf.length); | ||
|
||
ByteArrayInputStream bais = new ByteArrayInputStream(dup); | ||
bais.mark(dup.length); | ||
|
||
OutputStream baos = new ByteArrayOutputStream(); | ||
bais.transferTo(baos); | ||
bais.reset(); | ||
if (!Arrays.equals(buf, bais.readAllBytes())) | ||
throw new RuntimeException("Internal buffer has been modified"); | ||
|
||
bais.reset(); | ||
OutputStream out = new UntrustedOutputStream(); | ||
bais.transferTo(out); | ||
bais.reset(); | ||
if (!Arrays.equals(buf, bais.readAllBytes())) | ||
throw new RuntimeException("Internal buffer has been modified"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this protection defeated with:
Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch: that in fact defeats the protection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in 176d516 not to trust
FilterOutputStream
s.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only other alternative would be to walk
((FilterOutputStream)out).out
and if everything in the out chain is in the "java." package then the out can be trusted.I don't like the approach of deny list, walking the chain as (subjectively) it seems too fragile.
Also I think I can break this version of the code with ChannelOutputStream. I didn't run this through a compiler nor test it but the idea is that ChannelOutputStream calls ByteBuffer.wrap(bs) and doesn't call ByteBuffer.asReadOnlyBuffer. So a malicious WritableByteChannel should be able to gain access to the original array:
However, the ChannelOutputStream is in sun.nio.ch so on second thought it shouldn't break. The pattern is repeated in Channels.newOutputStream(AsynchronousByteChannel ch) so that should fail as it is in the "java." namespace.
I think an allow list would be safer but that brings all the drawbacks that Alan was talking about before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have done this incorrectly, but with this version of the above
wolf
I do not see any corruption:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the problem that unless we have an explicit whitelist, we do open the risk of accidentially adding another wrapper stream in future to the JDK somewhere and forget to add it to the blacklist. So for safety, I would pleae for not using .startsWith() but explitly mention the actively proven-as-safe classes only. That way, the code might be slower (sad but true) but inherently future-proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case of
Channels.newOutputStream(AsynchronousByteChannel)
could be handled by changing the return value of that method. For example,sun.nio.ch.Streams
could have a methodOutputStream of(AsynchronousByteChannel)
added to it which returned something like anAsynChannelOutputStream
and we could use that.That said, it is true that a deny list is not inherently future-proof like an allow list, as stated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a sufficiently future-proof deny list could be had by changing
back to
That would for example dispense with the problematic
Channels.newOutputStream(AynsynchronousByteChannel)
case:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if scope is limited to
java.io
you have deal with FilterOutputStream and ObjectOutputStream. I still haven't done a complete search so there could be other adapters I've yet to review.Thinking of a different approach, what if ByteArrayInputStream actually recorded and used
readlimit
of themark
method? This allows us to safely leak or poison 'this.data' because once transferTo is called we safely change owner of the byte array if we know this stream is allowed to forget it existed. Effectively you could do optimizations like this (didn't test or compile this):This would approach avoids having to maintain an allow or deny list. The downside of this approach and that is the constructor of ByteInputStream doesn't copy the byte[] parameter. The caller is warned about this in the JavaDocs but it might be shocking to have data escape ByteArrayInputStream. Maybe that is deal breaker? Obviously there a compatibility issue with recording readLimit in the mark method as it states it does nothing.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is getting too complicated. For the time being, I think it would be better simply to have a conservative allow-list and trust only the classes in it. The approach can always be broadened at a later date, but at least for now there would be protection against untrustworthy
OutputStream
s