Subject: Re: [TLS] WGLC: draft-ietf-tls-tls13-19

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 4 Apr 2017 18:20:53 -0500

On 03/31/2017 08:40 AM, Hubert Kario wrote:
> On Tuesday, 28 March 2017 08:23:33 CEST Kaduk, Ben wrote:
>> On 3/13/17, 12:30, "Sean Turner" <[email-masked]> wrote:
>> Do we want to add some commentary about the extant SHA1 collisions when we
>> say that {rsa_pkcs1,dsa,ecdsa}_sha1 are only SHOULD NOT?
>  
> There still are non-insignificant number of Internet facing servers that 
> require SHA-1 being advertised for connection to be successful.
> SHOULD NOT is a good compromise for it.

Right.  We could note that though SHA1 is "known to be broken", some
servers currently require it, even though they ought to be moving away
from it posthaste.

-Ben

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 11 Apr 2017 10:32:17 -0700

> It was already mentioned that the “major differences from TLS 1.2”
> section should not be a changelog, but I agree with that.

Yes, this is on my plate.

> Should Figure 4 (“message flow for a zero round trip handshake”)
> include a “+ early_data” for the server’s flight?  (The legend for
> Figure 4 also lacks the explanation for the ‘+’ symbol.)

I see you fixed this.

> The language on page 30 is perhaps unclear:
>
>    Because TLS 1.3 forbids renegotiation, if a server receives a
>    ClientHello at any other time, it MUST terminate the connection.
>
> Is that any TLS server, or just one that has negotiated and is using
> TLS 1.3?

The latter. Adjusted.

> In the description of legacy_compression_methods on page 31, we make
> restrictions on “every TLS 1.3 ClientHello”, but do not say how such
> things are identified.  (Hmm, maybe we also do so elsewhere in the
> document, too, now that I search for where) we explicitly define what
> a client “considered to be attempting to negotiate using this
> specification (i.e., a TLS 1.3 ClientHEello) on page 87, as
> supported_versions including 1.3.  Which, is maybe not the most
> future-proof thing.

I think I feel OK about this.

> The description of version negotiation (to populate
> ServerHello.version) on page 32 seems to leave undefined what the
> server should do when receiving a ClientHello that does not contain a
> supported_versions extension.  (Also, I don’t think
> “ClientHello.supported_versions extension” is a well-defined syntax.)

I think this clear in the section on Supported Versions.

> When covering the server_random version downgrade sentinels, we do not
> mention what is to be done when downgrading to TLS 1.0, which I
> thought was still a permitted version by this spec.

Interesting point. I'm trying to remember why we did things this way.
I am tempted to just say "1.1 or 1.0". Thoughts?

> It’s a little odd that we list in enum ExtensionType on page 35 a
> strict subset of the extensions enumerated in the table on the
> following pages.

This got fixed in PR#936.

> Do we want to add some commentary about the extant SHA1 collisions
> when we say that {rsa_pkcs1,dsa,ecdsa}_sha1 are only SHOULD NOT?

Nah.

> I’ll note that we define 256 private use ECDHE group code points but
> only four such FFDHE group code points.  Probably fine, but a bit
> surprising.

Too late now!!

> Should we forbid duplicate entries in
> PreSharedKeyExtension.identities?

I don't think that's necessary.

> Conversely, we might want to explicitly say that duplicate
> OIDFilter.certificate_extension_oid fields should be expected in
> OIDFilterExtensions, to enable the case where multiple values must be
> present.  Or is that supposed to work by concatenating(?) the multiple
> values’ DER encodings in the certificate_extension_values field?

Yeah, I read this text as saying that those all go in the same
DER, not that there can be multiple copies

> I’ll call out for Russ’s attention at the end of Section 4.4.3 where
> we say that “implementations MUST NOT combine external PSKs with
> certificate-based authentication.”  Is there any reason not to qualify
> that as some sort of “don’t’ do it until it’s defined”?

I'm actually going to move and modify this section per your issue #935.

> Should Alert.level be Alert.legacy_level?

i think we went over this and decided not to.

> Appendix B has a claim that “values listed as _RESERVED were used in
> previous versions of TLS and are listed here for completeness”, though
> that is not exactly true, e.g., for ContentType.invalid_RESERVED(0)

I see you fixed this as well.

> Section C.3 notes that “Certificates should always be verified to
> ensure proper signing by a trusted Certificate Authority”, which does
> not use RFC 2119 language, but might be seen as in conflict with
> opportunistic encryption in some circumstances.  I don’t object to
> this text, but it seems worth mentioning.

I think the "Absent a specific..."

> Page 113 still has the “[[NOTE: TLS 1.3 needs a new channel binding
> definition that has not yet been defined.]]”, which should not make it
> into the final spec!

Fixed.

On Mon, Mar 27, 2017 at 11:23 PM, Kaduk, Ben <[email-masked]> wrote:

