[Gen-art] Gen-art LC review of draft-ietf-tls-encrypt-then-mac-02

[Elwyn Davies <[e-mail]>]

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-tls-encrypt-then-mac-02.txt
Reviewer: Elwyn Davies
Review Date: 15 June 2014
IETF LC End Date: 20 June 2014
IESG Telechat date: (if known) -

Summary: Almost ready.

Major issues:

Minor issues:
Header/Abstract/Intro: Doesn't this update RFC 6347 and either or both
of RFC 6066 and RFC 5246 since it defines a new extension? 

s3.1: [I am not a TLS expert so I may not have got this right...] If the
rehandshake proposes to use a cipher that doesn't need encrypt-then-mac
as per GenericStreamCiphers, or GenericAEADCiphers as mentioned in s3,
then presumably this is allowed whether encrypt-then-mac had or had not
been negotiated previously on the session.  It should be clarified
whether this is allowed and, if it isn't, what the response should be.

If it is allowed and the session switches to a GenericStreamCipher, or a
GenericAEADCipher, then it appears that if the original session was
using encrypt-then-mac, a downgrade to a cipher that does need
encrypt-then-mac but isn't using it could be achieved by using a second
rehandshake without encrypt-then-mac since the first rehandshake goes to
a situation that doesn't use encrypt-then-mac after the first
rehandshake.   The solution seems to be to record whether the session
ever used encrypt-then-mac rather than whether is is currently using it,
and then apply the rules in s3.1 based on the record.  

Nits/editorial comments:
s3: (very nitty nit)  I think the MAC calculation piece would be clearer
with what it updates noted before the update. Thus:

OLD
In TLS [2] notation the MAC calculation is:

   MAC(MAC_write_key, seq_num +
       TLSCipherText.type +
       TLSCipherText.version +
       TLSCipherText.length +
       ENC(content + padding + padding_length));

   for TLS 1.0 without the explicit IV and:

   MAC(MAC_write_key, seq_num +
       TLSCipherText.type +
       TLSCipherText.version +
       TLSCipherText.length +
       IV +
       ENC(content + padding + padding_length));

   for TLS 1.1 and greater with explicit IV (for DTLS the sequence
   number is replaced by the combined epoch and sequence number as per
   DTLS [4]).

NEW:
In TLS [2] notation the MAC calculation is:
  o For TLS 1.0 without the explicit IV

   MAC(MAC_write_key, seq_num +
       TLSCipherText.type +
       TLSCipherText.version +
       TLSCipherText.length +
       ENC(content + padding + padding_length));

  o For TLS 1.1 and greater with explicit IV

   MAC(MAC_write_key, seq_num +
       TLSCipherText.type +
       TLSCipherText.version +
       TLSCipherText.length +
       IV +
       ENC(content + padding + padding_length));

  o For DTLS the sequence number in the TLS 1.1 description is replaced 
    by the combined epoch and sequence number as per DTLS [4]).

s3, next to last para:
> For DTLS, the record MUST be discarded and a fatal bad_record_mac MAY
>    be generated [2].
Shouldn't the reference here be [4] for DTLS? 

s3.1 (last para)/s7.1: Shouldn't the TLS_ext reference [3] be to the updated RFC 6066?