Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS-31: Usage of HKDF is vague #4197

Closed
gloinul opened this issue Oct 13, 2020 · 3 comments · Fixed by #4201
Closed

TLS-31: Usage of HKDF is vague #4197

gloinul opened this issue Oct 13, 2020 · 3 comments · Fixed by #4201
Labels
-tls editorial An issue that does not affect the design of the protocol; does not require consensus. ietf-lc An issue that was raised during IETF Last Call.

Comments

@gloinul
Copy link
Contributor

gloinul commented Oct 13, 2020

Section 5.2:

The secret used by clients to construct Initial packets uses the PRK and the label "client in" as input to the HKDF-Expand-Label function to produce a 32 byte secret; packets constructed by the server use the same process with the label "server in". The hash function for HKDF when deriving initial secrets and keys is SHA-256 [SHA].

Looking at RFC 5689 the HKDF-Expand function defines what is referenced as label as "info" input. It is a string. It is unclear that this is an ASCII string, but based on what is written it looks like something where each character is an octet is assumed, thus no UTF-16 or UTF-32 strings would work as intended.

If I am correct I think the draft can be improved by being explicit like in TLS that the label used as input into the functions as ASCII. So adding something like this sentence from RFC8446 is likely appropriate:

The labels specified in this document are all ASCII strings and do not
include a trailing NUL byte.

@larseggert larseggert added -tls ietf-lc An issue that was raised during IETF Last Call. labels Oct 13, 2020
@gloinul
Copy link
Contributor Author

gloinul commented Oct 13, 2020

Actually I think I understand what the issue is: Lets look at these three paragraphs in Section 5.2:

This secret is determined by using HKDF-Extract (see Section 2.2 of [HKDF]) with a salt of 0xafbfec289993d24c9e9786f19c6111e04390a899 and a IKM of the Destination Connection ID field. This produces an intermediate pseudorandom key (PRK) that is used to derive two separate secrets for sending and receiving.

The secret used by clients to construct Initial packets uses the PRK and the label "client in" as input to the HKDF-Expand-Label function to produce a 32 byte secret; packets constructed by the server use the same process with the label "server in". The hash function for HKDF when deriving initial secrets and keys is SHA-256 [SHA].

The HKDF-Expand-Label function defined in TLS 1.3 MUST be used for Initial packets even where the TLS versions offered do not include TLS 1.3.

I got the impression that the HKDF-Expand function was the one from RFC5689 and not the one from TLS1.3. So I think what is needed to clarify this is that the second paragraph needs to explicitly provide the HKDF-Expand-label function with section and document reference. I still think it should be made clear that the labels are ASCII, but that is clear in TLS 1.3 compared to RFC5689.

martinthomson added a commit that referenced this issue Oct 13, 2020
This was, I think clear, but the use of HKDF-Expand-Label and the
encoding of labels into octets could be even more explicit.

Closes #4197.
@larseggert
Copy link
Member

Based on #4201, labeling this as "editorial"

@larseggert larseggert added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Oct 15, 2020
@gloinul
Copy link
Contributor Author

gloinul commented Oct 15, 2020

Changes looks good, resolve the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls editorial An issue that does not affect the design of the protocol; does not require consensus. ietf-lc An issue that was raised during IETF Last Call.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants