Re: [TLS] Don't Split HelloRetryRequest

David Benjamin <davidben@chromium.org> Thu, 01 April 2021 18:56 UTC

Return-Path: <davidben@google.com>
X-Original-To: tls@ietfa.amsl.com
Delivered-To: tls@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id AD3523A1F20 for <tls@ietfa.amsl.com>; Thu, 1 Apr 2021 11:56:47 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -9.25
X-Spam-Level:
X-Spam-Status: No, score=-9.25 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HEADER_FROM_DIFFERENT_DOMAINS=0.249, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001, USER_IN_DEF_SPF_WL=-7.5] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=chromium.org
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Bw5gTm8cf2UA for <tls@ietfa.amsl.com>; Thu, 1 Apr 2021 11:56:42 -0700 (PDT)
Received: from mail-pg1-x529.google.com (mail-pg1-x529.google.com [IPv6:2607:f8b0:4864:20::529]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 638FD3A1F1F for <tls@ietf.org>; Thu, 1 Apr 2021 11:56:42 -0700 (PDT)
Received: by mail-pg1-x529.google.com with SMTP id p12so2127957pgj.10 for <tls@ietf.org>; Thu, 01 Apr 2021 11:56:42 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=29KZuGSq/NzYQyEzZPHbwj9jJfR7o/ta6Po7UPuXsTg=; b=HgiBcmQa0ngNs9xD4h3rMVF5E7WiUkRFgU8+Xh1PiC5sCw+NzlWeiOiBhMnqSfVYuZ tshmGTzQgh36pUBAYGSQ/YmD0EsP3irqEtG59/DG5HNemIx6LQrRiSvrl1Jpk8PwxSF9 ouNeAu7dicC8mV8TdHd0E+wVtdVJXgxrlG+ls=
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=29KZuGSq/NzYQyEzZPHbwj9jJfR7o/ta6Po7UPuXsTg=; b=ciJM0cRcFwkrtz7q39/wuP9ye0ZbOJy3UT9htMTtTwUPGe6aVsPzoOW6AOjn5jSLyh /tVIQya1X4Uzu3MINarq/Kt8Kh5WLtfGX5/64qkFBeKXlssIW3u7HglaG/R3MuLBX7wd jU3SXy5Z7jZeG+Hahl8bQi+Aj7epKuda64N6oxc1AkxcWY5oqGSV5m5B9OzK8EP4o1/b olVpBdnhTgMlB/FbvEO6d8NScXq6X1NUM7hsN00hQzhmoCQ8CZOT2AR3m+5CncdmqFC4 Ahzy5s78gFPp4buPM04Y5oLM0dqp6A0Sef1eN8UtnNC81nS/uHKq9NA+1NwGTSdSyRDe tlJg==
X-Gm-Message-State: AOAM5312VRYCQcwGgDAWR5HlCgBqs7Ybi8vBntTmQ3bqsZby3xkOkUJD H1JUvdkFeF50sqpHyWbZgv3IUwN3Kc7FilpiouxYaVzbtBaO
X-Google-Smtp-Source: ABdhPJybl0aqFjsnVp/0NGBevLtSO1NfzNwDF+2BSNwtrrT8ztheSmm+i9cELwX82XBFoDL1miKHsoER/nlrpsiu/x0=
X-Received: by 2002:a63:2e47:: with SMTP id u68mr8875797pgu.6.1617303399985; Thu, 01 Apr 2021 11:56:39 -0700 (PDT)
MIME-Version: 1.0
References: <d0758a0a-737b-40ac-8189-1b4168510859@www.fastmail.com>
In-Reply-To: <d0758a0a-737b-40ac-8189-1b4168510859@www.fastmail.com>
From: David Benjamin <davidben@chromium.org>
Date: Thu, 1 Apr 2021 14:56:23 -0400
Message-ID: <CAF8qwaBUp9OzmDGr0f74fGvyDyzB=VdOzYt2nUecoPC+CYebtg@mail.gmail.com>
To: Martin Thomson <mt@lowentropy.net>
Cc: "<tls@ietf.org>" <tls@ietf.org>
Content-Type: multipart/alternative; boundary="000000000000a6c63805beedca64"
Archived-At: <https://mailarchive.ietf.org/arch/msg/tls/XMw0VBjW3zX7vEjLscdr_iSoIGY>
Subject: Re: [TLS] Don't Split HelloRetryRequest
X-BeenThere: tls@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "This is the mailing list for the Transport Layer Security working group of the IETF." <tls.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/tls>, <mailto:tls-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/tls/>
List-Post: <mailto:tls@ietf.org>
List-Help: <mailto:tls-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/tls>, <mailto:tls-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 01 Apr 2021 18:56:48 -0000

Hi Martin,

Thanks for the comments. As the author of #374, I obviously didn't think it
so clearly unnecessary, but glad to hear your thoughts. Hopefully we can
get on the same page as to what the issues are and/or a better solution.
(Always happy to replace something with a simpler option, provided it also
works!) To that end, I'd second Chris's suggestion of working through an
alternate PR so we have something concrete.

Going through some of these in detail (inline)

On Wed, Mar 31, 2021 at 9:58 PM Martin Thomson <mt@lowentropy.net> wrote:

> # Matching Inner and Outer Values
>
> When we get right down to it, there are a very small number of things that
> truly change in response to HelloRetryRequest.  And all of these changes
> are to values that do not need confidentiality protection.
>
> The draft lists three fields that change: ciphersuites, key_share, and
> version.  From my perspective, changing cipher suites, supported groups, or
> versions would be a big mistake.  So what changes is even more limited.
> Just the shares in key_share.
>
> On this basis, a client that offers cipher suites, groups, versions, and
> key shares that are identical in both inner and outer ClientHello messages
> will always receive a HelloRetryRequest that applies equally to both
> messages.
>
> The only adjustment that is acceptable with respect to these fields being
> identical is the addition of TLS 1.2-only options to the outer ClientHello
> (or the removal of the same from the inner ClientHello if you prefer it
> that way around).  This is a fine optimization on the basis that accepting
> ECH represents a commitment to support TLS 1.3 (or higher).  But it is
> really just an optimization (the draft makes this mandatory, which is
> silly).  The client can therefore remove options from the inner ClientHello
> only if it is impossible to select them with TLS 1.3 or higher.
>
> For new extensions, if they define a means of adjustment or correction via
> HelloRetryRequest (there is currently just one: password_salt, which I
> haven't examined), then they too need to follow this restriction.   It's
> not an onerous one.
>
> Follow this simple constraint and HelloRetryRequest will always apply
> equally to both inner and outer ClientHello and everything works.
> Conveniently, this is more or less exactly what the current draft says.
> Almost.
>

This was a previous attempt at this, but it seems to have been confusing.

I do agree that having ClientHelloInner vs ClientHelloOuter differences
that result in different HelloRetryRequest acceptance criteria is a bad
plan: in general, I think a client implementation should be judicious about
which difference it exposes because each difference needs special logic to
make sure it's using the right set as you evaluate the handshake.

At the same time, I worry that *mandating* it in the extension will make
this decision process tricky to navigate and get us in a rough corner. PR
#316 was trying to capture exactly this, but it seems we didn't get on the
same page. (Maybe we can now?) See this discussion:
https://github.com/tlswg/draft-ietf-tls-esni/pull/316#discussion_r509880007

And so the text ended up with something self-contradictory. It says:

"[...] clients offering ECH MUST ensure that all extensions or parameters
that might change in response to receiving a HelloRetryRequest have the
same values in ClientHelloInner and ClientHelloOuter."
https://tlswg.org/draft-ietf-tls-esni/draft-ietf-tls-esni.html#name-handling-helloretryrequest

And then it goes to list things that differ from this criteria! Indeed the
list is right and the criteria is wrong. The criteria is neither sufficient
(we do need cipher suites to align), nor sufficient (as you note, the key
share *values* don't strictly need to match). I concluded from this
exercise that trying to formulate the rules is a bit of a mess. As you
allude to with password_salt, we need to formulate something that can
capture future HRR extensions. (I'd also gotten that sense from one of the
past WG meetings: land the HRR-matching rule as a bandaid, but we didn't
have consensus for it as an actual solution.)

I actually had not heard of password_salt, so thanks for the pointer. I've
only skimmed it but, at a glance, this seems exactly like something where
you'd want ClientHelloInner vs ClientHelloOuter to differ. It seems to be
about some password-based TLS authentication, but any client authentication
should only apply to the true ClientHelloInner handshake and not the
fallback ClientHelloOuter handshake.


> The draft currently allows inner and outer ClientHello to have different
> types of key share.  The way it handles this is terrible: it recommends
> breaking the transcript.  To me, that seems like it would only serve to
> open the protocol up to downgrade attack.
>

I'm not sure I follow this point. Do you mean the draft as-is, or the PR? I
don't believe the draft as-is does, and I don't think the PR does anything
to the transcript that draft as-is doesn't already do. The transcript
behaviors are largely forced by a combination of backwards compatibility
for ClientHelloOuter and split mode for ClientHelloInner.

Incidentally, I don't see a problem with having a different key share
> *value* in inner and outer ClientHello.  There's no point in doing that
> unless you believe that these values leak information (they really
> shouldn't), but it wouldn't break this model if a client decided to do that.
>
> https://github.com/tlswg/draft-ietf-tls-esni/issues/333 appears to be
> concerned about the cookie only applying to one or other ClientHello.  I
> don't see how is the case, so I'm just going to say that this is fixed by
> having HelloRetryRequest apply to both inner and outer ClientHello
> messages.  If the client receives HelloRetryRequest that applies to just
> one of the two, then the problem is that the client is faulty.  That would
> be treated as a programming error as normal (crash, open a bug report, send
> an internal_error alert, etc...).
>

I think you may have misunderstood #333. That or I'm misunderstanding your
response. Imagine a client-facing server that wants to support HRR
statelessly. When that client-facing server accepts ECH and gets an HRR
from the backend server to forward, it has no opportunity to inject its own
cookie.

Maybe our answer is client-facing servers always do HRR on ECH accept. I'm
perfectly fine with that, but if that's our answer, we should explicitly
make that decision.


> Then there are the things that more or less have to change in response to
> HelloRetryRequest, but really only because the ClientHello changes:
> padding, pre_shared_key, and ECH itself.  For those, we need to address a
> minor inconsistency problem at the level of the core protocol itself.
>
> # Addressing Minor HelloRetryRequest Problems
>
> We do need to fix RFC 8446 rules regarding HelloRetryRequest.  David
> already suggested some minute adjustments for that problem in
> https://github.com/tlswg/draft-ietf-tls-esni/issues/358 .  The short
> version is that extensions can define their own rules for how they change
> after HelloRetryRequest.  This is a good amendment, especially as it
> relates to extensions that are not known to the server.
>
> That tweak does have deployment issues, because the original rules have
> been interpreted too literally in some cases, but that should not affect
> ECH specifically.  Servers that have this bug won't be able to deploy ECH
> without fixing the bug and that's OK.  Other servers will only see grease.
>
> The draft currently mandates that greasing values not change after
> HelloRetryRequest, which will avoid this compatibility bug, but also reveal
> the fraud.  I can tolerate that small amount of leakage.
>

Independent of what we do in ECH, I like that adjustment to HRR rules. If
we want to do that as part of resolving the ECH issue, we should make that
decision in rfc8446bis. I filed
https://github.com/tlswg/tls13-spec/issues/1223, but it's not clear that
has consensus yet.

In terms of sticking out, Ben Schwartz also pointed out another issue with
the current situation. It's not just that, where HelloRetryRequest occurs
naturally, this quirk reveals GREASE. A forged HelloRetryRequest also
reveals GREASE. This PR resolves this by authenticating the ECH acceptance
signal in HelloRetryRequest.


> # Avoiding HelloRetryRequest
>
> I think that Nick's suggestion for helping avoid HelloRetryRequest by
> placing hints about key shares in DNS SVCB/HTTPS records is a fine one.
>
> I see the arguments about this being about the configuration needing to
> speak for backend servers when the record relates to frontend servers.  But
> my perspective here is that you already need to ensure that backend servers
> have a consistent cryptographic support profile; adding a small number of
> frontend servers to the set that need to be made consistent isn't that
> difficult.  If this consistency is not possible in some deployments, that's
> understandable, but then it is an optional enhancement that won't be
> available to those deployments, that's all.
>
> Of course, this is an extension that we can pursue separately.
>

I think trying to avoid HRR, provided we actually make it optional, can be
pursued independent of this question. If we say it's optional, we still
need to make ECH + HRR work. If we make it mandatory, I don't see how we
can make it deployable on the server. In particular:

- Servers can't guarantee client behavior a priori, so it'd need a binding
"hint".
- We'd need a story for mismatch between DNS configs and the server, to
keep ECH deployable at all.
- We'd need to be comfortable that every HRR reason, past and present, can
be hinted away by DNS. I suspect cookie and password_hint already break
this.


> # Conclusion
>
> I'm firmly opposed to splitting HelloRetryRequest.  I would like to deploy
> ECH and this doesn't really help with that.
>
> I don't agree that there is a problem that needs to be fixed with the
> current draft.
>
> On the other hand, I can guarantee that this change will delay Firefox
> deployment significantly (that is, for an indefinite period).  It would
> require rearchitecting a piece of code that is rarely used already (despite
> being a source of significant complexity) and replacing it with code that
> is even more complex and would include paths that are even more lightly
> used.
>

I'd be very interested to understand the architectural constraints here.
This didn't seem likely to be particularly challenging to implement, but I
only know our own stack and certainly could have mispredicted.

David