Subject: [TLS] New review through the TLS 1.3 Editor's Copy

From: Ilari Liusvaara <[email-masked]>
Date: Mon, 17 Oct 2016 21:10:30 +0300

Did a new review of the document, given that stuff from the older
review got addressed.

Consulted the older review when making this, but not the issue lists:

> EncryptedExtensions.
> : responses to any extensions that are not required to
>   determine the cryptographic parameters. [{{encrypted-extensions}}]

Also, with Certificate extensions, the extensions here need to not
be specific to a certificate or certificate chain.

> Finally, the client and server exchange Authentication messages. TLS
> uses the same set of messages every time that authentication is needed.
> Specifically:
> 
> Certificate.
> : the certificate of the endpoint. This message is omitted if the
>   server is not authenticating with a certificate. Note that if raw
>   public keys {{RFC7250}} or the cached information extension
>   {{?RFC7924}} are in use, then this message will not
>   contain a certificate but rather some other value corresponding to
>   the server's long-term key.  [{{certificate}}]

This is also omitted for client if CertificateRequest was not sent,
right?

> CertificateVerify.
> : a signature over the entire handshake using the public key
>   in the Certificate message. This message is omitted if the
>   server is not authenticating via a certificate. [{{certificate-verify}}]

Same as above. And additionally, for client, doesn't sending an
empty certificate omit this?

> ## Zero-RTT Data
> 
> With the Zero-RTT mode, clients can send data on their first flight
> ("early data") whereby the client uses a PSK either obtained 
> out-of-band or as a ticket (i.e., one with the "early_data_info" 
> extension) from an earlier exchange to authenticate to the server. 
> 
> When clients use a PSK obtained out-of-band then the following 
> information MUST be provisioned to both parties:
> 
>   * The PSK identity
>   * The cipher suite for use with this PSK
>   * The key exchange and authentication modes this PSK is allowed to be used with
>   * The Application-Layer Protocol Negotiation (ALPN) label(s)
>   * The Server Name Indication (SNI), if any is to be used

The ALPN labels can't be plural. There must (nothing else will work) be
a single ALP (possibly being the implicit default ALP) for any 0-RTT-
capable PSK.

Also, AFAIK, the reason why ticket_age exists in PSK is to support
0-RTT anti-replay. Without 0-RTT data, there is no point in using
anything that doesn't involve server-side contribution anyway (and
thus would get superrior security properties).

> 2. There are no guarantees of non-replay between connections.
> Unless the server takes special measures outside those provided by TLS,
> the server has no guarantee that the same
> 0-RTT data was not transmitted on multiple 0-RTT connections
> (See {{replay-time}} for more details).
> This is especially relevant if the data is authenticated either
> with TLS client authentication or inside the application layer
> protocol. However, 0-RTT data cannot be duplicated within a connection (i.e., the server
> will not process the same data twice for the same connection) and
> an attacker will not be able to make 0-RTT data appear to be
> 1-RTT data (because it is protected with different keys.)

Of course, if the server TLS library provodes the difference between
0-RTT and 1-RTT data is a different matter...

> ## Decoding Errors
> 
> TLS defines two generic alerts (see {{alert-protocol}}) to use upon failure to parse
> a message. Peers which receive a message which cannot be parsed according to the syntax
> (e.g., have a length extending beyond the message boundary or contain an out-of-range
> length) MUST terminate the connection with a "decoding_error" alert. Peers which receive
> a message which is syntactically correct but semantically invalid (e.g., a DHE share of p - 1)
> MUST terminate the connection with an "illegal_parameter" alert.

What alert is used if some field is an non-extensible enumeration and
the value transmitted is outside the legal values?

E.g. An unknown max_fragment_length value.

I have used illegal_parameter for errors like this.

Also, isn't the alert name decode_error, not decoding_error?

