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

Move name validation to the authentication section #638

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

martinthomson
Copy link
Contributor

This is partly motivated by my interests in doing something evil, but mostly this is because coupling name validity to ECH config validity is a layering violation. It's fine to invoke some name validation, but that should be dictated by the needs of the thing that ends up relying on that name.

This corrects both problems.

In doing this, I realized that RFC 791 says nothing about the IP address textual format. That's problematic, but I couldn't come up with anything better in short notice.

Closes #628.
Closes #637.

This is partly motivated by my interests in doing something evil,
but mostly this is because coupling name validity to ECH config
validity is a layering violation.  It's fine to invoke some name
validation, but that should be dictated by the needs of the thing that
ends up relying on that name.

This corrects both problems.

In doing this, I realized that RFC 791 says nothing about the IP address
textual format.  That's problematic, but I couldn't come up with
anything better in short notice.

Closes tlswg#628.
Closes tlswg#637.
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
* the sequence does not begin or end with an ASCII dot;
* all labels are at most 63 octets; and
* the sequence does not parse as an IPv4 address {{!RFC0790}} in textual form,
including a hexadecimal form that starts with "0x".
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a behavior change from what's currently in the draft.

This is a bit of a mess. So, it used to be that WHATWG URLs followed this rule. But it leads to weird behaviors like:

  • 1.2 is an IP address
  • a.1.2 is a DNS name
  • The eTLD+1 of a.1.2 is 1.2, which is not a DNS name

To avoid this chaos, the WHATWG rule changed to only key on the final component. If the final component is numeric, it is an IP address (or an error). If the final component is non-numeric, it is a DNS name. This is a much better rule as it preserves the invariant that suffixes of DNS names are still DNS names, and avoids having to account for really weird things like...

  • 1.0xffffff is an IP address (it's 1.255.255.255)
  • 1.0xffffffff is a DNS name (the last component is too big to stand in for the missing components)

Also this rule is still compatible with the IETF URL definition; the goal is to never produce a DNS name that someone else thinks is an IP address.

(Why, oh why, did people make IP addresses and DNS names overlap? That was such a foolish decision! The original rule that LDH labels cannot begin with a digit was great.)

Alternative option

If we want to avoid this mess, here's another option: don't do it at the ECHConfig rejection layer, and do it at public name verification. We say:

  • When you send the ClientHello, just put the public_name in there without thinking about it very much.
  • When you validate the certificate against the public name, you MUST treat it as a DNS name. If your verifier is unwilling to treat that input string as a DNS name (e.g. because it's URL-based and thinks that string is an IP address), you MUST fail verification

The downside here is that now an attacker can induce the client to put an arbitrary string into the server_name field, which the client would otherwise never be willing to send. If we completely do not validate the public name, this is a truly arbitrary string, which might be unwise. For example, if you put a NUL in the public name, you might upset some systems that assumed they could stick SNIs into NUL-terminated C strings.

I'm not sure what the consequences of that are. I think we went with the validating option because there's no real reason to put in so garbage of a string in the public name, and it's tidier to just remove this possible mischief avenue.

The other downside is that publishing an ECHConfig with a garbled public name shoots yourself in the foot because the client will use the ECHConfig but refuse you the recovery flow. But so does publishing an ECHConfig with a syntactically valid public name that you don't control, so I'm not super concerned about this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, this alternative is more or less the approach that I'm angling towards, just as incrementally as I can manage. Your point about name validity and how it has changed over time is well taken; this could be improved further, but maybe we should deal with the macro-level question first: is this general direction sensible?

There's some value in catching mistakes earlier in terms of not expending resources chasing down an obviously unfruitful path, but leave the layering violation to those who want to optimize that way.

Copy link
Collaborator

@davidben davidben Dec 5, 2024

Choose a reason for hiding this comment

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

I'm not sure what "this general direction" is. :)

This PR still encourages checking for IPs before acting on an ECHConfig, but it now changes the criteria from the previous consensus to a different and more complex criteria (checking if something is a valid IPv4 address is... a lot), and one which leads to a notion of DNS name which fails basic suffix invariants. (The WHATWG URL change was prompted by a real world implementation issue found by a fuzzer. Turns out lots of things break when this invariant fails.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "general direction" is toward your alternative. That is, I would have written up a change that implements what you suggested, except that I think it would require more text changes. If you think that doing so is the right thing to do, I am happy to take another go at it. That said, it's going to take me some time.

The IP checks are orthogonal. There is no simple test that applies here, which leads me to think that maybe we do just need to say "it's a name, even if it looks like an IP". That's possible in this case, because public names ultimately only need to match against a dNSName in subjectAltName.

veiled
Co-authored-by: David Benjamin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS issues from AD review.
2 participants