Subject: [TLS] predictability of inputs in ESNI

From: Rob Sayre <[email-masked]>
Date: Fri, 1 Nov 2019 14:27:50 -0700

Hi,

I am not sure how important these findings are, but I've noticed three
instances of unnecessarily predictable inputs in ESNI:

1) Trailing padding after domain names are zeros.
2) The checksum calculation seems to start with predictable version bytes
in draft -04, and in shipping implementations
3) In practice, NSS inserts 8 bytes of zeros at the beginning of its AAD
input (<https://github.com/tlswg/draft-ietf-tls-esni/issues/190>)

It seems like these values should all be allowed to be opaque, and I am not
sure why NSS is prepending zeros to its AAD (although I asked in the github
issue).

thanks,
Rob

---

From: Eric Rescorla <[email-masked]>
Date: Fri, 1 Nov 2019 15:09:06 -0700

> in draft -04, and in shipping implementations

I'm not aware of any cryptographic reason why these would be a problem.
It's reasonably common practice to have partially fixed inputs to the TLS
cryptographic functions (see, for instance, HMAC labels and the trailing
padding 0s in the record protection). In general, if this sort of thing
turns out to be a problem for some algorithm, we consider it to be a
problem with the algorithm.

> input (<https://github.com/tlswg/draft-ietf-tls-esni/issues/190>)

I believe you're misreading the code. For historical reasons, the internal
NSS TLS AEAD API prefixes the AAD argument with the sequence number. When
we repurposed this code for ESNI, we just passed in a 0 sequence number.
That value is stripped off and is not part of the AAD computation.

> sure why NSS is prepending zeros to its AAD (although I asked in the github

As I said, it's not.

-Ekr

---

From: Rob Sayre <[email-masked]>
Date: Fri, 1 Nov 2019 15:27:06 -0700

> problem with the algorithm.

I see. But is there any reason to make these inputs predictably zero by
spec?

> That value is stripped off and is not part of the AAD computation.

I see, so you're saying that NSS adds 8 bytes to the AAD input, and then
strips it off at some later point? I wouldn't exactly call that misreading.
Maybe something to fix?

thanks,
Rob

---

From: Eric Rescorla <[email-masked]>
Date: Fri, 1 Nov 2019 15:39:22 -0700

> I see. But is there any reason to make these inputs predictably zero by

Absent some reason not to, this seems like a reasonable design choice. At
minimum, it has the advantage that it's easy to detect some classes of
implementation errors.

>> I believe you're misreading the code. For historical reasons, the
>> internal NSS TLS AEAD API prefixes the AAD argument with the sequence
>> number. When we repurposed this code for ESNI, we just passed in a 0
>> sequence number. That value is stripped off and is not part of the AAD
>> computation.
>>
>
> strips it off at some later point?
>

No. I'm saying that the function argument is called |additionalData| but
that's not what's used as AAD in the actual cryptographic function.

I wouldn't exactly call that misreading.

This seems like a definitional question that is not worth debating.

Maybe something to fix?

This is not on topic for this mailing list. This internal interface is
quite old and when we wrote the TLS 1.3 code, we considered changing it and
decided it wasn't worth it. If you'd like to submit a CL to NSS which
improves this, we'll take a look.

-Ekr

---

From: Rob Sayre <[email-masked]>
Date: Fri, 1 Nov 2019 15:54:01 -0700

> minimum, it has the advantage that it's easy to detect some classes of

I don't think the current spec is a good idea. It mandates padding with
zeros, and lets servers dictate a length. I think the client should dictate
the length, at least.

> This seems like a definitional question that is not worth debating.

Hey now, you're the one that wrote "misreading" initially. If "additional
data" is not what's used as "additional authenticated data" in the end,
fine.

>
> Maybe something to fix?
>>
>
> This is not on topic for this mailing list.
>

I think it is, since no one could read any ESNI draft and get the DNS
record's checksum right, at the very least. So, there are some issues where
it can be hard to tell if the spec is wrong, my implementation is wrong, or
another implementation is wrong.

thanks,
Rob

---

From: Eric Rescorla <[email-masked]>
Date: Fri, 1 Nov 2019 16:03:32 -0700

> zeros, and lets servers dictate a length. I think the client should dictate

This is a separate issue from the structure of the padding.

> data" is not what's used as "additional authenticated data" in the end,

 NSS does not include the leading 0s as part of the computation of the AAD
that appears on the wire. To the extent to which you may have believed
otherwise based on your reading of the code, that is not a correct
understanding.

-Ekr

---

From: Rob Sayre <[email-masked]>
Date: Fri, 1 Nov 2019 16:18:39 -0700

On Fri, Nov 1, 2019 at 4:04 PM Eric Rescorla <[email-masked]> wrote:

>>> Absent some reason not to, this seems like a reasonable design choice.
>>> At minimum, it has the advantage that it's easy to detect some classes of
>>> implementation errors.
>>>
>>

I think the length of the padding might be fairly considered part of the
"structure of the padding".

> otherwise based on your reading of the code, that is not a correct

I see, so I should chase down every last obfuscated padding byte? I can't
actually read a draft to implement this spec.

I also do not think this issue should have been unilaterally closed:
https://github.com/tlswg/draft-ietf-tls-esni/issues/190

Maybe it's time for some new TLS editors.

thanks,
Rob

---

From: "Christopher Wood" <[email-masked]>
Date: Fri, 01 Nov 2019 16:29:11 -0700

On Fri, Nov 1, 2019, at 4:18 PM, Rob Sayre wrote:
> https://github..com/tlswg/draft-ietf-tls-esni/issues/190 
> <https://github.com/tlswg/draft-ietf-tls-esni/issues/190>
> 

As the issue was filed based on a misunderstanding of the NSS implementation, closing this issue was appropriate. 

Best,
Chris (chair hat on)