Re: [pkcs11] Groups - TLS 1.2 mechanisms uploaded

[Wan-Teh Chang <wtc@google.com>]

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