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

From: Elwyn Davies <[email-masked]>
Date: Sun, 15 Jun 2014 14:23:44 +0100

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?

---

From: Sean Turner <[email-masked]>
Date: Tue, 17 Jun 2014 06:52:40 -0400

> of RFC 6066 and RFC 5246 since it defines a new extension? 

My feeling is that it doesn’t update any of the drafts.  It documents a new optional extensions that implementations are free to implement so it need not update the base specs.  WRT including all the extensions in one draft, RFC 6066 defines many extensions but not all:

http://datatracker.ietf.org/doc/rfc6520/
http://datatracker.ietf.org/doc/rfc6961/

this is just another one.

> and then apply the rules in s3.1 based on the record.  

I’ll leave this one to Peter.

> Shouldn't the reference here be [4] for DTLS? 

Nope - the bad_record_mac is defined in [2].  DTLS and TLS share some things: e.g., alert protocol values.

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

At first I thought the answer was no, but since three people have commented on it I guess should be updated.

spt

---

From: Elwyn Davies <[email-masked]>
Date: Tue, 17 Jun 2014 12:40:58 +0100

> this is just another one.

Fair enough
> Nope - the bad_record_mac is defined in [2].  DTLS and TLS share some things: e.g., alert protocol values.
True; However s4.1.2.1 of [4] has quite a lot to say about exactly this
situation.  I'd be inclined to s/[2]/as discussed in Section 4.1.2.1 of
[4]/.
> At first I thought the answer was no, but since three people have commented on it I guess should be updated.
Fine!

Regards,
Elwyn