> ###  Client Hello
>
> In the event that a client requests additional functionality using
> extensions, and this functionality is not supplied by the server, the
> client MAY abort the handshake. Note that TLS 1.3 ClientHello messages
> MUST always contain extensions, and a TLS 1.3 server MUST respond to
> any TLS 1.3 ClientHello without extensions or with data following
> the extensions block with a "decode_error"
> alert. TLS 1.3 servers may receive TLS 1.2 ClientHello messages
> without extensions. If negotiating TLS 1.2, a server MUST check that
> the message either contains no data after legacy_compression_methods
> or that it contains a valid extensions block with no data following.
> If not, then it MUST abort the handshake with a "decode_error" alert.

Note that it is now impossible to receive a TLS 1.3 ClientHello without
extensions (any such would be interpretted as TLS 1.2 ClientHello
without extensions).

> ###  Server Hello
> 
> version
> : This field contains the version of TLS negotiated for this session.  Servers
>   MUST select the lower of the highest supported server version and the version
>   offered by the client in the ClientHello.  In particular, servers MUST accept
>   ClientHello messages with versions higher than those supported and negotiate
>   the highest mutually supported version.  For this version of the
>   specification, the version is { 3, 4 }.  (See {{backward-compatibility}} for
>   details about backward compatibility.)

Err, don't they have to select highest client indicated version that they
support (now that version negotiation is with extensions)?

> ##  Hello Extensions
> 
> An extension type MUST NOT appear in the ServerHello or HelloRetryRequest
> unless the same extension type appeared in the corresponding ClientHello.
> If a client receives an extension type in ServerHello or HelloRetryRequest
> that it did not request in the associated ClientHello, it MUST abort the
> handshake with an "unsupported_extension" fatal alert.

Add note that Cookie is an exception to this?

> When multiple extensions of different types are present in the ClientHello or
> ServerHello messages, the extensions MAY appear in any order. There MUST NOT be
> more than one extension of the same type.

pre_shared_key is an exception to this in ClientHello, right?

> In general, the specification of each extension type needs to describe the
> effect of the extension both during full handshake and session resumption. Most
> current TLS extensions are relevant only when a session is initiated: when an
> older session is resumed, the server does not process these extensions in
> ClientHello, and does not include them in ServerHello. However, some
> extensions may specify different behavior during session resumption.
> [[TODO: update this and the previous paragraph to cover PSK-based resumption.]]

Basically, treating any extension specially with regards to full
handshake versus "resumption" is very likely to be a Bad Idea and
there should be good justifications on doing so.

(Such difference would likely be a major source of implementation bugs.)

> ###  Signature Algorithms
> 
> The client uses the "signature_algorithms" extension to indicate to the server
> which signature algorithms may be used in digital signatures. Clients which
> desire the server to authenticate via a certificate MUST send this extension.
> If a server
> is authenticating via a certificate and the client has not sent a
> "signature_algorithms" extension then the server MUST
> abort the handshake with a "missing_extension" alert
> (see {{mti-extensions}}).
> 
> Servers which are authenticating via a certificate MUST indicate so
> by sending the client an empty "signature_algorithms" extension.

This looks to be a Bad Idea. It was to support that poorly tought-out
server certificate authentication in PSK mode, right?

Basically, offering any means (even if there is "MUST" to "prevent" the
use) of disabling server authentication in non-PSK mode is a footgun.

> ### Negotiated Groups
> 
> As of TLS 1.3, servers are permitted to send the "supported_groups"
> extension to the client. If the server has a group it prefers to the
> ones in the "key_share" extension but is still willing to accept the
> ClientHello, it SHOULD send "supported_groups" to update the client's
> view of its preferences; this extension SHOULD contain all groups
> the server supports, regardless of whether they are currently
> supported by the client. Clients MUST NOT act upon any information
> found in "supported_groups" prior to successful completion of the
> handshake, but MAY use the information learned from a successfully
> completed handshake to change what groups they offer to a server in
> subsequent connections.

This means altering the set of key shares they offer to the server,
right? Not the set of groups they indicate as supported?

> ### Key Share
> 
> Upon receipt of this extension in a HelloRetryRequest, the client MUST first
> verify that the selected_group field corresponds to a group which was provided
> in the "supported_groups" extension in the original ClientHello. It MUST then
> verify that the selected_group field does not correspond to a group which was
> provided in the "key_share" extension in the original ClientHello. If either of
> these checks fails, then the client MUST abort the handshake with an
> "illegal_parameter" alert.  Otherwise, when sending the new ClientHello, the
> client MUST append a new KeyShareEntry for the group indicated in the
> selected_group field to the groups in its original KeyShare. The remaining
> KeyShareEntry values MUST be preserved.

