8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted #16893

bplb · 2023-11-30T00:03:21Z

Pass ByteArrayInputStream.buf directly to the OutputStream parameter of BAIS.transferTo only if the target stream is in the java.io package.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted (Bug - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 8a13b553

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16893/head:pull/16893
$ git checkout pull/16893

Update a local copy of the PR:
$ git checkout pull/16893
$ git pull https://git.openjdk.org/jdk.git pull/16893/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16893

View PR using the GUI difftool:
$ git pr show -t 16893

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16893.diff

Webrev

Link to Webrev Comment

…sferTo() is trusted

bridgekeeper · 2023-11-30T00:04:13Z

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-11-30T00:05:46Z

@bplb The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-11-30T00:08:18Z

Webrevs

jmehrens · 2023-11-30T04:26:44Z

src/java.base/share/classes/java/io/ByteArrayInputStream.java

@@ -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()))


Isn't this protection defeated with:

ByteArrayInputStream bais = new ByteArrayInputStream(bytes); UntrustedOutputStream uos = new UntrustedOutputStream(); bais.transferTo(new java.io.DataOutputStream(uos));

Or am I missing something?

Good catch: that in fact defeats the protection.

Changed in 176d516 not to trust FilterOutputStreams.

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.

byte[] tmp = null; for (OutputStream os = out; os != null;) { if (os.getClass().getPackageName().startsWith("java.")) { if (os instanceof FilterOutputStream fos) { //loops in this chain is going to cause this code to never end. // self reference A -> A or transitive reference A -> B -> C ->A os = fos.out; continue; } break; } tmp = new byte[Integer.min(len, MAX_TRANSFER_SIZE)]; break; }

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:

WritableByteChannel wolf = new WritableByteChannel() { public int write(ByteBuffer src) throws IOException { src.array()[0] = '0'; //oh no! return 0; } }; ByteArrayInputStream bais = new ByteArrayInputStream(bytes); OutputStream wolfInSheepSuitAndTie = Channels.newOutputStream(wolf); bais.transferTo(wolfInSheepSuitAndTie);

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.

I might have done this incorrectly, but with this version of the above wolf I do not see any corruption:

java.nio.channels.WritableByteChannel wolf = new java.nio.channels.WritableByteChannel() { private boolean closed = false; public int write(java.nio.ByteBuffer src) throws IOException { int rem = src.remaining(); Arrays.fill(src.array(), src.arrayOffset() + src.position(), src.arrayOffset() + src.limit(), (byte)'0'); src.position(src.limit()); return rem; } public boolean isOpen() { return !closed; } public void close() throws IOException { closed = true; } };

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.

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 method OutputStream of(AsynchronousByteChannel) added to it which returned something like an AsynChannelOutputStream 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.

I think that a sufficiently future-proof deny list could be had by changing

211 if (out.getClass().getPackageName().startsWith("java.") &&

back to

211 if ("java.io".equals(out.getClass().getPackageName()) &&

That would for example dispense with the problematic Channels.newOutputStream(AynsynchronousByteChannel) case:

jshell> AsynchronousSocketChannel asc = AsynchronousSocketChannel.open() asc ==> sun.nio.ch.UnixAsynchronousSocketChannelImpl[unconnected] jshell> OutputStream out = Channels.newOutputStream(asc) out ==> java.nio.channels.Channels$2@58c1670b jshell> Class outClass = out.getClass() outClass ==> class java.nio.channels.Channels$2 jshell> outClass.getPackageName() $5 ==> "java.nio.channels"

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 the mark 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):

public synchronized long transferTo(OutputStream out) throws IOException { int len = count - pos; if (len > 0) { byte[] data = this.data; byte[] tmp = null; if (this.readLimit == 0) { //<- recorded by mark method, initial value on construction of this would be zero. data = this.data; //swap owner of bytes this.data = new byte[0]; Arrays.fill(data, 0, pos, (byte) 0); // hide out of bounds data. Arrays.fill(data, count, data.length, (byte) 0); } else { tmp = new byte[Integer.min(len, MAX_TRANSFER_SIZE)]; } 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); out.write(tmp, 0, nbyte); } else out.write(buf, pos, nbyte); pos += nbyte; nwritten += nbyte; } assert pos == count; if (data.length ==0) { //uphold rules of class. pos = count = mark = 0; } } return len; }

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?

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 OutputStreams

stsypanov · 2023-11-30T12:34:24Z

src/java.base/share/classes/java/io/ByteArrayInputStream.java

@@ -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()))


I think we should trust all classes in java.* packages, i.e. the check should be

out.getClass().getPackageName().startsWith("java.")

Change in 176d516 to use java. instead of java.io.

mkarg · 2023-12-01T13:33:01Z

src/java.base/share/classes/java/io/ByteArrayInputStream.java

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


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 is a tradeoff here between number of invocations of arraycopy and amount of memory allocated for tmp. (We have seen this before in #14981 which I have allowed to languish.) The allocation limit is MAX_TRANSFER_SIZE which is presently 128 kB, so any transfer of size less than this will invoke arraycopy only once already.

mkarg · 2023-12-01T13:41:54Z

src/java.base/share/classes/java/io/ByteArrayInputStream.java

@@ -207,10 +207,21 @@ 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 (out.getClass().getPackageName().startsWith("java.") &&


Has anybody actually estimated or measured if such an exception is actually useful / needed given the fact that System.arraycopy is fast native code and most buffers used by java.io-located streams are just few KB? Just asking as it could be the case that interpreting this Java bytecode could be slower than executing some ASM ops to create a few-KB copy, and we might do an "premature optimization" here.

mkarg · 2023-12-05T07:37:39Z

src/java.base/share/classes/java/io/ByteArrayInputStream.java

+            // 'tmp' is null if and only if 'out' is trusted
+            byte[] tmp;
+            Class<?> outClass = out.getClass();
+            if (outClass.getPackageName().equals("java.io") &&


For what do we need this string-based check here?

I suspect it's left over from a previous iteration. In any case, limiting it to a small number of output streams makes this easier to look at. BAOS and FOS seem okay, POP seems okay too but legacy and not interesting.

I suspect it's left over from a previous iteration. In any case, limiting it to a small number of output streams makes this easier to look at. BAOS and FOS seem okay, POP seems okay too but legacy and not interesting.

Agreed for a rather short list of explicitly whitelisted implementations. We should get rid of the package check.

I checked all the OutputStreams in the list for trustworthiness. The package check is vestigial; will remove. It could be useful if multiple packages were involved with multiple trusted classes in each.

AlanBateman · 2023-12-05T17:02:00Z

src/java.base/share/classes/java/io/ByteArrayInputStream.java

+                outClass == PipedOutputStream.class)
+                tmp = null;
+            else
+                tmp = new byte[Integer.min(len, MAX_TRANSFER_SIZE)];


Looks okay, I'd probably rename tmp to something better, maybe tmpbuf.

openjdk · 2023-12-05T17:35:56Z

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 88 new commits pushed to the master branch:

db5613a: 8317288: [macos] java/awt/Window/Grab/GrabTest.java: Press on the outside area didn't cause ungrab
b1cb374: 8320349: Simplify FileChooserSymLinkTest.java by using single-window testUI
18c7922: 8321224: ct.sym for JDK 22 contains references to internal modules
83ffc1a: 8320303: Allow PassFailJFrame to accept single window creator
fd31f6a: 8321183: Incorrect warning from cds about the modules file
027b5db: 8321215: Incorrect x86 instruction encoding for VSIB addressing mode
61d0db3: 8318468: compiler/tiered/LevelTransitionTest.java fails with -XX:CompileThreshold=100 -XX:TieredStopAtLevel=1
87516e2: 8320943: Files/probeContentType/Basic.java fails on latest Windows 11 - content type mismatch
800f347: 8321216: SerialGC attempts to access the card table beyond the end of the heap during card table scan
a1fe16b: 8321300: Cleanup TestHFA
... and 78 more: https://git.openjdk.org/jdk/compare/c86431767e6802317dc2be6221a5d0990b976ddc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

bplb · 2023-12-05T19:39:03Z

/integrate

openjdk · 2023-12-05T19:41:15Z

Going to push as commit b0d1450.
Since your change was applied there have been 92 commits pushed to the master branch:

acaf2c8: 8318590: JButton ignores margin when painting HTML text
d3df3eb: 8294699: Launcher causes lingering busy cursor
fddc02e: 8321225: [JVMCI] HotSpotResolvedObjectTypeImpl.isLeafClass shouldn't create strong references
640d7f3: 8314327: Issues with JShell when using "local" execution engine
db5613a: 8317288: [macos] java/awt/Window/Grab/GrabTest.java: Press on the outside area didn't cause ungrab
b1cb374: 8320349: Simplify FileChooserSymLinkTest.java by using single-window testUI
18c7922: 8321224: ct.sym for JDK 22 contains references to internal modules
83ffc1a: 8320303: Allow PassFailJFrame to accept single window creator
fd31f6a: 8321183: Incorrect warning from cds about the modules file
027b5db: 8321215: Incorrect x86 instruction encoding for VSIB addressing mode
... and 82 more: https://git.openjdk.org/jdk/compare/c86431767e6802317dc2be6221a5d0990b976ddc...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-05T19:41:21Z

@bplb Pushed as commit b0d1450.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8321053: Use ByteArrayInputStream.buf directly when parameter of tran…

a42a45c

…sferTo() is trusted

openjdk bot added the rfr Pull request is ready for review label Nov 30, 2023

openjdk bot added the core-libs core-libs-dev@openjdk.org label Nov 30, 2023

jmehrens reviewed Nov 30, 2023

View reviewed changes

stsypanov reviewed Nov 30, 2023

View reviewed changes

8321053: Trust non-FilterOutputStreams in "java." packages

176d516

mkarg reviewed Dec 1, 2023

View reviewed changes

vlsi mentioned this pull request Dec 4, 2023

perf: improve performance of PreparedStatement.setBlob, BlobInputStream, and BlobOutputStream with dynamic buffer sizing pgjdbc/pgjdbc#3044

Merged

bplb added 2 commits December 4, 2023 11:21

8321053: Constrain trusted OutputStreams to a limited set

29ee889

8321053: instanceof -> ==

7aa3766

mkarg reviewed Dec 5, 2023

View reviewed changes

8321053: Remove package check in trustworthiness verification

8a13b55

AlanBateman approved these changes Dec 5, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 5, 2023

bplb added 2 commits December 5, 2023 09:44

8321053: s/tmp/tmpbuf/

b0d4cb9

8321053: Fix overlooked comment update in previous commit

0e14c97

openjdk bot added the integrated Pull request has been integrated label Dec 5, 2023

openjdk bot closed this Dec 5, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 5, 2023

bplb deleted the ByteArrayInputStream-transferTo-8321053 branch December 5, 2023 20:33

mkarg mentioned this pull request Dec 23, 2023

8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted #16879

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted #16893

8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted #16893

bplb commented Nov 30, 2023 •

edited by openjdk bot

bridgekeeper bot commented Nov 30, 2023

openjdk bot commented Nov 30, 2023

mlbridge bot commented Nov 30, 2023 •

edited

jmehrens Nov 30, 2023

bplb Nov 30, 2023

bplb Nov 30, 2023

jmehrens Dec 1, 2023 •

edited

bplb Dec 1, 2023

mkarg Dec 1, 2023

bplb Dec 1, 2023

bplb Dec 1, 2023 •

edited

jmehrens Dec 2, 2023 •

edited

bplb Dec 4, 2023

stsypanov Nov 30, 2023

bplb Nov 30, 2023

mkarg Dec 1, 2023

bplb Dec 1, 2023

mkarg Dec 1, 2023

mkarg Dec 5, 2023

AlanBateman Dec 5, 2023

mkarg Dec 5, 2023

bplb Dec 5, 2023

AlanBateman Dec 5, 2023

openjdk bot commented Dec 5, 2023 •

edited

bplb commented Dec 5, 2023

openjdk bot commented Dec 5, 2023

openjdk bot commented Dec 5, 2023

8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted #16893

8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted #16893

Conversation

bplb commented Nov 30, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 30, 2023

openjdk bot commented Nov 30, 2023

mlbridge bot commented Nov 30, 2023 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmehrens Dec 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bplb Dec 1, 2023 • edited

Choose a reason for hiding this comment

jmehrens Dec 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Dec 5, 2023 • edited

bplb commented Dec 5, 2023

openjdk bot commented Dec 5, 2023

openjdk bot commented Dec 5, 2023

bplb commented Nov 30, 2023 •

edited by openjdk bot

mlbridge bot commented Nov 30, 2023 •

edited

jmehrens Dec 1, 2023 •

edited

bplb Dec 1, 2023 •

edited

jmehrens Dec 2, 2023 •

edited

openjdk bot commented Dec 5, 2023 •

edited