>     This is a working group last call announcement for
> draft-ietf-tls-tls13-19, to run through March 27.  Please send your reviews
> to the list as soon as possible so we can prepare for any discussion of
> open issues at IETF 98 in Chicago.
>
> As the price of running the WGLC right during the meeting lead-up, my
> review comes in at the last minute.
>
> Generally, it is in good shape.  I think I still owe some text about what
> we aim for and expect to achieve with respect to side channel resistance,
> though at this point it may be too late to get that text in :(
>
> The following is basically a laundry list of the minor issues; I will send
> editorial notes under separate cover, probably as a pull request.
>
> It was already mentioned that the “major differences from TLS 1.2” section
> should not be a changelog, but I agree with that.
>
> Should Figure 4 (“message flow for a zero round trip handshake”) include a
> “+ early_data” for the server’s flight?  (The legend for Figure 4 also
> lacks the explanation for the ‘+’ symbol.)
>
> The language on page 30 is perhaps unclear:
>
>    Because TLS 1.3 forbids renegotiation, if a server receives a
>    ClientHello at any other time, it MUST terminate the connection.
>
> Is that any TLS server, or just one that has negotiated and is using TLS
> 1.3?
>
> In the description of legacy_compression_methods on page 31, we make
> restrictions on “every TLS 1.3 ClientHello”, but do not say how such things
> are identified.  (Hmm, maybe we also do so elsewhere in the document, too,
> now that I search for where)  we explicitly define what a client
> “considered to be attempting to negotiate using this specification (i.e., a
> TLS 1.3 ClientHEello) on page 87, as supported_versions including 1.3.
> Which, is maybe not the most future-proof thing.
>
> The description of version negotiation (to populate ServerHello.version)
> on page 32 seems to leave undefined what the server should do when
> receiving a ClientHello that does not contain a supported_versions
> extension.  (Also, I don’t think “ClientHello.supported_versions
> extension” is a well-defined syntax.)
>
> When covering the server_random version downgrade sentinels, we do not
> mention what is to be done when downgrading to TLS 1.0, which I thought was
> still a permitted version by this spec.
>
> It’s a little odd that we list in enum ExtensionType on page 35 a strict
> subset of the extensions enumerated in the table on the following pages.
>
> I’ll note that we define 256 private use ECDHE group code points but only
> four such FFDHE group code points.  Probably fine, but a bit surprising.
>
> Should we forbid duplicate entries in PreSharedKeyExtension.identities?
>
> Conversely, we might want to explicitly say that duplicate
> OIDFilter.certificate_extension_oid fields should be expected in
> OIDFilterExtensions, to enable the case where multiple values must be
> present.  Or is that supposed to work by concatenating(?) the multiple
> values’ DER encodings in the certificate_extension_values field?
>
> I’ll call out for Russ’s attention at the end of Section 4.4.3 where we
> say that “implementations MUST NOT combine external PSKs with
> certificate-based authentication.”  Is there any reason not to qualify that
> as some sort of “don’t’ do it until it’s defined”?
>
> Should Alert.level be Alert.legacy_level?
>
> The editors copy has already removed the reference to RFC 4507, which is
> obsoleted by RFC 5077 (and was not cited anywhere, anyway).
>
> Appendix B has a claim that “values listed as _RESERVED were used in
> previous versions of TLS and are listed here for completeness”, though that
> is not exactly true, e.g., for ContentType.invalid_RESERVED(0)
>
> Section C.3 notes that “Certificates should always be verified to ensure
> proper signing by a trusted Certificate Authority”, which does not use RFC
> 2119 language, but might be seen as in conflict with opportunistic
> encryption in some circumstances.  I don’t object to this text, but it
> seems worth mentioning.
>
> Page 113 still has the “[[NOTE: TLS 1.3 needs a new channel binding
> definition that has not yet been defined.]]”, which should not make it into
> the final spec!
>
> -Ben
>
> _______________________________________________
> TLS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tls
>

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 11 Apr 2017 13:27:41 -0500

> > Figure 4 also lacks the explanation for the ‘+’ symbol.)

I guess I didn't consult back with what I had put in this mail when
preparing my editorial-changes pull request :)

> I think I feel OK about this.

(For the gallery, there were some tweaks in this area in #936)

> I think this clear in the section on Supported Versions.

It looks like this changed a little via #936 as well; I'm fine with your
followup change there.

> I am tempted to just say "1.1 or 1.0". Thoughts?

That's probably fine; I expect there is some additional attack in there
where an attacker could force 1.0 if 1.1 would otherwise have been used,
but both of those are not in a great place right now, so we don't need
to try too hard to help them out.

> DER, not that there can be multiple copies

Okay.

> I'm actually going to move and modify this section per your issue #935.

Yeah, I missed the bits I called out in #935 when I was doing my WGLC
review, but the two are related and can be handled together.

> i think we went over this and decided not to.

There was a pull request from not-me, yes.  Though IIRC it only changed
the name locally when describing alerts, and was rejected on the grounds
that "we don't use the legacy_level version for this anywhere else in
the spec", which is a little bit of circular reasoning.  I'm okay with
leaving it as-is.

> I see you fixed this as well.

Well, maybe.  ContentType.invalid is the only one that I marked up in
red pen on my paper copy, but I can't certify that I compared the entire
list against the IANA registry.  I did look at the extensions registry
when preparing #936, though.

> I think the "Absent a specific..."

Yeah, I guess I snuck that fix into #936.  So much for keeping things
separate...

> > definition that has not yet been defined.]]”, which should not make it

