Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

On 6/6/2018 1:11 PM, Anthony Scarpino wrote:
> I can make the below changes if they are accepted.
> 
Please take care of the update, except the fragmentSize for 
InputRecord.java.

> Tony
> ------
> 
> InputRecord.java
> - Optimize Imports
> - fragmentSize appears not to be used.  It is constructed,
> it can be set by changeFragmentSize.  MaxFragExtension even calls these 
> methods, but no where can I find that uses the fragmentSize.  It appears 
> the fragment sizes are either calculated in the SSLCipher or the 
> negotiatedMaxFragLen is used in the handshakeSession.
> 
I need more time for this point.

> 65: getHelloVersion() is unused
> 
> - For the below, I would assume integer overflow isn't an issue since 
> these are all internal APIs and the worse we would see is an Out of 
> Bounds exception
> 
> 337:   for (int i = offset, j = 0;
> 338:        i < (offset + length) && j < headerSize; i++) {
> 
> 359:        for (int i = offset; i < offset + length; i++) {
> 
> 373:        for (int i = offset; i < offset + length; i++) {
> 
Yes. These integers have been checked before calling into this method.

> 
> 
> DTLSInputRecord.java
> - Optimize Imports
> 102         } else if (srcsLength == 1) {
> 103             return decode(srcs[srcsOffset]);
> 
> shouldn't it be decode(srcs[0]); otherwise it could be out of bounds.
> 
The 'srcsLength' refer to the length from the offset.  I think 
srcsOffset should be used as the index.

> 
> 
> SSLEngineInputRecord.java
> - Optimize Imports
> 41:  prevType never used
> 347: srcLimit never used
> 
> 42 & 43:  hsMsgOff and hsMsgLim are set to 0 and never change, later 
> they are used in a check with themselves:
>       232:  if ( ... && hsMsgOff != hsMsgLen) {
> 
I missed the cleanup.  hsMsgOff and hsMsgLen are not used any more. 
"hsMsgOff != hsMsgLen" may be replaced with something like 
handshakeBuffer.hasRemaining() or similar.

> 
> 
> SSLSocketInputRecord.java
> - Optimize Imports
> 47: prevType never used
> 48 & 49:  nsMsgOff and hsMsgLim are set to 0 and never change, later 
> they are used in a check with themselves:
>       266:  if ( ... && hsMsgOff != hsMsgLen) {
> 
As above.

Thanks,
Xuelei