Subject: [TLS] (strict) decoding of legacy_record_version?

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 7 Nov 2016 17:50:46 -0600

(was Re: [TLS] PR#625: Change alert requirements)

Digging up an old sub-thread...

On 09/20/2016 08:03 AM, Eric Rescorla wrote:
>
>     in Record Layer there's the following text:
>
>         legacy_record_version : This value MUST be set to { 3, 1 } for all
>         records. This field is deprecated and MUST be ignored for all
>     purposes.
>
>     in Record Layer Protection there's the following text:
>
>         legacy_record_version : The legacy_record_version field is
>     identical to
>         TLSPlaintext.legacy_record_version and is always { 3, 1 }.
>     Note that the
>         handshake protocol including the ClientHello and ServerHello
>     messages
>         authenticates the protocol version, so this value is redundant.
>
>     which doesn't say if the version can be ignored completely
>     (skipped while
>     parsing) or if it should be verified.
>
>
> These are different fields.
>

There's still the question of whether the receiver should enforce 0x0301
in either/both cases.
OpenSSL is implementing and seems to be reading the spec that it MUST be
ignored (even though I guess strictly speaking that MUST only applies
before record protection is engaged); if I'm doing my code survey
correctly, Mint and NSS always enforce, and Boring only checks the first
octet.

Is there a reason to not do strict enforcement?

-Ben

---

From: David Benjamin <[email-masked]>
Date: Tue, 08 Nov 2016 00:04:09 +0000

On Mon, Nov 7, 2016 at 6:50 PM Benjamin Kaduk <[email-masked]> wrote:

    legacy_record_version : This value MUST be set to { 3, 1 } for all
    records. This field is deprecated and MUST be ignored for all purposes.

    legacy_record_version : The legacy_record_version field is identical to
    TLSPlaintext.legacy_record_version and is always { 3, 1 }. Note that the
    handshake protocol including the ClientHello and ServerHello messages
    authenticates the protocol version, so this value is redundant.

which doesn't say if the version can be ignored completely (skipped while
parsing) or if it should be verified.

There's still the question of whether the receiver should enforce 0x0301 in
either/both cases.

Before you know the version, it is not safe to enforce. Prior versions of
TLS did not pin down the value. (BoringSSL checks the first octet because
OpenSSL did so, and it's vaguely nice to sanity-check on the first few
bytes of data in the protocol.)

Immediately after you know the version, enforcement is also problematic.
Alerts sent in response to a ClientHello or ServerHello have somewhat
ambiguous versions. One side may have decided the version is set while the
other has not. Prior to 1.3, the record version needs to be set to the
protocol version (or you break pre-1.3 enforcing implementations). But this
means a receiver which enforces will fail to parse the alert. Not the end
of the world, but weird.

Once you've gotten as far as to switch to TLSCiphertext, I don't see a
reason not to enforce. Keying on versions is problematic (which is why we
avoided a transition to enforcement), but keying on whether the record is
encrypted seems fine. I think it just didn't occur to us to base it on
that. :-)

David

---

From: Eric Rescorla <[email-masked]>
Date: Mon, 7 Nov 2016 18:17:27 -0800

> encrypted seems fine. I think it just didn't occur to us to base it on

As you say, this is what NSS does. There's no legit reason for a TLS 1.3
encrypted record to have the wrong version number here, so I suggest
changing the text.

-Ekr

---

From: Brian Smith <[email-masked]>
Date: Mon, 7 Nov 2016 17:01:18 -1000

> encrypted seems fine. I think it just didn't occur to us to base it on

Since this field isn't included in the additional_data of the AEAD in TLS
1.3 any more, it isn't authenticated. That means an active MitM can use
this to transport up to 2 bytes of information hop-to-hop if the receiver
doesn't check it. That seems like a good reason to check it, and also to
check TLSCiphertext.opaque_type is application_data. Assuming this is the
reason, the reasoning should be explicitly called out because it is
non-obvious.

If that isn't a reason to do the check, then I don't think there's any
reason to mandate that implementations do it.

Cheers,
Brian
-- 
https://briansmith.org/

---

From: Martin Thomson <[email-masked]>
Date: Tue, 8 Nov 2016 14:53:11 +1100

On 8 November 2016 at 14:01, Brian Smith <[email-masked]> wrote:
> 1.3 any more, it isn't authenticated. That means an active MitM can use this
> to transport up to 2 bytes of information hop-to-hop if the receiver doesn't
> check it. That seems like a good reason to check it, and also to check
> TLSCiphertext.opaque_type is application_data. Assuming this is the reason,
> the reasoning should be explicitly called out because it is non-obvious.

I don't think that's the primary reason.  A MitM could use TCP headers
to carry other bits if they wanted.

The main reason I can think of is ecosystem health.  If you permit
junk, then you get junk.  Then you can't use the field any more
because it contains junk.

---

From: Brian Smith <[email-masked]>
Date: Tue, 8 Nov 2016 08:59:15 -1000

Martin Thomson <[email-masked]> wrote:

> this

Smuggling at the application layer would be more reliable for them, though.
I'd say the goal of the check isn't to prevent all possible smuggling;
rather it's part of a solution to it. Anyway, this is the only convincing
rationale I see for requiring ("MUST") vs allowing ("MAY") the checking.

> because it contains junk.

This isn't a pervasively shared goal, though. It's good to let the browsers
police things if they want, but I think a lot of implementations would
prefer to avoid doing work that isn't necessary for interop or security.

Cheers,
Brian
-- 
https://briansmith.org/

---

From: Martin Thomson <[email-masked]>
Date: Wed, 9 Nov 2016 11:25:37 +1100

