Code Review Request for 7030966, Support AEAD CipherSuites (JSSE part of JEP 115) (original) (raw)

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