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

_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art

Reply via email to