[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [pkcs11] Groups - TLS 1.2 mechanisms uploaded
On Fri, Aug 16, 2013 at 5:57 PM, Robert Relyea <rrelyea@redhat.com> wrote: > The following is Michael's latest draft after Wan-Teh's comments. The > only controversy seems to be the naming of the CKM_TLS12_MAC mechanism. > > I'm OK with Michael's name, and would like the attached version to go > forward for ballot. I divide my review comments into two sections. Section I is the substantive review comments. Section II covers nits and typos. For your convenience, I suggest fixes for the problems I found. I. Substantive review comments: 1. Naming: In the three new mechanism names "CKM_TLS12_MAC", "CKM_TLS10_MAC_SERVER", and "CKM_TLS10_MAC_CLIENT", the word "MAC" refers to the verify_data field in the TLS Finished message. But in the TLS protocol specification, the word "MAC" refers to the MAC in encrypted TLS records. This is also the meaning of "MAC" in the new mechanism name "CKM_TLS12_KEY_AND_MAC_DERIVE" in your proposal. Suggestion: use the term "VERIFY_DATA", which is used in the TLS protocol specification. Alternatively, add a word to clarify which MAC this is, for example, "FINISHED_MAC" or "HANDSHAKE_MAC". 2. Struct definition: the CK_TLS_KDF_PARAMS structure is defined for the keying material exporter of RFC 5705. Unfortunately RFC 5705 does not prohibit a present but zero-length context value. So pContextData=NULL, ulContextDataLength=0 can mean either "no context value" or a "zero-length context value". Suggestion: add a "CK_BBOOL hasContext" field to the CK_TLS_KDF_PARAMS structure. Both OpenSSL and NSS use this approach. Alternatively, specify that pContextData=NULL, ulContextDataLength=0 means "no context value" and pContextData=<a non-null pointer>, ulContextDataLength=0 means a zero-length context value. 3. Incomplete CKA_ALLOWED_MECHANISMS value: In both Sections 1.1.5 and 1.1.6, we have: The mechanism also contributes the CKA_ALLOWED_MECHANISMS attribute consisting only of CKM_TLS12_KEY_AND_MAC_DERIVE and CKM_TLS12_MAC. The list of allowed mechanisms for the master_secret key is incomplete. Suggestion: add CKM_TLS_KDF and CKM_TLS12_KEY_SAFE_DERIVE to the list of allowed mechanisms. 4. Design: CKM_TLS12_KEY_SAFE_DERIVE cannot be used to implement the AES-GCM cipher suites, which are the main reason people are interested in TLS 1.2 today. This means CKM_TLS12_KEY_SAFE_DERIVE will very likely be "dead on arrival". Suggestion: define CKM_TLS12_KEY_SAFE_DERIVE to output client_write_IV and server_write_IV as CKK_GENERIC_SECRET key objects. Alternatively, remove CKM_TLS12_KEY_SAFE_DERIVE from this proposal. 5. Design: a token that implements both CKM_TLS12_KEY_AND_MAC_DERIVE and CKM_TLS12_KEY_SAFE_DERIVE will still leak the private key data via CKM_TLS12_KEY_AND_MAC_DERIVE. Suggestion: add a warning about this, and recomend that if CKM_TLS12_KEY_SAFE_DERIVE is implemented, CKM_TLS12_KEY_AND_MAC_DERIVE should not be implemented. 6. Design: Section 1.1.9 (on the CKM_TLS_KDF mechanism) says: The mechanism should not be used with the labels defined for use with TLS, but the token does not enforce this behavior. If the token does not enforce this behavior, the private key data *could* be leaked by using the CKM_TLS_KDF mechanism with the "key expansion" label. To stop this leak requires the derived key have appropriate sensitivity and extractibility. Suggestion: either specify that the token should enforce this behavior, or ask someone to doublecheck that the rules about key sensitivity and extractability in Section 1.1.9 are correctly specified. II. Nits and typos: 7. Typo: After the definition of the CK_TLS12_KEY_MAT_PARAMS structure, we have CK_TLS_KEY_MAT_PARAMS_PTR is a pointer to a CK_TLS_KEY_MAT_PARAMS. Suggestion: add "12" after the two occurrences of "CK_TLS". 8. Nit: Section 1.1.3 has the sentence: They both calculate the verification data as described for TLS 1.0 and 1.1 with ... Suggestion: change "verification data" to "verify_data", the exact term used in TLS protocol specifications. Alternatively, say "the verification data in the Finished message" to provide context. 9. Nit: Section 1.1.3 has the sentence: In TLS 1.2 the "finished" message verify_data (i.e. the output signature from the MAC mechanism) is a minimum of 12 bytes, defaults to 12 bytes, but maybe negotiated to longer length. Suggestion: change it to In TLS 1.2 the "finished" message verify_data (i.e. the output signature from the MAC mechanism) defaults to 12 bytes, but may be negotiated to longer length. Note the "maybe" => "may be" change. 10. Nit: In Section 1.1.7, we have: The two MACing keys ("client_write_MAC_secret" and "server_write_MAC_secret") (if present) are always given a type of CKK_GENERIC_SECRET. The two MACing keys were renamed in TLS 1.2. Suggestion: use the new names in TLS 1.2, "client_write_MAC_key" and "server_write_MAC_key". 11. Nit: In Section 1.1.7, the last sentence in the black box warning says: Repeated calls with the same master key and same public data but with differing lengths for the private key material ... It is not clear what "public data" refers to. Suggestion: change "public data" to "RandomInfo". Wan-Teh Chang
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]