It looks like that was just by removing the note.  Is a channel binding
spec for 1.3 still a needed work item, then?

-Ben

>
> On Mon, Mar 27, 2017 at 11:23 PM, Kaduk, Ben <[email-masked]
> <mailto:[email-masked]>> wrote:
>
>     On 3/13/17, 12:30, "Sean Turner" <[email-masked]
>     <mailto:[email-masked]>> wrote:
>
>     draft-ietf-tls-tls13-19, to run through March 27.  Please send
>     your reviews to the list as soon as possible so we can prepare for
>     any discussion of open issues at IETF 98 in Chicago.
>
>     As the price of running the WGLC right during the meeting lead-up,
>     my review comes in at the last minute.
>
>     Generally, it is in good shape.  I think I still owe some text
>     about what we aim for and expect to achieve with respect to side
>     channel resistance, though at this point it may be too late to get
>     that text in :(
>
>     The following is basically a laundry list of the minor issues; I
>     will send editorial notes under separate cover, probably as a pull
>     request.
>
>     Is that any TLS server, or just one that has negotiated and is
>     using TLS 1.3?
>
>     In the description of legacy_compression_methods on page 31, we
>     make restrictions on “every TLS 1.3 ClientHello”, but do not say
>     how such things are identified.  (Hmm, maybe we also do so
>     elsewhere in the document, too, now that I search for where)  we
>     explicitly define what a client “considered to be attempting to
>     negotiate using this specification (i.e., a TLS 1.3 ClientHEello)
>     on page 87, as supported_versions including 1.3.  Which, is maybe
>     not the most future-proof thing.
>
>     server should do when receiving a ClientHello that does not
>     contain a supported_versions extension.  (Also, I don’t think
>     When covering the server_random version downgrade sentinels, we do
>     not mention what is to be done when downgrading to TLS 1.0, which
>     I thought was still a permitted version by this spec.
>
>     I’ll note that we define 256 private use ECDHE group code points
>     but only four such FFDHE group code points.  Probably fine, but a
>     bit surprising.
>
>     OIDFilterExtensions, to enable the case where multiple values must
>     be present.  Or is that supposed to work by concatenating(?) the
>     multiple values’ DER encodings in the certificate_extension_values
>     field?
>
>     I’ll call out for Russ’s attention at the end of Section 4.4.3
>     where we say that “implementations MUST NOT combine external PSKs
>     with certificate-based authentication.”  Is there any reason not
>     to qualify that as some sort of “don’t’ do it until it’s defined”?
>
>     The editors copy has already removed the reference to RFC 4507,
>     which is obsoleted by RFC 5077 (and was not cited anywhere, anyway).
>
>     Appendix B has a claim that “values listed as _RESERVED were used
>     in previous versions of TLS and are listed here for completeness”,
>     though that is not exactly true, e.g., for
>     ContentType.invalid_RESERVED(0)
>
>     ensure proper signing by a trusted Certificate Authority”, which
>     does not use RFC 2119 language, but might be seen as in conflict
>     with opportunistic encryption in some circumstances.  I don’t
>     object to this text, but it seems worth mentioning.
>
>     Page 113 still has the “[[NOTE: TLS 1.3 needs a new channel
>     binding definition that has not yet been defined.]]”, which should
>     not make it into the final spec!
>
>     -Ben
>
>     [email-masked] <mailto:[email-masked]>
>     <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_tls&d=DwMFaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=sssDLkeEEBWNIXmTsdpw8TZ3tAJx-Job4p1unc7rOhM&m=zDb_mbfXWxowypc9E4E6zZZ_lXTab2DcV9qm--twWoM&s=WD0bv4QMm5OHI8RqolDFScW-e1jMk-YNzkVmbC4cmEw&e=>
>
>

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 11 Apr 2017 11:45:07 -0700

On Tue, Apr 11, 2017 at 11:27 AM, Benjamin Kaduk <[email-masked]> wrote:
> spec for 1.3 still a needed work item, then
>

Yeah, I think so.

-Ekr

>> The following is basically a laundry list of the minor issues; I will
>> send editorial notes under separate cover, probably as a pull request.
>>
>> Should Figure 4 (“message flow for a zero round trip handshake”) include
>> a “+ early_data” for the server’s flight?  (The legend for Figure 4 also
>> Do we want to add some commentary about the extant SHA1 collisions when
>> we say that {rsa_pkcs1,dsa,ecdsa}_sha1 are only SHOULD NOT?
>>