Err... Hasn't the KeyShare behaviour been changed from "append" to
"replace"?

> ### Pre-Shared Key Extension
> 
> obfuscated_ticket_age
> : For each ticket, the time since the client learned about the server
>   configuration that it is using, in milliseconds.  This value is
>   added modulo 2^32 to with the "ticket_age_add" value that was
>   included with the ticket, see {{NewSessionTicket}}.  This addition
>   prevents passive observers from correlating sessions unless tickets
>   are reused.  Note: because ticket lifetimes are restricted to a
>   week, 32 bits is enough to represent any plausible age, even in
>   milliseconds. External tickets SHOULD use an obfuscated_ticket_age of
>   0; servers MUST ignore this value for external tickets.

The comment above about purpose of ticket_age also applies here.

> ### Pre-Shared Key Exchange Modes
> 
> In order to use PSKs, clients MUST also send a "psk_key_exchange_modes"
> extension. The semantics of this extension are that the client only
> supports the use of PSKs with these modes, which restricts both the
> use of PSKs offered in this ClientHello and those which the server
> might supply via NewSessionTicket.

Why is this a separate extension and not part of pre_shared_key, given
that if the latter is present, this must be too?

Seems like more complexity for absolutely no reason. I would understand
if this extension was optional if PSK is present, but it is not.

> ### Early Data Indication
> 
> When PSK resumption is used, the client can send application data
> in its first flight of messages. If the client opts to do so, it MUST
> supply an "early_data" extension as well as the "pre_shared_key"
> extension.

Can't early data now be used with any type of PSK, not just ones
provisioned by NewSessionTicket?

> A server MUST validate that the ticket age for the selected PSK
> identity (computed by un-masking PskIdentity.obfuscated_ticket_age)
> is within a small tolerance of the time since the ticket was
> issued (see {{replay-time}}).  If it is not, the server SHOULD proceed
> with the handshake but reject 0-RTT, and SHOULD NOT take any other action
> that assumes that this ClientHello is fresh.

PSKs that are externally provisioned don't have any valid ticket age...

> In order to accept early data, the server server MUST have accepted a
> PSK cipher suite and selected the the first key offered in the
> client's "pre_shared_key" extension. In addition, it MUST verify that
> the following values are consistent with those negotiated in the
> connection during which the ticket was established.
> 
> - The TLS version number, AEAD algorithm, and the hash for HKDF.
> - The selected ALPN {{!RFC7443}} value, if any.

Actually, it must verify that the record protection matches, not
just the HKDF.

> ## Server Parameters
> 
> The same extension types MUST NOT appear in both the ServerHello and
> EncryptedExtensions. All server-sent extensions other than those explicitly
> listed in {{server-hello}} or designated in the IANA registry MUST only
> appear in EncryptedExtensions. Extensions which are designated to
> appear in ServerHello MUST NOT appear in EncryptedExtensions. Clients
> MUST check EncryptedExtensions for the presence of any forbidden
> extensions and if any are found MUST abort the handshake with an
> "illegal_parameter" alert.

Also there are extensions in Certificates as well...

> #### Receiving a Certificate Message
>
> Any endpoint receiving any certificate signed using any signature algorithm
> using an MD5 hash MUST abort the handshake with a "bad_certificate" alert.
> SHA-1 is deprecated and it is RECOMMENDED that
> any endpoint receiving any certificate signed using any signature algorithm
> using a SHA-1 hash abort the handshake with a "bad_certificate" alert.
> All endpoints are RECOMMENDED to transition to SHA-256 or better as soon
> as possible to maintain interoperability with implementations
> currently in the process of phasing out SHA-1 support.

These requirements are problematic, and I have absolutely no intention
of ever implementing this in my TLS libs. They treat signatures involving
MD5 or SHA1 the same as unknown signature algorithm (which is not a fatal
error).

