Re: [TLS] Draft 18 review : Message splitting and interleaving

[Eric Rescorla <[e-mail]>]

On Tue, Nov 22, 2016 at 2:18 PM, David Benjamin <[e-mail]>
wrote:

> On Tue, Nov 22, 2016 at 2:05 PM Olivier Levillain <
> [e-mail]> wrote:
>
>> Hi list,
>>
>> I am sorry for the very late answer concerning draft 18, but we
>> (ANSSI) have several remarks after proof-reading the current
>> specification.
>>
>> We are sorry for the multiple long messages.
>>
>> If the WG is interested by some of our concerns/proposals, we would be
>> glad to propose some PRs.
>>
>>
>> = Message splitting and interleaving =
>>
>> How to split and interleave subprotocols in TLS has not been clearly
>> specified in the past, and it would be useful to be crystal clear on
>> this point.
>>
>> In the specification, the subject is first presented in 4.5 (P.61):
>>    Handshake messages sent after the handshake MUST NOT be interleaved
>>    with other record types.  That is, if a message is split over two or
>>    more handshake records, there MUST NOT be any other records between
>>    them.
>> But I am wondering why this text only concern "messages after the
>> handshake".  It should always be the case!
>>
>> It is next present in section 4.5.3 (P.64):
>>    Handshake messages MUST NOT span key changes.  Because the
>>    ServerHello, Finished, and KeyUpdate messages signal a key change,
>>    upon receiving these messages a receiver MUST verify that the end of
>>    these messages aligns with a record boundary; if not, then it MUST
>>    terminate the connection with an "unexpected_message" alert.
>>
>> And then in section 5 (mostly P.64 and P.65) plus a repetition P.69.
>>
>>
>> It is worth noting that this specific point was (at least partially)
>> the origin of several security issues:
>>  - the alert attack (https://www.mitls.org/pages/attacks/Alert);
>>  - Heartbleed (where OpenSSL answered with a fragmented record whereas
>>    it was not the spirit of the spec);
>>  - the OpenSSL Downgrade attack in 2014.
>>
>> Some of this is covered in the current specification, but it might be
>> worth being even stricter:
>>  - regarding splitting/merging, they should be forbidden by default;
>>  - the default would apply to alerts (currently, the spec states that
>>    alerts MUST not be split, but they should not be merged either
>>    for simplicity's sake and since it is useless);
>>
>
> +1. We (BoringSSL) and I believe NSS already forbid this for alerts at all
> versions.
>
> This rule is actually implied by TLS 1.3 already, because we've gotten rid
> of warning alerts. All alerts terminate the connection, except for
> end_of_early_data, and end_of_early_data immediately signals a key change.
> So it is not legal to send two alerts in the same epoch, much less in the
> same record. (Being explicit about this is good, of course.)
>

This seems fine. I would take a PR for this.


 - incidentally, the default behaviour would apply to Heartbeat, as
>>    was the intent of the specification;
>>  - ApplicationData should be considered as a stream with possibly
>>    0-length records
>>  - Handshake messages should either come as a sequence of multiple
>>    *entire* messages, or as a fraction of *one* message.  That is,
>>    the number of HS messages inside one record should either be a
>>    round number or strictly less than 1.
>>
>
> What simplifications were you expecting out of this? It seems to me this
> would be a nuisance to both enforce as a receiver and honor as a sender.
>
> Our implementation doesn't try to pack handshake messages into records,
> but I believe NSS does. NSS folks should confirm, but I expect such
> implementations just buffer the messages up and flush when the buffer
> exceeds the records size. That means all kinds of splits are plausible:
>
>   [EncryptedExtensions Certifi]
>   [cateRequest Certificate Cer]
>   [tificateVerify Finished]
>

Yeah, that's how this works in NSS.

I'm not seeing a real benefit in prohibiting this behavior.

-Ekr

I share your dislike of the way handshake messages and record boundaries
> interact (or, rather, don't), but I'm not sure how this rule helps.
>
>
>> These constraints can be expressed in 5.1 and cover most of the
>> problematic cases.  They are easy to check and to enforce.
>>
>>
>> However, the problem of the OpenSSL Downgrade flaw (where the first
>> record containing the beginning of the ClientHello was cut before the
>> ClientHello.version field) will still stand, but there is no simple
>> way to fix this, since version negotiation can now happen anywhere in
>> the ClientHello extensions, so it might be impossible to enforce that
>> the client versions are sent in the first record.
>>
>> I might volunteer some text if there is an interest of the WG.
>>
>>
>> Olivier Levillain
>>
>> _______________________________________________
>> TLS mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/tls
>>
>
> _______________________________________________
> TLS mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/tls
>
>