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 23:55:57 UTC 2013

I've pulled out things that need no further discussion.

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.

I will likely be putting back on Monday, maybe Tuesday. I'll try to coordinate with you over the weekend.

CipherBox.java ============== 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?

We don't have chaining constructor for ArrayIndexOutOfBoundsException class.

Well, that explains it. :) I wonder if there is already a RFE for this?

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?

Good catch. EC algorithm will be checked in KeyExchange, rather than BulkCipher. If EC algorithm is not available, EC cipher suites will be disabled. I worry about the RC4 cipher. The AES/CBC/128 is the minimal requirements of a crypto provider, so I don't worry about AES/CBC/128. However, RC4 is not in the minimal requirements of a crypto provider, so we should check the availability of RC4 here. I will file a new for this issue.

Thanks.

522: Why are you also checking for CipherType.AEADCIPHER?

AEAD/GCM is not implemented in all providers, for example the SunPKCS11 provider. So we need to check the available of AEAD implementation.

Of course, now I see it. The extra check for AES_256_GCM threw me. If you're also checking for AEAD, you probably don't need the separate check for AES_256_GCM. Maybe:

 if (this == B_AES_256 ||
     (this.cipherType == CipherType.AEAD_CIPHER)) {

is sufficient?

970: "before" -> "while"

It looks strange to me with two whiles: // ..., we decrease the // priority of cipher suites in GCM mode for a while while GCM // technologies become mature in the industry. I think "before" may be better word here.

Sorry, definitely not two whiles. I think I probably meant to say "as" but obviously didn't.

Maybe:

 // Placeholder for cipher suites in GCM mode.
 //
 // For better compatibility and interoperability, we decrease the
 // priority of cipher suites in GCM mode for a while as GCM
 // technologies mature in the industry.  Eventually we'll move
 // the GCM suites here.

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.

I'm not sure I catch your ideas. ;-) Would you please show me the code?

Just a simple reversal of the lines so that the code you're talking about is contained in the block that handles it:

 if (!writeCipher.isAEADMode()) {
     // DON'T encrypt the nonce_explicit for AEAD mode
     dstBB.position(dstPos + headerSize);
 }   // The explicit IV in TLS 1.1 and later can be encrypted.

Hope that's clearer.

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() == ctchangecipherspec))))) { Is my thinking incorrect? In the old code, there is no dump for "handshake" level log unless it is a changecipherspec message. However, the log is dumped for "record" level log. I think it was right because the log here is record information, but not handshake message. I think the update does not changes the behavior.

You are correct, the actual update does not change the observed behavior, but I had to write an app to prove it to myself.

However, this code is not following the failfast paradigm of Debug, though it does give the same answer because of the way the Debug.isOn() code was written. debug!=null is supposed to be a lightweight check to see if any Debug options are on, and if so, then do the more heavyweight checks.

Abstracting a bit, test1 is the "debug!=null" test. The resulting code is:

     if (test1() && test2() || test3() && test4()) {
         System.out...
     }

So if test1 fails, then we jump to the "test3 && test4" case, which then potentially has to go through the isOn processing twice. Granted, because of the way isOn was written (if args == null) it's not adding a lot of overhead or throwing NullPointers, but it's not following the paradigm.

Looking a little more closely, I think the original logic was flawed.

 if ((debug && record) || handshake)
     if ((debug && record) || CCS)

both handshake/CCS are outside the debug check. I think what was intended was:

 if (debug && (record || (handshake && CCS)))

My underlying original comment remains: please add parens here to make it clear what you are intending.

(P.S. If you want my test code, it's in my homedir under tmp/Template.java.)

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. Yes. 2. If a remote server selects a GCM ciphersuite and a version other than TLS 1.2, our client will send an alertillegalparameter. 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? Good catch. We did not check the cipher suite and protocol version together. Need to address this in a new bug so that we can backport the update.

Not following what you mean by "backport the update"? GCM only exists in 8 at the moment. Or do you mean check the other TLS 1.2 ciphersuites besides GCM?

InputRecord.java ================ 198: Didn't follow this comment "before the exception occurs"? Before the MAC occurs? Or in the case of an exception in signer.compute.

Need more comments here: count -= macLen; // Set the count before any MAC checking // exception occurs, so that the following // process can read the actual decrypted // content (minus the MAC) in the fragment // if necessary.

Thanks.

Hope these comments are helpful to you.

Brad

More information about the security-dev mailing list