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

Xuelei Fan xuelei.fan at oracle.com
Fri Jan 18 17:09:09 UTC 2013

webrev: http://cr.openjdk.java.net./~xuelei/7030966/webrev.02/

No significant changes in the new webrev.

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. Good!

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. I usually use 4 spaces for new lines. But if the next line is a continuation of the previous line, I did not follow the 8 spaces strictly.

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. I just want to use the tag in order to make the comments clear and easy understood.

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.

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.

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.

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.

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 TLSECDHEECDSAWITHAES128GCMSHA256 or the TLSECDHEECDSAWITHAES256GCMSHA384 cipher suite in the ClientHello message. The TLSECDHEECDSAWITHAES128GCMSHA256 cipher suite is preferred; if offered, it MUST appear before the TLSECDHEECDSAWITHAES256GCMSHA384 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. ;) You are right. The current default cipher suites preference is not Suite-B/RFC6460 compliant. We need additional work to configure the cipher suites in order to comply with RFC 6460. We may want to add a new feature in the future.

EngineInputRecord.java ====================== 191: Your comment is a little confusing here. Does this mean it does this internally? Need to remove this line. Removed.

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?

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 change_cipher_spec 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.

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.

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.

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. Good point. Need to address this in a new bug so that we can backport the update.

Thanks, Xuelei

More information about the security-dev mailing list