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

[David Benjamin <[e-mail]>]

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.)


>  - 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]

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
>