On Thu, 30 Nov 2023 23:27:13 GMT, Brian Burkhalter <b...@openjdk.org> wrote:
>> Good catch: that in fact defeats the protection.
>
> Changed in 176d5165f7d8f3fa4814c9838abb5d18d9f3c338 not to trust
> `FilterOutputStream`s.
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);
Which leads us back to an allow list that Alan was talking about and the draw
backs that it brings.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16893#discussion_r1411491609
