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

Define Context and Length inputs to HKDF-Expand-Label #4571

Merged
merged 3 commits into from Jan 7, 2021

Conversation

martinthomson
Copy link
Member

This was a pretty serious omission, even if it was obvious enough that no one ever noticed when implementing it.

Closes #4489.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -tls labels Jan 6, 2021
Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. I have one concern.

to HKDF-Expand-Label along with these labels is determined by the size of keys
in the AEAD or header protection algorithm. The Length provided with "quic iv"
is the length of the AEAD nonce, specifically N_MAX, though the value of N_MIN
is the same for the AEAD algorithms that can be used; see {{!AEAD}}.
Copy link
Member

@kazuho kazuho Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had assumed that we are using the nonce length specified in RFC 8446 section 5.3. As the proposed text mentions, the results for the cipher suites being defined are the same, but the definition in RFC 8446 is that the nonce length is max(8, N_MIN). I do not think there is a reason to deviate from TLS 1.3 and think that we should delegate the definition of nonce length to RFC 8446.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this being independent of RFC 8446. Though I've changed to N_MIN (no sense in being different). We can deal with the problem of N_MIN < 8 if that ever happens (such an AEAD might not be suitable for use with QUIC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes. It's better, but IMO max(8, N_MIN) is preferable than N_MIN.

The reasons are:

  • I'm not sure if we need to allow N_MIN < 8 (should that happen), considering the fact that our packet numbers are 62 bits.
  • From implementation perspective, it would be better if we can be clear that QUIC would not change the size of the nonce of cipher suites that are defined by TLS 1.3.

Though I can concede with current state of PR, because QUIC cannot silently add support for new cipher suites as they get defined for TLS. We need to define header protection for each of them, so we can delegate the discussion of AEAD nonce size.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that was my rationale: no one can sneak in an AEAD with N_MIN < 8. At that point we can deal with it. One way we might decide to deal with it is to limit the maximum packet number to 256^N_MIN, assuming we really want that AEAD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinthomson

no one can sneak in an AEAD with N_MIN < 8

I think the correct argument is "no one can sneak in an AEAD with N_MAX < 8"? RFC 8446 states that (which I think is correct), and to support cipher suites that are N_MIN < 8 && N_MAX >= 8, it says that the AEAD nonce size is max(N_MIN, 8).

I think it's perfectly reasonable for us to follow that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's good enough for me. I don't like it, but the "can't sneak one in" still applies.

@martinthomson martinthomson merged commit 9d40fe6 into master Jan 7, 2021
@martinthomson martinthomson deleted the hkdf-expand-label-context branch January 7, 2021 07:21
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ben Kaduk's TLS Comment 13
2 participants