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

From: Olivier Levillain <[email-masked]>
Date: Tue, 22 Nov 2016 20:08:26 +0100

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

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

---

From: David Benjamin <[email-masked]>
Date: Tue, 22 Nov 2016 22:18:27 +0000

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

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

> _______________________________________________
> TLS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tls
>

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 22 Nov 2016 15:24:04 -0800

> same record. (Being explicit about this is good, of course.)

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

> Our implementation doesn't try to pack handshake messages into records,
> but I believe NSS does. NSS folks should confirm, but I expect such

Yeah, that's how this works in NSS.

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

-Ekr

---

From: Martin Thomson <[email-masked]>
Date: Wed, 23 Nov 2016 11:36:42 +1100

> Yeah, that's how this works in NSS.

To be clear, NSS buffers an entire flight of messages and then sends
them.  It might fragment things between TCP segments as a result, but
usually fits everything in a single record (with some exceptions,
thanks to CertificateRequest being bloated, foor example).  (In DTLS,
it's more complicated because we have MTU detection, but the same
basic principle applies.)

Like others, I would find stricter rules around record splits very
hard to enforce, and for not much gain.

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 22 Nov 2016 16:45:09 -0800

> basic principle applies.)

Yes, this is what I meant to say. Basically, it tries to cram as much as it
can into
one record and the answer to "as much as it can" is "it depends"

-Ekr

---

From: Olivier Levillain <[email-masked]>
Date: Wed, 23 Nov 2016 10:08:34 +0100

>>> handshake".  It should always be the case!

I don't know what is your take on this first point, but I think I will
propose something in the PR.

I will try to add/move some text so the information is all present in
section 5.

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

Will do.

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

The expected benefit was that it was a way to enforce the rule from
section 4.6 (P.64), stating that a Handshake message should not span key
changes. That is why the receiver already had to do some work, and my
proposal was to restrict it even more. Yet I see your point that from
the sender's point of view, this is more complex.

After re-reading the 4.6 part, I find it sufficient. Sorry for the noise
about this.

olivier

---

From: Olivier Levillain <[email-masked]>
Date: Wed, 30 Nov 2016 10:34:00 +0100

Hi list,

I just proposed a new PR (#798) concerning the text in 5.1 (Record layer), following the discussion here (and not including the extra Handshake constraints).

Best regards,
olivier