On Fri, 1 Dec 2023 02:48:42 GMT, Brian Burkhalter <b...@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
