On Sat, 2 Dec 2023 03:48:46 GMT, jmehrens <d...@openjdk.org> wrote:
>> 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 `OutputStream`s
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16893#discussion_r1414382406
