RFR 8211018: Session Resumption without Server-Side State

[Xuelei Fan]

On 6/6/2019 9:46 PM, Anthony Scarpino wrote:
> On 6/5/19 9:58 AM, Xuelei Fan wrote:
>> Continue for the SessionTicketExtension.java.
>>
>> On 6/3/2019 5:42 PM, Anthony Scarpino wrote:
>>> http://cr.openjdk.java.net/~ascarpino/stateless/webrev.02
>>>
>> SessionTicketExtension.java (continue):
>> ---------------------------------------
>>   231         SessionStateSpec(ByteBuffer buf) throws IOException {
>>   ...
>>   240             sessiondata = buf;
>> The local filed 'sessiondata' shares the 'buf' object.  While the 
>> 'buf' object is mutable, and could be updated later.  The 
>> SessionStateSpec was used after the construction.  It looks like a 
>> potential issue.
> 
> Are you saying this because you have seen the bytebuffer underneath 
> changed by the TLS code for other operations, or is it just because the 
> object definition is mutable?   Does the ByteBuffer extra data in there 
> that it should not?
> 
There are two potential issues of the line:
     240             sessiondata = buf;

the 'buf' is the input parameters, update the 'buf' will updated the 
'sessiondata", and update the 'sessiondata' will update the 'buf' as well.

When I reviewed this line, I have to search back to the full calling 
stacks of the method, and make sure all of the super callers does not 
modify the 'buf'.  Although I made the search, I'm still not very sure 
that no super caller updates the 'buf'.

For 'sessiondata', the underneath operation could change it.  For 
example, the following line will change the current position of sessiondata:
     284             int curkey = sessiondata.getInt();

Please search for sessiondata for more changes of the object.

> I've gone through and redid a lot of this, but it creates an extra copy 
> of the ~700byte array, and I don't like it and want to revert back.   So 
> a lot of the below bytebuffer comments below are unanswered
> 
Maybe ByteBuffer.duplicate() could be used.  This method shares the same 
backend array, but using different pointers (position, limit, etc).

I'm not sure how many times the decrypt() method get used. 
Alternatively, it might be possible to store the plaintext of the ticket 
rather than the ciphertext of it.

>>
>>   270                 System.arraycopy(iv,0, result, Integer.BYTES, 
>> iv.length);
>>   271                 System.arraycopy(encrypted,0, result,
>> It's nice to add a whitespace before '0'.
>>
>>   258                 SecureRandom random = 
>> hc.sslContext.getSecureRandom();
>>   259                 random.nextBytes(iv);
>> The IV is not necessarily be random number, it's good as if it is 
>> unique.  Random number generation could be slow, I may use a sequence 
>> number as the IV.
> 
> Pseudo random is not slow,  I don't feel comfortable using some 
> manufactured IV for many of the bytes may not be unique.
> 
The secure random (sslContext.getSecureRandom()) could be configured by 
applications.  I'm not confident that the random number generation is 
not slow or cannot be blocked.

Manufactured IV are used a lot in the TLS protocols. But I understand 
your point, I'm fine if you want to use secure random.

Thanks,
Xuelei