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
