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

dtls.c: Handle DTLS1.3 ClientHello when calculating cookie #223

Closed
wants to merge 1 commit into from

Conversation

mrdeep1
Copy link
Contributor

@mrdeep1 mrdeep1 commented Feb 6, 2024

Do not calculate the cookie using the Extensions as these are different between DTLS1.2 and DTLS1.3

https://datatracker.ietf.org/doc/html/rfc6347#section-4.2.1

When responding to a HelloVerifyRequest, the client MUST use the same parameter values (version, random, session_id, cipher_suites, compression_method) as it did in the original ClientHello. The server SHOULD use those values to generate its cookie and verify that they are correct upon cookie receipt.

https://www.rfc-editor.org/rfc/rfc9147.html#section-5.3

The ClientHello up to, but not including the Extensions is the same for DTLS1.2 and DTLS1.3

Do not calculate the cookie using the Extensions as these are
different between DTLS1.2 and DTLS1.3

https://datatracker.ietf.org/doc/html/rfc6347#section-4.2.1

When responding to a HelloVerifyRequest, the client MUST use the same
parameter values (version, random, session_id, cipher_suites,
compression_method) as it did in the original ClientHello.  The
server SHOULD use those values to generate its cookie and verify that
they are correct upon cookie receipt.

https://www.rfc-editor.org/rfc/rfc9147.html#section-5.3

The ClientHello up to, but not including the Extensions is the same for
DTLS1.2 and DTLS1.3

Signed-off-by: Jon Shallow <[email protected]>
@boaks
Copy link
Contributor

boaks commented Feb 6, 2024

That mainly fails then later in the handshake, or?

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Feb 6, 2024

The regenerated Cookie for second ClientHello does not match the second ClientHello provided Cookie (generated for the first ClientHello and sent in HelloVerifyRequest) , so a second HelloVerifyRequest is sent.

If the client continued to respond to HelloVerifyRequest with yet another ClientHello, then I guess things may eventually work (unnecessary packet exchanges), but WolfSSL sends back an alert for a duplicated HelloVerifyRequest.

@boaks
Copy link
Contributor

boaks commented Feb 6, 2024

I'm still wondering/confused:
I understand your description of the processing of DTLS 1.2 / 1.3 mixed handshakes.
But that would mean "DTLS 1.3 breaks the backwards compatibility to 1.2". At least if the 1.3 is the client and 1.2 is the server. Not sure, if that is intended by the TLS working group.

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Feb 6, 2024

When testing WolfSSL DTLS1.3 against DTL1.2 servers using OpenSSL, mbedtls, or gnutls there are no issues, so I strongly suspect (not checked) they only check the legacy compatible options rather than adding in all of the extensions.
Certainly looking at wireshark, the extensions are not the same for the second ClientHello.

@boaks
Copy link
Contributor

boaks commented Feb 6, 2024

I guess, there are two understandings of

"same parameter values (version, random, session_id, cipher_suites, compression_method)"

one is, that the list defines exactly the " parameter values", the other, that it defines an example and the extensions may be added. I guess, the first one is the common one. I wrote a e-mail to one of the RFC9147 authors. Let's see if I get an answer.
If not, LGTM.

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Feb 7, 2024

While unclear what should happen with a HelloVerifyRequest (as opposed to a HelloRetryRequest), https://www.ietf.org/archive/id/draft-ietf-tls-esni-17.html#section-6.1.5 will update the ClientHello for the second ClientHello request

@boaks boaks added the available on develop Mark PRs (pre-)available only on develop label Feb 7, 2024
/* Add in the Compression length byte + value */
compatability_length += dtls_uint8_to_int(msg + DTLS_HS_LENGTH + e
+ compatability_length) + 1;

Copy link
Contributor

@boaks boaks Feb 28, 2024

Choose a reason for hiding this comment

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

The other places, where the length of the field is read, a length check is done before reading it.

  if (e + DTLS_HS_LENGTH + sizeof(uint8_t) > msglen)
    return dtls_alert_fatal_create(DTLS_ALERT_HANDSHAKE_FAILURE);
  /* skip cookie bytes and length byte */
  e += dtls_uint8_to_int(msg + DTLS_HS_LENGTH + e);

In order to "standardize" that, we have two macros, SKIP_VAR_FIELD and GET_VAR_FIELD. In this case I guess, using SKIP_VAR_FIELD (together with other modifications) will do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

See PR #230

@boaks
Copy link
Contributor

boaks commented May 12, 2024

Merged alternative implementation with PR #230.

@boaks boaks closed this May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
available on develop Mark PRs (pre-)available only on develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants