RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Adam Petcher
This is a bug fix for a corner case in which a DER value has length
equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read
the value, which interprets length=Integer.MAX_VALUE to mean "read to
the end." The result is that no exception will be thrown when fewer then
Integer.MAX_VALUE bytes are read from the stream. The fix adds a check
after the readFully() to ensure that the expected number of bytes were
read.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8183591

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Bernd Eckenfels-4
BTW: Can in.available() be < length as well? In that case then exception before your changed line would be misleading.

Gruss
Bernd


From: security-dev <[hidden email]> on behalf of Adam Petcher <[hidden email]>
Sent: Wednesday, July 12, 2017 8:38:25 PM
To: [hidden email]
Subject: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length
 
This is a bug fix for a corner case in which a DER value has length
equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read
the value, which interprets length=Integer.MAX_VALUE to mean "read to
the end." The result is that no exception will be thrown when fewer then
Integer.MAX_VALUE bytes are read from the stream. The fix adds a check
after the readFully() to ensure that the expected number of bytes were
read.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8183591

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Adam Petcher

On 7/12/2017 6:27 PM, Bernd Eckenfels wrote:

BTW: Can in.available() be < length as well? In that case then exception before your changed line would be misleading.


Yes. I changed the text of that exception to make it a bit more general, and made the text of the new exception match.

New webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.01/

Gruss
Bernd


From: security-dev [hidden email] on behalf of Adam Petcher [hidden email]
Sent: Wednesday, July 12, 2017 8:38:25 PM
To: [hidden email]
Subject: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length
 
This is a bug fix for a corner case in which a DER value has length
equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read
the value, which interprets length=Integer.MAX_VALUE to mean "read to
the end." The result is that no exception will be thrown when fewer then
Integer.MAX_VALUE bytes are read from the stream. The fix adds a check
after the readFully() to ensure that the expected number of bytes were
read.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8183591


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Adam Petcher
In reply to this post by Adam Petcher
Some additional investigation revealed that IOUtils.readFully() is only
used by DER, JKS, and Kerberos. None of these need the "read to the end
of the buffer" feature. This behavior of readFully() is confusing, so it
is probably best to remove it.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.01/


On 7/12/2017 2:38 PM, Adam Petcher wrote:

> This is a bug fix for a corner case in which a DER value has length
> equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read
> the value, which interprets length=Integer.MAX_VALUE to mean "read to
> the end." The result is that no exception will be thrown when fewer
> then Integer.MAX_VALUE bytes are read from the stream. The fix adds a
> check after the readFully() to ensure that the expected number of
> bytes were read.
>
> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
> JBS: https://bugs.openjdk.java.net/browse/JDK-8183591
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Adam Petcher
Oops. Better to throw an IOException when a negative length is given to
readFully.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.02/


On 7/18/2017 1:55 PM, Adam Petcher wrote:

> Some additional investigation revealed that IOUtils.readFully() is
> only used by DER, JKS, and Kerberos. None of these need the "read to
> the end of the buffer" feature. This behavior of readFully() is
> confusing, so it is probably best to remove it.
>
> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.01/
>
>
> On 7/12/2017 2:38 PM, Adam Petcher wrote:
>> This is a bug fix for a corner case in which a DER value has length
>> equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read
>> the value, which interprets length=Integer.MAX_VALUE to mean "read to
>> the end." The result is that no exception will be thrown when fewer
>> then Integer.MAX_VALUE bytes are read from the stream. The fix adds a
>> check after the readFully() to ensure that the expected number of
>> bytes were read.
>>
>> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8183591
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Bernd Eckenfels-4
In reply to this post by Adam Petcher
Why not make a different utility method for this case.

 readRemaining() vs. readFully(int)

The name makes not much sense and the code does not get easier if both cases are in one method for no good reason.

And I wonder if allocating a MAXINTEGER buffer from untrusted source is a good idea.

Gruss
Bernd

2017-07-20 15:49 GMT+02:00 Adam Petcher <[hidden email]>:
Oops. Better to throw an IOException when a negative length is given to readFully.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.02/



On 7/18/2017 1:55 PM, Adam Petcher wrote:
Some additional investigation revealed that IOUtils.readFully() is only used by DER, JKS, and Kerberos. None of these need the "read to the end of the buffer" feature. This behavior of readFully() is confusing, so it is probably best to remove it.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.01/


