Code review request, 7200295 CertificateRequest message is wrapping when using large numbers of Certs

[Christophe Ravel]

Hi Andrew,

You need to add a regression test for this fix.

Regards,
Christophe.

On Sep 24, 2012, at 7:01 PM, Xuelei Fan <Xuelei.Fan at oracle.COM> wrote:

> On 9/25/2012 9:23 AM, Brad Wetmore wrote:
>> Are there situations where we might overflow the int?
>> 
> Yes, it is possible for many integer add operations. As 2^32 is a lot
> bigger than 2^24 (the biggest number TLS protocol allows), I'm not
> worried too much about int32 overflow.
> 
> Integer overflow checking would make the code ugly. For example,
> normally, we do add operations as:
>   int result = 1 + len + anotherLen;
> 
> if we want to check overflow, the code would look like:
>   int result = 1;
>   if (result > Integer.MAX_VALUE - len) {
>       result += len;
>   } else {
>       // overflow
>   }
> 
>   // the same for anotherLen
> 
> I did not think it is necessary.
> 
>> For example, in CertificateRequest.messageLength()
>> 
>>        for (int i = 0; i < authorities.length; i++) {
>>            len += authorities[i].length();
>>        }
>> 
>> What if len overflows?
>> 
>> Also, all of these field's callers are overflow-1?
>> 
> I'm not sure I get your point. In RFC5246, exception session ID, other
> variable length is one of 2^8-1, 2^16-1 or 2^24 -1.
> 
> Xuelei
> 
>> Brad
>> 
>> 
>> 
>> 
>> On 9/23/2012 7:42 PM, Xuelei Fan wrote:
>>> Hi,
>>> 
>>> Please review the update to check output filed length overflow in TLS
>>> handshaking.
>>> 
>>> bug   : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7200295
>>> webrev: http://cr.openjdk.java.net/~xuelei/7200295/webrev.00/
>>> 
>>> The cause of the bug is that for 8, 16, 24 bits length-variable fields,
>>> before put the bytes into the fields, we do not check that the length of
>>> the bytes is less than the capabilities of the field.
>>> 
>>> Thanks,
>>> Xuelei
>>> 
> 

Christophe Ravel | Principal Member of Technical Staff | +1.650.506.2162
Oracle Java SQE - Security
4220 Network Circle, B160A, Santa Clara, CA