hkdf: increase MAX_HKDF_INFO_LEN

[cpu]

Hi there,

I've been working on encrypted client hello (ECH) support in Rustls. As part of that we're using aws-lc-rs for the HKDF operations the draft calls for.

In particular, ECH offer confirmation is determined by computing an 8 byte confirmation value:

   accept_confirmation = HKDF-Expand-Label(
      HKDF-Extract(0, ClientHelloInner.random),
      "ech accept confirmation",
      transcript_ech_conf,
      8)

Similarly, in the hello-retry request confirmation case we compute:

   hrr_accept_confirmation = HKDF-Expand-Label(
      HKDF-Extract(0, ClientHelloInner1.random),
      "hrr ech accept confirmation",
      transcript_hrr_ech_conf,
      8)

The HKDF-Expand-Label and HKDF-Extract algorithms are unmodified from RFC 8446.

For a handshake using SHA-384 for the digest algorithm when we confirm ECH acceptance we perform an HKDF expand operation with the info:

    let label = b"ech accept confirmation"; // or b"hrr ech accept confirmation";
    let n = 8;
    const LABEL_PREFIX: &[u8] = b"tls13 ";

    let output_len = u16::to_be_bytes(n as u16);
    let label_len = u8::to_be_bytes((LABEL_PREFIX.len() + label.len()) as u8);
    let context_len = u8::to_be_bytes(context.len() as u8);

    let info = &[
        &output_len[..],
        &label_len[..],
        LABEL_PREFIX,
        label,
        &context_len[..],
        context,
    ];

This works out to be:

  2 bytes for the output len
  1 byte for the label len
  6 bytes for the label prefix
 23 bytes for the ECH confirmation label (or 27 bytes for HRR ECH confirmation label)
  1 byte for the context len
 48 bytes for the context
------------------------------------------
= 81 bytes total

This exceeds the existing MAX_HKDF_INFO_LEN limit of 80 bytes, and so produces an Unspecified error. Equivalent inputs do not cause an error with *ring*, where there doesn't seem to be a limit imposed on the info parameter for HKDF.

The only point of variability in the input is the context length, which in the ECH case is always a transcript digest and so the length is dependent on the hash algorithm in use. If we assume HRR confirmation (for the longer 27 byte label), and SHA512 for the digest, then the context could be as large as 64 bytes, so I think the largest info that would be required to be supported for ECH confirmation is 101 bytes. (2 + 1 + 6 + 27 + 1 + 64 = 101).

This PR increase the limit to 102 bytes (it seemed a nicer value than 101), and experimentally all of my ECH confirmation tests using aws-lc-rs for the HKDF operations pass with this limit where previously they resulted in a panic from exceeding the info limit.

[cpu]

The only point of variability in the input is the context length, which in the ECH case is always a transcript digest and so the length is dependent on the hash algorithm in use. If we assume SHA512, then the context could be as large as 64 bytes, so I think the largest info that would be required to be supported for ECH confirmation is 97 bytes. (2 + 1 + 6 + 23 + 1 + 64 = 97).

Reviewing this for myself I realized I forgot to account for one more piece of variability: the label could be either b"ech accept confirmation"; (23 bytes) for the normal confirmation case, or b"hrr ech accept confirmation"; (27 bytes) for the HRR case.

I think that means the upper limit is in fact 101 bytes:

  2 bytes for the output len
  1 byte for the label len
  6 bytes for the label prefix
  27 bytes for the ECH confirmation label (note: changed from 23)
  1 byte for the context len
 64 bytes for the context
------------------------------------------
= 101 bytes total

So I think I should set the limit in this branch to 102 bytes instead of 100. I will push a small adjustment.

[cpu]

I think that means the upper limit is in fact 101 bytes:

Limit, commit message & PR description all updated accordingly. Sorry about the last minute indecision :-)

[justsmth]

Thanks for the PR!

This might be a value we should have in a Box<[u8]>. Let us know if the larger size impacts performance, we can do some minor refactoring if needed.

[cpu]

This might be a value we should have in a Box<[u8]>. Let us know if the larger size impacts performance, we can do some minor refactoring if needed.

Good thought; I will see about running our benchmark suite to see if this unexpectedly regressed performance.

@justsmth What are your thoughts on whether this branch should update the Prk::expand docs to mention the limit? Presently it says:

error::Unspecified if (and only if) len is too large.

but I think this experience shows it can also return error::Unspecified if info is too large.

[justsmth]

What are your thoughts on whether this branch should update the Prk::expand docs to mention the limit?

Yeah, the documentation should be specific about such limitations -- "too large" is needlessly vague (and applies to more than just len). Were we to put this [u8] into a Box in a subsequent PR, we could then remove the limitation (or increase it further?) and update the documentation accordingly. Thanks!

[justsmth]

I updated the docs to provide more specific guidance about the errors of Prk::expand. The way I embedded the value of the non-pub const into the documentation is really clunky, I would love to find a better way to do that!

[cpu]

Thanks for the doc update, LGTM.

[cpu]

Good thought; I will see about running our benchmark suite to see if this unexpectedly regressed performance.

Within our benchmarking framework the change doesn't seem to cause any significant impact (results here).

[cpu]

@justsmth @skmcgrail 😅 at the risk of hijacking a closed issue I wanted to get your thoughts on whether there might be a broader solution to this problem. Having a fixed maximum for the HKDF info length seems like a sensible optimization for the TLS (and since c358484, ECH) use-case but a one-size-fits-all limit makes general application tricky.

As a concrete example while working on rustls/rustls#1963 I found I had to increase the limit to 300 bytes in order to support all of the base mode RFC 9180 test vectors (the largest suite + input produced 298 byte HKDF info input while deriving a key schedule key). Trying to upstream that change feels a bit like whack-a-mole. The next interesting use-case might need an even larger maximum, or we might start to see perf impact from the bloat.

I'm trying to intuit the reason for the divergence in implementation between aws-lc-rs and *ring* in this area. It seems like *ring* maintains the input info slice-of-slices in the Okm and feeds each piece to a digest context at the time of filling, whereas aws-lc-rs is flattening the info slices into one contiguous slice when building the Okm, so that later it's appropriate to pass through the FFI boundary to the aws-lc HKDF. Does that interpretation track? Would a smallvec-like solution work here to stack-allocate in the common TLS case but support heap allocation for folks pushing large info past the limit for the stack version?

[justsmth]

@cpu -- Thanks for your feedback! I agree, and your assessment is 100% accurate.

I've posted PR #424 that will remove our arbitrary limit on the HKDF info length. Let us know if this adequately addresses your concerns. Feel free to comment. Thanks again!