To be clear, MD5 (and SHOULD for SHA-1) signatures are absolutely not
to be trusted. But those should be treated as unknown with no special
handling.

Also, if there are self-signed certs, the self-signature is just to be
ignored. There are still CA root certs out there with MD5 signatures.

> %%% Authentication Messages

> If sent by a server, the signature algorithm MUST be one offered in the
> client's "signature_algorithms" extension unless no valid certificate chain can be
> produced without unsupported algorithms (see {{signature-algorithms}}).

This is seemingly about server signatures. In that context, an
unknown algorithm has absolutely no chance of working.

> ### New Session Ticket Message {#NewSessionTicket}
> 
> Any ticket MUST only be resumed with a cipher suite that is identical
> to that negotiated connection where the ticket was established.

This kind of requirement is problematic when "external" PSKs have
requirement that just the hash matches. (Implementation bugs).

-Ilari

---

From: Dave Garrett <[email-masked]>
Date: Mon, 17 Oct 2016 15:18:34 -0400

> unknown algorithm has absolutely no chance of working.

This came up in a discussion a while back and we decided to allow unsupported algorithms as a last-ditch fall-back. Opportunistic encryption might not care and there are systems that may trust certs as a whole, not caring about the signatures. The end result is that the client should be tasked with making the decision to accept or reject, not the server. Also can be helpful for debugging.

Dave

---

From: Ilari Liusvaara <[email-masked]>
Date: Mon, 17 Oct 2016 22:41:58 +0300

On Mon, Oct 17, 2016 at 03:18:34PM -0400, Dave Garrett wrote:
> This came up in a discussion a while back and we decided to allow
> unsupported algorithms as a last-ditch fall-back. Opportunistic
> encryption might not care and there are systems that may trust
> certs as a whole, not caring about the signatures. The end result
> is that the client should be tasked with making the decision to
> accept or reject, not the server. Also can be helpful for debugging.

The only possible way stuff like this is going to work is for the
client either ignores Certificate/CertificateVerify entierely (can't
trust the certificate via any method) or if the client has serious
security bug.

While e.g. trusting the server by SPKI does bypass any and all
signatures in certificates, it does not bypass this server signature.

-Ilari

---

From: Hubert Kario <[email-masked]>
Date: Tue, 18 Oct 2016 12:40:35 +0200

On Monday, 17 October 2016 21:10:30 CEST Ilari Liusvaara wrote:
> > ## Decoding Errors
> > 
> > TLS defines two generic alerts (see {{alert-protocol}}) to use upon
> > failure to parse a message. Peers which receive a message which cannot be
> > parsed according to the syntax (e.g., have a length extending beyond the
> > message boundary or contain an out-of-range length) MUST terminate the
> > connection with a "decoding_error" alert. Peers which receive a message
> > which is syntactically correct but semantically invalid (e.g., a DHE
> > share of p - 1) MUST terminate the connection with an "illegal_parameter"
> > alert.
> 

That would be my reading of the text too.

Anything that can be parsed given the requirements on structs should not 
generate decode_error, if it needs to be rejected because of other 
limitations, it should result in an illegal_parameter.

-- 
Regards,
Hubert Kario
Senior Quality Engineer, QE BaseOS Security team
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 18 Oct 2016 18:43:25 -0500

