Code Review Request for 7030966, Support AEAD CipherSuites (JSSE part of JEP 115)
Brad Wetmore
bradford.wetmore at oracle.com
Fri Jan 18 01:13:06 UTC 2013
Hi Xuelei,
Minor stuff. A couple things to check.
Xuelei wrote:
>>> Hi Valerie, Max or Brad,
>>>
>>> Can you review the update for JDK-7030966? It is the JSSE part of JEP 115.
>>>
>>> webrev: http://cr.openjdk.java.net./~xuelei/7030966/webrev.00/
>>> JEP 115: http://openjdk.java.net/jeps/115
I looked at webrev.01.
>>> In the update, I have not remove the debug synchronization. I will
>>> remove them before pushing the changeset.
I will be doing a putback of the JCE providers, so I can do your SunJCE
signed provider putback for you. Let's coordinate when you are ready.
I will probably be ready early next week.
Minor nits:
===========
What is the indentation style you are using? I noticed a few places
where it's not 4/8 spaces, and it's not lining up with anything on the
previous line. For example CipherSuite.java:522:544.
Also, in several of your new methods, you are writing pseudo-javadoc
style (@return/etc), but you're not starting the comment with "/**".
This probably doesn't matter since we don't generate javadoc for
internal classes. Just mentioning since you took the time to format
it and hoping it would render.
TlsKeyMaterialGenerator.java
============================
OK
TlsKeyMaterialParameterSpec.java
================================
OK
TlsKeyMaterialSpec.java
=======================
OK
P11TlsKeyMaterialGenerator.java
===============================
OK
CipherBox.java
==============
In various places, you mention RFC 5246 (TLS 1.2), but it also applies
to RFC 4346 (TLS 1.1). For example in getExplicitNonceSize(). You
might want to mention both RFCs in these places for completeness and to
avoid confusion.
180: We only need to keep the key around for AEAD, so saving a copy
here for CBC prevents GC of an object we'll never use again. In your
comment of 126, you also mention that it's reserved for AEAD, so I would
not expect to see this key nonnull for anything besides AEAD.
131/205: In a similar vein, tagSize is only used for AEAD, so you
could initialize tagSize to 0 and then move line 205 inside the "if
(AEAD)" arm. Not critical, but it seems incorrect to have a regular
AES/CBC CipherBox with a tagsize of 16.
214/218: Same comment about fixedIV/recordIVSize. Only used in AEAD.
No need to keep around this dummy array or calculate a value we don't
need.
207/215: Some additional commenting of the AEAD vs. CBC Cipher init
logic would be helpful here for people trying to understand the code.
Something like:
AEAD must completely initialize the cipher for each packet, and so
we save initialization parameters for packet processing time.
CBC only requires one initialization during its lifetime (future
packets/IVs set the proper CBC state), so we can initialize now.
298: Exception chaining for debugging? Minor nit: since you're using
the new multi-catch Exceptions here, maybe a name change (ibse) is in
order?
312/564: I don't remember why we decided to use the RuntimeException
AIOOBE here. Ugh...anyway, can you also use exception chaining to help
debug any reported problems?
361: Variable name OutputSize is capitalized.
774/803: Minor nit (take or leave): you might want to change "explicit
nonce" -> "nonce_explicit" for readers are following the RFC. The
others are fine.
790/849: "alsoe" -> "also"
798: "Applys" -> "Applies"
825: Your exception description just mentions nonces, but your check
is for both nonce/tagSize. This description is incomplete.
CipherSuite.java
================
510: I don't think this statement is true anymore. This is likley a
carryover from the 1.4 days when we used to have a crypto provider in
the JSSE jar file. Not for this review, but maybe we should actually
check that implementations are available? If we remove SunEC and
SunPKCS11 providers from the provider list, could we potentially
disable the EC suites?
522: Why are you also checking for CipherType.AEAD_CIPHER?
577: If you actually pulled out something for b in 538, you're putting
it right back in at 577. You should probably have the second if
completely inside the first and move 577 inside that if.
Boolean b = availableCache.get(cipher);
if (b == null) {
if (cipher.keySize > 128)
...deleted...
if (b == null) {
b = Boolean.FALSE;
...deleted...
}
availableCache.put(cipher, b);
}
return b.booleanValue();
540-550: Question: is this just an optimization to see if it's > 128
before actually trying it at 566? You might mention that.
970: "before" -> "while"
1054: From http://www.rfc-editor.org/rfc/rfc6460.txt
A Suite B TLS client configured at a minimum level of security of 128
bits MUST offer the TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 or the
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 cipher suite in the
ClientHello message. The TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
cipher suite is preferred; if offered, it MUST appear before the
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 cipher suite.
I would have thought what you have is the correct order, but it
doesn't follow the RFC. Please add a note to remind us, because this
order will not make sense in 6 months. ;)
EngineInputRecord.java
======================
191: Your comment is a little confusing here. Does this mean it does
this internally?
193: Great comment. Thank you!
213: Also RFC 5246 6.2.3.3 also says that a bad_record_mac should be
sent. Although you catch this and send that in the upper layers, you
might mention this here.
219: authenticatoin -> authentication
EngineOutputRecord.java
=======================
294/296: Another great comment. I might suggest reversing the
comments so that the comment about AEAD is in the AEAD arm, and CBC is
outside.
306: The original code was bad (double debug != null :) ), and I
realize the original code was lacking in parens "()", but can you
please add parens to indicate exactly what expression order you intend
here. My head is spinning from parsing the various cases: I'm not sure
the logic here is correct. I think we should output if debug is on
and either handshake/record is active or we're outputing a CCS. That
is:
if ((debug ! null) &&
((Debug.isOn("record") || (Debug.isOn("handshake") ||
(contentType() == ct_change_cipher_spec))))) {
Is my thinking incorrect?
EngineWriter.java
=================
OK
Handshaker.java
===============
This is a comment for ClientHandshaker/Handshaker. I just want to
confirm some restrictions from Section 4 of RFC 5288:
1. If a remote client offers <= TLS 1.1 and a GCM suite, we won't
possibly select a GCM suite. I'm pretty sure this is ok, just wanted
to check.
2. If a remote server selects a GCM ciphersuite and a version other
than TLS 1.2, our client will send an alert_illegal_parameter. I
think this is done at ClientHandshaker:461 calling into
Handshaker:isNegotiable(), but the comments in getActiveCipherSuites()
only talk about the initial request, not the CipherSuite we actually
get back from the server. Can you please double check this?
InputRecord.java
================
166: Another good comment. You're on a roll! ;)
174: Same comment about later RFCs (See first comment in CipherBox)
198: Didn't follow this comment "before the exception occurs"? Before
the MAC occurs? Or in the case of an exception in signer.compute.
JsseJce.java
============
OK
Authenticator.java
==================
OK
MAC.java
========
OK
OutputRecord.java
=================
74: Thanks for the IM session yesterday, just reading this comment I
wasn't following the point of this "unused header". My comment here is
to point out why you have this unused header. One can eventually
figure it out, but it would have been much quicker to have this right
up front. Something like: When this is object is created, we don't
know the protocol version #/IV length/etc., so reserve space in front
to avoid extra data movement (copies).
81: records -> record
193: Could you put in the comments what description is?
You're asking "is this alert of type 'description'"?
236: Please add ()'s.
250/252: Same comment about reversing the comments.
Record.java
===========
55: Never noticed this before. Is maxIVLength really 256, or were you
just being overly cautious? All of our current block ciphers (e.g.
AES-CBC) max out at 16 bytes, and there's only 8 bytes for TLS GCM mode
nonce-explicit? For each OutputRecord, I think you're reserving an
extra 245 bytes here that will never be used. (5 + 256 - 16) If
you're nervous about future suites bumping the size, you could add a
simple check during CipherSuite.java initialization.
70: The GCM tag size hasn't been mentioned anywhere in this class. You
might include a few words to that effect. Even though the tag lives in
the Data section, you might talk about it in the MAC section.
SSLEngineImpl.java
==================
OK
SSLSocketImpl.java
==================
OK
I didn't check the tests.
Thanks,
Brad
More information about the security-dev
mailing list