Subject: [secdir] Secdir last call review of draft-ietf-tls-exported-authenticator-09

From: Yaron Sheffer via Datatracker <[email-masked]>
Date: Tue, 16 Jul 2019 12:59:32 -0700

Reviewer: Yaron Sheffer
Review result: Has Issues

The document is generally clear in both its motivation and the proposed
solution.

I think it's playing a bit too liberal with TLS 1.3, in sort-of but not quite
deprecating renegotiation, and in changing the IANA registry in a way that
actually changes the protocol. Details below.

Other comments:

* Abstract: please reword to make it clear that it's the proof (not the cert)
that is portable. * Introduction: TLS 1.3 post-handshake auth "has the
disadvantage of requiring additional state to be stored as part of the TLS
state machine." Why is that an issue is practice, assuming this feature is
already supported by TLS libraries? Also, are we in effect deprecating this TLS
1.3 feature because of the security issue of the mismatched record boundaries?
* "For simplicity, the mechanisms... require", maybe a normative REQUIRE? *
Authenticator Request: please clarify that we use the TLS 1.3 format even when
running on TLS 1.2. * Also, I suggest to change "is not encrypted with a
handshake key" which is too specific to "is sent unencrypted within the normal
TLS-protected data". * Before diving into the detailed messages in Sec. 3, it
would be nice to include a quick overview of the message sequence. * Sec. 3:
"SHOULD use TLS", change to MUST. There's no way it can work otherwise anyway.
* "This message reuses the structure to the CertificateRequest message" -
repeats the preceding paragraph. * Sec. 4: again, SHOULD use TLS -> MUST. * Is
there a requirement for all protocol messages to be sent over a single TLS
connection? If so, please mention it. Certainly this is true for the
Authenticator message that must be sent over a single connection and cannot be
multiplexed by the high-level protocol. I think this protocol actually assumes
"nice" transport properties (no message reorder, reliability) that also require
a single, clean TLS connection. * Why no authenticator request for servers?
Don't we care about replay? OTOH if the client wants to detect replays, it
would need to keep state on received authenticators, which would be very messy.
* 4.1: when referencing Exporters, mention this is Sec. 7.5 of [TLS1.3]. * The
discussion of Extended Master Secret only applies to TLS 1.2 - please clarify
that, plus I suggest to reword this paragraph which I find hard to parse. *
4.2: "the extensions are chosen from the TLS handshake." - What does it mean
exactly, and how does an application even know which extensions were used at
the TLS layer? Reading further, we mention "ClientHello extensions." Maybe the
authenticator should also include a ClientHello message to indicate supported
extensions. Assuming we can peek into ClientHello contradicts the hopeful "even
if it is possible to implement it at the application layer" in Sec. 6. * 4.2.1:
the first sentence of the section is incomplete. * Can I use TLS 1.3-specific
extensions (oid_filters) if I'm running on a TLS 1.2 connection? Is there a
situation where a certificate is acceptable for TLS 1.3 connections but not for
TLS 1.2 connections? * "using a value derived from the connection secrets" - I
think we should recommend a specific construction here to ensure people do it
securely. * 4.2.3: Are we computing HMAC(MAC-key, Hash(a || b || c || d))? If
so, please say so. * 4.2.4: "a constant-time comparison" - why is it actually
required, what is the threat? An attacker can do very little because each
authenticator being sent is different. * 5: please say explicitly which
messages are used in this case to construct the authenticator. * 6.1: the
"MUST" is strange in a section that's only supposed to be informative. Also,
the library may provide this extension (and possibly others) without requiring
it as input. * 6.4: "The API MUST return a failure if the
certificate_request_context of the authenticator was used in a previously
validated authenticator." Are we requiring the library to keep previous
contexts for replay protection? If so, please make it explicit. *  7.1: this is
changing TLS 1.3 by allowing this extension in Cert Request! I think it's a
really bad idea. Better to hack special support to existing libraries, allowing
this specific extension for this special case, than to change the base
protocol. Otherwise, this document should UPDATE 8446. * 8: I think the
Security Considerations is the right place to talk about interaction between
this protocol and TLS 1.3 (or 1.2) renegotiation. Even if we simply RECOMMEND
not to do it. Or else explain the use cases where renegotiation is still useful
if there are any.

---

From: Nick Sullivan <[email-masked]>
Date: Tue, 16 Jul 2019 16:24:44 -0700

Yaron,

Thank you for this review. I'm going to internalize it, come up with
potential ways forward and get back to you soon.

Best,
Nick

---

From: Benjamin Kaduk <[email-masked]>
Date: Sat, 20 Jul 2019 20:57:00 -0500

Hi Yaron,

Thanks for the thoughtful review.  It seems like we'll need to think about
some points a bit more...

> actually changes the protocol. Details below.

To be clear, 1.3 does not have renegotiation at all, so that deprecation
has already been done and not by this document.

> that is portable.
> * Introduction: TLS 1.3 post-handshake auth "has the

It requires either caching the entire handshake messages (including
certificate), which can add up, or being able to "fork" a copy of internal
hash state, which is a pretty uncommon API.

> 1.3 feature because of the security issue of the mismatched record boundaries?

I think in some peoples' minds that would be a good thing, though it's not
entirely clear that we are actually doing so.

> running on TLS 1.2.
> * Also, I suggest to change "is not encrypted with a
> TLS-protected data".
> * Before diving into the detailed messages in Sec. 3, it

What about QUIC?

> repeats the preceding paragraph.
> * Sec. 4: again, SHOULD use TLS -> MUST.
> * Is
> a single, clean TLS connection.

It seems like we should be more clear about what we do (and do not)
require.  Though IIRC we can do separate authenticators in parallel and not
require them to be ordered with respect to each other.

> * Why no authenticator request for servers?

IIUC the thinking is that there's no way to revoke an authenticator for a
connection, so replay (if  not detected by another layer anyway) would not
cause the authentication state to change.

> that, plus I suggest to reword this paragraph which I find hard to parse.

(RFC 8446 tries to say "when something  requires EMS, TLS 1.3 counts for
that", but it's not great.)

> *
> TLS 1.2 connections?
> * "using a value derived from the connection secrets" - I
> securely.
> * 4.2.3: Are we computing HMAC(MAC-key, Hash(a || b || c || d))? If
> so, please say so.
> * 4.2.4: "a constant-time comparison" - why is it actually
> authenticator being sent is different.

I  think this text was added because of  a question I asked during AD
review, and we didn't put a huge amount of thought into it.

> * 5: please say explicitly which
> it as input.
> * 6.4: "The API MUST return a failure if the
> protocol. Otherwise, this document should UPDATE 8446.

Or, since extension codepoints are cheap, just use a newcodepoint.

-Ben

> * 8: I think the

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 22 Jul 2019 00:04:51 -0500

It seemed worth copying this to the WG list; sorry for those getting an
extra copy.

-Ben

On Sun, Jul 21, 2019 at 10:26:56AM -0400, Martin Thomson wrote:
> On Sat, Jul 20, 2019, at 21:57, Benjamin Kaduk wrote:
> This wouldn't deprecate post-HS auth.  It is intended to cover the cases where post-HS auth does not work.  See draft-ietf-httpbis-http2-secondary-certs.
> 
> I think that we can safely regard QUIC (at least those versions that use TLS) as TLS.  An informative mention might help avoid any confusion regarding this point.  We might also regard DTLS-SRTP as one such protocol.
> 
> > > * Is
> See above regarding DTLS-SRTP.  Being too crisp might constrain usage inappropriately.
> 
> Ben is correct.
> > > *  7.1: this is
> We might want to discuss this one this week if we can.
>