[I trimmed a couple things that already had sub-threads spun off;
leaving in others even where I don't have a comment right now]

> This is also omitted for client if CertificateRequest was not sent,

That as my understanding.

> empty certificate omit this?

For post-handshake, that is explicitly stated.  It is also implicitly
stated for in-handshake ("Clients MUST send this message
[CertificateVerify] whenever authenticating via a Certificate (i.e.,
when the Certificate message is non-empty)."

> a single ALP (possibly being the implicit default ALP) for any 0-RTT-

I pointed this out on Kyle's declined pull request and he fixed it in a
later one that has already been merged.

> thus would get superrior security properties).

I basically agree, and see only minimal reason why the ticket_age_add
needs to be mandatory.  Sure, it can give the server some insight into
rejecting stale tickets, but there are other ways to do so.

> 0-RTT and 1-RTT data is a different matter...

Yes, the library has to tell the application that 0-RTT data and 1-RTT
data are different, or bad things happen.  Watson had some text to that
effect in PR 694, which was not accepted (some text in a
counter-proposal by Martin Thomson was accepted).

> support (now that version negotiation is with extensions)?

Basically.  It is not clear to me whether there is consensus to say
"highest version" or just "server picks".

> Add note that Cookie is an exception to this?

I thought there was already an exception for cookie:

   [...]  As with
   ServerHello, a HelloRetryRequest MUST NOT contain any extensions that
   were not first offered by the client in its ClientHello, with the
   exception of optionally the "cookie" (see Section 4.2.2
<https://tools.ietf.org/html/draft-ietf-tls-tls13-16#section-4.2.2>) extension.

Though perhaps it is still worth duplicating that guidance so it's
easier to find.

> pre_shared_key is an exception to this in ClientHello, right?

Yes.

> Err... Hasn't the KeyShare behaviour been changed from "append" to

I thought so, yes.

> if this extension was optional if PSK is present, but it is not.

The justification for making it separate is that the
psk_key_exchange_modes also applies to what the server will send back in
NST, so in theory one could send psk_key_exchange_modes but not
pre_shared_key.  I'm still a little dubious, but accepted the justification.

> provisioned by NewSessionTicket?

Yes.

-Ben

> _______________________________________________
> TLS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tls

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 18 Oct 2016 20:34:54 -0400

I've updated the draft in response to a bunch of these comments and
scheduled some for update when I enter my own review comments which
are still in the "marked up document stage"

> be specific to a certificate or certificate chain.

Updated.

> empty certificate omit this?

Updated both of these.

> a single ALP (possibly being the implicit default ALP) for any 0-RTT-

This was updated in PR#706.

> thus would get superrior security properties).

I don't quite agree with this. I moved it from the 0-RTT messages to the
PSK messages so that the server had an opportunity to detect replay
of the ClientHello in case it was going to send 0.5 RTT data or other
side effects. This is an observation due to someone on the miTLS team
(can't remember who) that 0.5RTT (even without 0-RTT client data)
is like a client 0-RTT "I am here" message.

> > TLS defines two generic alerts (see {{alert-protocol}}) to use upon
failure to parse
> I have used illegal_parameter for errors like this.

Yeah, this seems like illegal_parameter.

I updated to make this clear.

> Also, isn't the alert name decode_error, not decoding_error?

Fixed.

> extensions (any such would be interpretted as TLS 1.2 ClientHello

True. Updated.

> support (now that version negotiation is with extensions)?

This was recently updated to just require that the pick from the list.

> Add note that Cookie is an exception to this?

Noted.

> pre_shared_key is an exception to this in ClientHello, right?

Thanks. Good catch.

> (Such difference would likely be a major source of implementation bugs.)

Yes. I have in my notes to fix this.

> server certificate authentication in PSK mode, right?

This is just cruft from before. Thanks for catching it.

> right? Not the set of groups they indicate as supported?

Clarified.

> The comment above about purpose of ticket_age also applies here.

See above.

> if this extension was optional if PSK is present, but it is not.

As noted elsewhere, this allows the server to know whether it will be
able to send a ticket that will be acceptable to the client in
advance.

> provisioned by NewSessionTicket?

Yes, thanks.

> PSKs that are externally provisioned don't have any valid ticket age...

Yes, thanks.

> Actually, it must verify that the record protection matches, not

Note that it says "AEAD algorithm", but this was before we broke the
cipher suites apart, so I have changed this to "cipher suite"

> Also there are extensions in Certificates as well...

Yes. I have scheduled a more general cleanup here.

> unknown algorithm has absolutely no chance of working.

I generally share your view of these requirements, but they were the
result of a lot of list discussion, so I think we'll need a new
PR to change it. If you want to produce one, that would be useful
for discussion.

> requirement that just the hash matches. (Implementation bugs).

Agreed. I will harmonize these.

-Ekr

> -Ilari

On Mon, Oct 17, 2016 at 2:10 PM, Ilari Liusvaara <[email-masked]>
wrote:

> failure to parse