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.

Sign up for GitHub

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
Open

Move name validation to the authentication section #638

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39723d5
Move name validation to the authentication section
martinthomson Nov 26, 2024
aacf20e
Obvious improvements
martinthomson Nov 26, 2024
f6b6111
veiled
martinthomson Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions draft-ietf-tls-esni.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,17 +289,6 @@ public_name
to update the ECH configuration. This is used to correct misconfigured clients,
as described in {{rejected-ech}}.

: Clients MUST ignore any `ECHConfig` structure whose public_name is not
parsable as a dot-separated sequence of LDH labels, as defined in
{{!RFC5890, Section 2.3.1}} or which begins or end with an ASCII dot. Clients
additionally SHOULD ignore the structure if the final LDH label either consists
of all ASCII digits (i.e. '0' through '9') or is "0x" or "0X" followed by some,
possibly empty, sequence of ASCII hexadecimal digits (i.e. '0' through '9', 'a'
through 'f', and 'A' through 'F'). This avoids public_name values that may be
interpreted as IPv4 literals. Additionally, clients MAY ignore the
`ECHConfig` if the length of any label in the DNS name is longer than 63
octets, as this is the maximum length of a DNS label.

: See {{auth-public-name}} for how the client interprets and validates the
public_name.

Expand Down Expand Up @@ -917,6 +906,18 @@ and session IDs presented by the server. These connections are only used to
trigger retries, as described in {{rejected-ech}}. This may be implemented, for
instance, by reporting a failed connection with a dedicated error code.

Prior to attempting a connection, a client SHOULD validate the `ECHConfig` to
ensure that the public_name can be authenticated. Clients SHOULD ignore any
`ECHConfig` structure with a public_name that is not a vaild host name in
martinthomson marked this conversation as resolved.
Show resolved Hide resolved
preferred name syntax (see {{Section 2 of ?DNS-TERMS=RFC8499}}). That is, to be
valid, the public_name needs to be a dot-separated sequence of LDH labels, as
defined in {{Section 2.3.1 of !RFC5890}}, where:

* 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 •
edited
Loading

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.



### Impact of Retry on Future Connections

Expand Down Expand Up @@ -1371,7 +1372,7 @@ has size k = 1. Client-facing servers SHOULD deploy ECH in such a way so as to
maximize the size of the anonymity set where possible. This means client-facing
servers should use the same ECHConfig for as many hosts as possible. An
attacker can distinguish two hosts that have different ECHConfig values based
on the ECHClientHello.config_id value.
on the ECHClientHello.config_id value.

This also means public information in a TLS handshake should be
consistent across hosts. For example, if a client-facing server
Expand Down
Loading