On 7/12/2017 2:38 PM, Adam Petcher wrote:
This is a bug fix for a corner case in which a DER value has length equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read the value, which interprets length=Integer.MAX_VALUE to mean "read to the end." The result is that no exception will be thrown when fewer then Integer.MAX_VALUE bytes are read from the stream. The fix adds a check after the readFully() to ensure that the expected number of bytes were read.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8183591




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Adam Petcher

On 7/20/2017 1:32 PM, Bernd wrote:

Why not make a different utility method for this case.

 readRemaining() vs. readFully(int)

The name makes not much sense and the code does not get easier if both cases are in one method for no good reason.

I agree, but the method that takes a length argument is the only one that is needed at the moment. We can add another method later if it is needed. Are you saying there is something wrong with the readFully(int) method in my last webrev?


And I wonder if allocating a MAXINTEGER buffer from untrusted source is a good idea.

This method will only allocate a large buffer if the untrusted source actually sent a large amount of bytes. 


Gruss
Bernd

2017-07-20 15:49 GMT+02:00 Adam Petcher <[hidden email]>:
Oops. Better to throw an IOException when a negative length is given to readFully.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.02/



On 7/18/2017 1:55 PM, Adam Petcher wrote:
Some additional investigation revealed that IOUtils.readFully() is only used by DER, JKS, and Kerberos. None of these need the "read to the end of the buffer" feature. This behavior of readFully() is confusing, so it is probably best to remove it.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.01/


On 7/12/2017 2:38 PM, Adam Petcher wrote:
This is a bug fix for a corner case in which a DER value has length equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read the value, which interprets length=Integer.MAX_VALUE to mean "read to the end." The result is that no exception will be thrown when fewer then Integer.MAX_VALUE bytes are read from the stream. The fix adds a check after the readFully() to ensure that the expected number of bytes were read.

Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8183591





Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Sean Mullan
In reply to this post by Adam Petcher
Looks good to me.

--Sean

On 7/20/17 9:49 AM, Adam Petcher wrote:

> Oops. Better to throw an IOException when a negative length is given to
> readFully.
>
> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.02/
>
>
> On 7/18/2017 1:55 PM, Adam Petcher wrote:
>> Some additional investigation revealed that IOUtils.readFully() is
>> only used by DER, JKS, and Kerberos. None of these need the "read to
>> the end of the buffer" feature. This behavior of readFully() is
>> confusing, so it is probably best to remove it.
>>
>> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.01/
>>
>>
>> On 7/12/2017 2:38 PM, Adam Petcher wrote:
>>> This is a bug fix for a corner case in which a DER value has length
>>> equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read
>>> the value, which interprets length=Integer.MAX_VALUE to mean "read to
>>> the end." The result is that no exception will be thrown when fewer
>>> then Integer.MAX_VALUE bytes are read from the stream. The fix adds a
>>> check after the readFully() to ensure that the expected number of
>>> bytes were read.
>>>
>>> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8183591
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8183591: Incorrect behavior when reading DER value with Integer.MAX_VALUE length

Weijun Wang
In reply to this post by Adam Petcher

> On Jul 19, 2017, at 1:55 AM, Adam Petcher <[hidden email]> wrote:
>
> Some additional investigation revealed that IOUtils.readFully() is only used by DER, JKS, and Kerberos. None of these need the "read to the end of the buffer" feature. This behavior of readFully() is confusing, so it is probably best to remove it.

Just back from vacation.

Yes, you are right. I filed https://bugs.openjdk.java.net/browse/JDK-8182151 some time ago. IIRC, with the proposed InputStream::readNBytes(int length) and existing InputStream::readAllBytes(), there will be no need to call IOUtils.readFully() anymore.

Thanks
Max

>
> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.01/
>
>
> On 7/12/2017 2:38 PM, Adam Petcher wrote:
>> This is a bug fix for a corner case in which a DER value has length equal to Integer.MAX_VALUE. The code uses IOUtils.readFully() to read the value, which interprets length=Integer.MAX_VALUE to mean "read to the end." The result is that no exception will be thrown when fewer then Integer.MAX_VALUE bytes are read from the stream. The fix adds a check after the readFully() to ensure that the expected number of bytes were read.
>>
>> Webrev: http://cr.openjdk.java.net/~apetcher/8183591/webrev.00/
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8183591
>>
>

Loading...