RFR: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted [v2]

[jmehrens]

On Fri, 1 Dec 2023 02:48:42 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

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

@bplb You did it right.  The reason it works is because the ChannelOutputStream is in the "sun." package and not the "java." package.  That is not the case for Channels.newOutputStream(AsynchronousByteChannel ch) as that wrapper should be able to access the byte array.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16893#discussion_r1411575001