> prefer to avoid doing work that isn't necessary for interop or security.

If you permit someone to enforce it, then that is sufficient.  I don't
think that we should ever force someone to enforce these sorts of
things (as you say, sometimes strict enforcement isn't cheap or even
desirable).

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 10 Nov 2016 10:22:17 -0600

> things (as you say, sometimes strict enforcement isn't cheap or even

Agreed.  We should probably change the text a bit, though, as right now
readers can get two different readings depending on whether they go for
a strict decode_error (or illegal_parameter?) since the struct doesn't
match the definition, or follow the "MUST be ignored for all purposes".

-Ben

---

From: "Andreas Walz" <[email-masked]>
Date: Wed, 23 Nov 2016 12:39:35 +0100

Dear all,

bringing up this thread again ....

In the course
 of studying the way TLS implementations treat the "version" (or 
"legacy_record_version") field in the record header, we were wondering 
(please excuse if we missed some arguments here from past discussions):

(1)
 What is an implementation (in particular when receiving the first bytes
 over a new connection) supposed to do if the record's version field 
signals a protocol version the implementation does not support? I 
understand that, at this stage, enforcing a specific value (e.g. 0x0301 
according to the TLSv1.3 draft) is detrimental to interoperability. 
However, if that field bears any meaning (in either TLSv1.3 or previous 
versions), what is it? I would expect this field is supposed to allow 
signaling a potentially non-backward compatible record format 
(inauspiciously interfering with a receiver disregarding the record 
version). Provided this field isn't treated as an enum, what about 
checking/enforcing at least the major version as BoringSSL does (as far 
as I know)? In any case, I would propose to be very clear about this in 
the text (my sense was that there is some work in progress, but I 
couldn't find anything). In implementations (<TLSv1.3), we found all 
kinds of interpretations.

(2) What is an implementation (up to 
TLSv1.2, as the TLSv1.3 spec is rather clear about that) supposed to use
 for the record's protocol version field before a version has been 
agreed upon (e.g. when sending an alert after receiving an unparsable 
ClientHello)? My best guess would be to set it to the lowest (TLS) 
protocol version that uses the same record format (probably 0x0301). 
However, we observe several servers which, in such cases, answer with an
 alert with weird record protocol version values, e.g. 0x0000.]

Thanks and Cheers,
Andi

___________________________________

Andreas Walz
Research Engineer
Institute of reliable Embedded Systems and Communication Electronics (ivESK)
Offenburg University of Applied Sciences, 77652 Offenburg, Germany

>>> Benjamin Kaduk <[email-masked]> 11/10/16 5:22 PM >>>
          
     Agreed.  We should probably change the text a bit, though, as right     now readers can get two different readings depending on whether they     go for a strict decode_error (or illegal_parameter?) since the     struct doesn't match the definition, or follow the "MUST be ignored     for all purposes".
     
     -Ben

---

From: Eric Rescorla <[email-masked]>
Date: Wed, 23 Nov 2016 05:17:52 -0800

On Wed, Nov 23, 2016 at 3:39 AM, Andreas Walz <[email-masked]>
wrote:

> In the course of studying the way TLS implementations treat the "version"
> (or "legacy_record_version") field in the record header, we were wondering
> (1) What is an implementation (in particular when receiving the first
> bytes over a new connection) supposed to do if the record's version field
> checking/enforcing at least the major version as BoringSSL does (as far as
> I know)? In any case, I would propose to be very clear about this in the
> text (my sense was that there is some work in progress, but I couldn't find
> anything). In implementations (<TLSv1.3), we found all kinds of
> interpretations.
>

In general, it should ignore it. It's going to become increasingly common
to have this be a version you don't support given the recommendation to use
0301 and the ongoing deprecation of TLS 1.0. I think it would be fine to
sanity check the major version, but I'm not sure what would be gained by
requiring this.

> (2) What is an implementation (up to TLSv1.2, as the TLSv1.3 spec is
> rather clear about that) supposed to use for the record's protocol version
> field before a version has been agreed upon (e.g. when sending an alert
> after receiving an unparsable ClientHello)? My best guess would be to set
> it to the lowest (TLS) protocol version that uses the same record format
> (probably 0x0301). However, we observe several servers which, in such
> cases, answer with an alert with weird record protocol version values, e.g.
> 0x0000.]
>

Yes, this seems like a reasonable procedure. Not sure how to tell TLS 1.2
impls what to do at this point, though.

-Ekr

> On 9 November 2016 at 05:59, Brian Smith <[email-masked]> <[email-masked]> wrote:
>
> readers can get two different readings depending on whether they go for a
> strict decode_error (or illegal_parameter?) since the struct doesn't match
> the definition, or follow the "MUST be ignored for all purposes".
>
> -Ben
>
> _______________________________________________
> TLS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tls
>
>

---

From: "Andreas Walz" <[email-masked]>
Date: Thu, 24 Nov 2016 08:38:32 +0100

>>> Eric Rescorla <[email-masked]> 11/23/16 2:18 PM >>>
> In general, it should ignore it. It's going to become increasingly common to 
> have this be a version you don't support given the recommendation to use
> 0301 and the ongoing deprecation of TLS 1.0. I think it would be fine to sanity
> check the major version, but I'm not sure what would be gained by requiring this. 

The one benefit of checking at least the record's major version I see is that one
preserves means for the future to signal a record format incompatible with the
current one. Otherwise, this field is given away for arbitrary and non-standardized
(ab)use ...

Thanks and Cheers,
Andi

Andreas Walz
Research Engineer

     
     -Ben