Re: [TLS] rfc7366: is encrypt-then-mac implemented?

[Manuel Pégourié-Gonnard <[e-mail]>]

On 01/11/2014 20:25, Yngve N. Pettersen wrote:
> For reference, I have tried to implement RFC 7366 in my TLS Prober, but I  
> have not been able to establish a connection with the "eid" server. It  
> seems to have some non-standard requirements to the client that AFAICT  
> have not been documented anywhere.
>
IIRC, I found out that it doesn't like 03 00 as the record-level version number
in the ClientHello. I was able to connect by setting the minimum version of my
test client to TLS 1.0, which upped the record-level version number of the
ClientHello to 03 01.

> I am starting to wonder if the confusion about this spec update is partly  
> due to the way the record structure is defined, with the length separate  
>  from the data vectors. This separation have been a problem in connection  
> with at least some extensions.
> 
Out of curiosity, which ones?

> The way a TLS Protocol record is defined in previous versions (Using TLS  
> 1.2), before this update is, can a generic way be rewritten as:
> 
>    struct {
>         ContentType type;
>         ProtocolVersion version;
>         opaque record_data<0..maxlength>
>    } TLS_Record;
> 
> This is how all clients and servers will parse current SSL and TLS  
> versions' records on the wire, and also how all the intermediate servers  
> (the ones that are causing concern for version rollback recovery) will be  
> parsing the TLS traffic.
> 
Agreed so far. (And AFAIK all implementations of RFC 7366 agreed on this point.)

> The field record_data containing a strucure of exactly the length of  
> record_data
> 
>    struct {
>      select(encryption_mode) {
>         case plaintext: TLS_Plaintext_data;
>         case stream_encrypted: TLS_Stream_Encrypted_data;
>         case block_encrypted: TLS_Block_Encrypted_data;
>      }
>    } TLS_Record_payload;
> 
And AEAD, and plaintext can be seen as a special case of Stream.

> With RFC7366 the format for the encrypted record payloads got changes, and  
> it seems to me that the problem is a disagreement of what the length field  
> in the TLSCipherText record applies to.
> 
As far as I'm concerned, I've always been convinced that it was the length of
the rest of the record (in that case, padded-encrypted stuff plus unencrypted
MAC), and I've only mentioned alternative hypothesis as a sort of reductio ad
absurdum.

>    struct {
>       opaque IV[SecurityParameters.record_iv_length];
>       block-ciphered struct {
>         opaque  
> fragment[TLS_Record.record_data.length-SecurityParameters.record_iv_length-MAC-paddinglength-1];
>         uint8 padding [paddinglength];
>         uint8 paddinglength;
>       } encrypted;
>       opaque MAC[MAClength];
>    } TLS_EtM_Block_Encrypted_data;
> 
I find this one complicated: to me it looks more natural to defined the length
of the record as content length + padding length + etc. than otherwise.

> I think one thing that is to be determined is whether this rewritten  
> structure definitions accurately describes both the old format and the new  
> RFC 7366 format. Do they?
> 
I guess they do, but I agree with Peter that such extensive redefinitions are
not really suited to an erratum.

> If RFC 7366 means to say that the MAC field is no longer part of the  
> record_data field (and not counted in the length field), then that is  
> going to cause trouble, because processors that are not aware of the  
> extension will then read the first bytes of the MAX as the ContentType  
> field and the version field, probably resulting in a connection closure.
> 
I think it's pretty clear for everyone RFC 7366 never intended to say that.

> The second issue that seems to have developed is which value to use as the  
> length part of the MAC calculation.
> 
Which was actually the only issue causing interop failures.

> Adapted to my rewritten structure the question is if the MAC (for block  
> cipher encryption) is calculated as
> 
>     MAC(MAC_write_key, seq_num +
>         TLS_Record.type +
>         TLS_Record.version +
>         TLS_Record.record_data.length +
>         IV +
>         ENC(content + padding + padding_length));
> 
> or
> 
>     MAC(MAC_write_key, seq_num +
>         TLS_Record.type +
>         TLS_Record.version +
>         TLS_EtM_Block_Encrypted_data.fragment.length +
>         IV +
>         ENC(content + padding + padding_length));
> 
If I follow correctly your new notation, the second one is not what the RFC
meant: the length should be the length of
TLS_EtM_Block_Encrypted_data.encrypted, not fragment (ie including the padding).

> As written, as I understand it, and assuming compatibility with ignorant  
> intermediates, RFC 7366 specifies the first formula.
> 
That's what I first thought too, but it turns out it was not the intention of
the RFC and most importantly, it's not what the implementations deployed so far do.

Unless you're aware of an implementation that is deployed and incompatible with
the eid server (which I seriously hope not), there's not point discussing which
choice makes the most sense: it's to match the deployed implementation(s).

> My suggestion for an answer to the first question is that we have to  
> maintain backwards compatibility with ignorant intermediates, which means  
> that TLSCipherText.length MUST count the MAC bytes, not just the fragment  
> (including padding).
> 
I think we all agree, and always agreed, on this one.

> Regarding the second question, IMO it does not really matter much which  
> value is used. However, it is probably easier to use the length of the  
> encrypted block, rather than TLSCipherText.length, since  
> TLSCipherText.length have to be calculated instead of derived from the  
> MACed encrypted block.
> 
Yes. (Just nitpicking: when doing authenticated decryption, it's the other way:
the length of the encrypted block needs to be computed, while the length of the
record is readily available, so the situation is quite symmetric actually.)

> Given the current confusion we need at least an errata that accurately  
> specifies which values include which data.
> 
I'm under the impression that Peter's paragraph in [1] suits this goal.

[1] http://mailarchive.ietf.org/arch/msg/tls/3NOHlIUShLo9AAzXx4t2u7QV8Sw

Do you agree? I reproduce the specific paragraph here for convenience's sake:

  Note that the value of the 'uint16 length' field in the TLSCiphertext record
  differs from the length value used in the MAC calculation, since the
  TLSCiphertext record contains both the ciphtertext and the MAC, while the
  MAC calculation is performed only over the ciphertext.  So the length value
  encoded in the TLSCiphertext record is 'length' while the length value used
  in the MAC calculation is 'length - SecurityParameters.mac_length'.

> However, given that such changes may modify the meaning more than a normal  
> errata do, perhaps a better idea is a new RFC that either replace RFC 7366  
> or one that specifies a new extension ID and deprecate the old one, so  
> that we reduce the potential for confusion among implementations?

Unless we have reasons to think there already are mutually incompatible
implementations in the wild, I don't think it's worth the trouble allocating a
new extension, which would delay adoption of the feature, while we want it
available and actually usable sooner rather than later.

> After  
> all, some implementers might miss the existence of the errata, and in this  
> case the errata would be important for the proper understanding of the  
> specification.
> 
I'd expect any half-serious implementer to test interop with at least one other
implementation before releasing. Unfortunately, experience proves people are not
always serious about it. I honestly don't know.

Manuel.