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

[Elwyn Davies <[e-mail]>]

On Tue, 2014-06-17 at 06:52 -0400, Sean Turner wrote:
> On Jun 15, 2014, at 09:23, Elwyn Davies <[e-mail]> wrote:
> 
> > 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? 
> 
> 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.

Fair enough
> 
> > 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.  
> 
> I’ll leave this one to Peter.
> 
> > 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? 
> 
> 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]/.
> > 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.
> 
Fine!

Regards,
Elwyn

> spt