Subject: [TLS] Some feedback on draft 18

From: David Wong <[email-masked]>
Date: Wed, 14 Dec 2016 16:38:39 -0600

Hello! I read the current specification and I have some feedback on it. I
hope this can be useful. Sorry for the length or if I point to obvious
things. Here is a link as well for readability:
https://gist.github.com/mimoo/b6c64a133cd85da53f2f744dcdb39839

   1.

   *1-Byte Length of Vectors*: One edge-case that is implied and not
   discussed in the Presentation Language section
   <https://tlswg.github.io/tls13-spec/#presentation-language> is: "do I
   need to write the length of a vector<0...255>?". It seems logical that it
   will always be 1 byte and that it might be an implicit length. It would be
   nice to have this edge case mentioned or say something along the lines
   "there are no exceptions when encoding the length of variable-length
   vectors".
   2.

   *Lack of repetition*: The intro diagram
   <https://tlswg.github.io/tls13-spec/#rfc.section.2> tells you what
   messages are encrypted with what key. But after that, no reminders are
   present. For example, look at the Finished message. How do you know that
   this has to be encrypted with the handshake key? It is not written
in the Finished
   message <https://tlswg.github.io/tls13-spec/#rfc.section.4.4.3> section,
   it is not written in the authentication messages
   <https://tlswg.github.io/tls13-spec/#rfc.section.4.4> parent section and
   it is not written in the handshake protocol
   <https://tlswg.github.io/tls13-spec/#rfc.section.4> parent-parent
   section either. Only in the overview section.
   3.

   *How to define application data in an application data message*: It is
   not clear how "real" application data should appear in an application data
   message. Encrypted handshakes and alerts are clearly defined, but it takes
   some time to understand that "real" application data is written -- without
   structure or metadata -- into the content field of a TLSInnerPlaintext
   message. The current definition is "content: The cleartext of
   TLSPlaintext.fragment". I think a better one would be "content: the byte
   encoding of a handshake or alert message, or the raw bytes of the
   application's data to send".
   4.

   *Unencrypted Application Data*: Does it say somewhere in the
   specification that real application data should never appear in a
   TLSPlaintext message? At the moment the application_data content type is
   present in the declaration of a TLSPlaintext structure, see the record
   layer section <https://tlswg.github.io/tls13-spec/#record-layer>
   5.

   *What alert to use*: Sometimes it is not mentioned what level of alert
   should be used. For example: "Because TLS 1.3 forbids renegotiation, if a
   server receives a ClientHello at any other time, it MUST terminate the
   connection". I'm guessing it should be a fatal alert, but this is open to
   interpretation.
   6.

   *Why force the signature algorithm extension*: If the client does not
   send a signature algorithm extension, why would the server abort the
   handshake? It would make more sense to continue with the server's prefered
   certificate. Especially since currently, most webservers only possess one
   kind of certificate anyway.
   7.

   *Resumption_secret*: The Key Schedule
   <https://tlswg.github.io/tls13-spec/#rfc.section.7.1> section mentions a
   resumption_secret but nowhere in the spec does this keyword re-appear.
   It should be mentioned somewhere that this is the secret to be used as PSK
   in a future resumption handshake.
   8.

   *Auth_modes*: there is a single mention of "auth_modes" in the Pre-Shared
   Key Extension
   <https://tlswg.github.io/tls13-spec/#pre-shared-key-extension> section.
   No definition of this variable is to be found.
   9.

   *Computing Finished keys*: The section on how to compute Finished keys
   <https://tlswg.github.io/tls13-spec/#rfc.section.4.4.3> should be
in the cryptographic
   computation
   <https://tlswg.github.io/tls13-spec/#cryptographic-computations> section
   in my opinion. It should probably be added to the key schedule diagram as
   well.
   10.

   *Computation of the Binders*: From the relevant section
   <https://tlswg.github.io/tls13-spec/#psk-binder>: "*Each entry in the
   binders list is computed as an HMAC over the portion of the ClientHello up
   to and including the PreSharedKeyExtension.identities field. That is, it
   includes all of the ClientHello but not the binder list itself. The length
   fields for the message (including the overall length, the length of the
   extensions block, and the length of the “pre_shared_key” extension) are all
   set as if the binder were present*". There are several things that are
   unclear to me and I think they should be mentioned here: the fake binder
   should be of size hash.size, it should also say that the HMAC is
   computed over the handshake message containing the client hello, not only
   the client hello structure.
   11.

   *What binder key to use*: It is not clear which binder are to be used
   when doing a session resumption. The previous handshake one or the new
   handshake one? The confusion comes from the fact that the old handshake's
   resumption_secret/PSK is to be used in the new handshake, it could be
   thought that the old handshake's binder key should be the one used in
   the new handshake as well. I think it would help if this
   resumption_secret is called something like
   next_handshake.resumption_secret. Or make it clearer from which key
   schedule the binder key comes from.
   12.

   *PSK and Cipher Suite*: I think the following fact should be emphasized
   in more places: the client needs to advertise the single cipher suite they
   previously used when doing session resumption. It could be added to the
   cipher_suite definition of the client hello section
   <https://tlswg.github.io/tls13-spec/#client-hello> as well as in
the overview
   of session resumption
   <https://tlswg.github.io/tls13-spec/#resumption-and-psk>.
   13.

   *Implicit Lengths in The Presentation Language*: nested structures +
   implicit lengths = trouble. It's easy to forget parsing/writing all the
   different "sizes" when they are not clearly stated in the spec. Why not
   just having a length field in front of these variable-length vectors? It
   would add a bit of overhead but would make things clearer in my opinion.
   14.

   *No Help For The Implementer*: If you look at the Noise Protocol
   Framework <http://www.noiseprotocol.org/noise.html>, you can clearly see
   that the specifications were written with implementers in mind: several
   states, and objects are defined. For example, one way of implementing TLS
   is to have some sort of complex state that will remember what state you are
   in the handshake, what is the cryptographic context, etc... here the
   implementer has to decide everything for himself.
   15.

   *Whitelist Of State Transitions*: There is nothing about whitelisting
   state transitions. In my opinion there should be a list of allowed state
   transitions that the implementer could use. I made one here, I suspect
   it is still missing some states
   <https://gist.github.com/mimoo/779dcf8c44d80a2a34a1a2f2ed620711>.
   16.

   *How to parse extensions*: I think that IANA's consideration
   <https://tlswg.github.io/tls13-spec/#iana-considerations> section's
   table would be useful to have in the earlier Extension section
   <https://tlswg.github.io/tls13-spec/#extensions> (probably only the
   extensions included in the RFC should be listed). Another useful pragraph:
   how to parse these extensions, making sure that different messages come
   with their own whitelist of extensions.
   17.

   *Language <-> Bytes Translation*: There are no real introduction/no
   clear section about this. I understand that everything is in the
Presentation
   Language <https://tlswg.github.io/tls13-spec/#presentation-language>
   section but for someone who discovers the specification it takes some time
   to find out that this section is about both the presentation language and
   its byte encoding. A better introduction in this section as well as an
   emphasis on the byte representation in each of the subsections of this
   section would in my opinion benefit the whole document.
   18.

   *Asymmetrical MUSTs*: some "MUST" keywords are not symmetrical. What is
   the point of them? For example, the spec says that as a client, you MUST
   use a unique 0x0303 value in the legacy_version field of the client hello
   <https://tlswg.github.io/tls13-spec/#client-hello>. But if you do not,
   there are no problems from the server side in the specification and the
   client hello will still be well received. (Is this maybe for forward
   compatibility?)

And finally, here are some minor nitpicks:

   1.

   *PSK extension*: *debatable*, but I would rather have the PSK section
   <https://tlswg.github.io/tls13-spec/#pre-shared-key-extension> at the
   very end of the Extensions section
   <https://tlswg.github.io/tls13-spec/#extensions> as it MUST be the final
   extension of a client hello.
   2.

   *No Indication Of Encryption*: This is *debatable*, but I find it
   inelegant that there are no indication in a record that it is encrypted.
   The implementation is supposed to keep track of it.
   3.

   *What PSK mode does a server support*: I think the server should
   indicate what mode (psk_dh_ke or psk_ke) it supports in a
   server_hello.psk_mode extension or in the new_session_ticket message.
   Like that the client would know if it has to send a key_share or not in
   the resumption. Right now, the only indication of a psk_dh_ke is that
   the server will send a key_share even when presented with a PSK. *this
   is debatable*.
   4.

   *The Key Derivation*: The key derivation section is the least clear
   section in this specification. The diagrams kind of help, but are limited
   by the RFC format... I have nothing else to say here, the whole section is
   confusing to say the least. No solution offered :o)

---

From: Eric Rescorla <[email-masked]>
Date: Wed, 14 Dec 2016 16:17:44 -0700

Hi David,

Thanks for taking the time to read this. Some responses below.

> 1-Byte Length of Vectors: One edge-case that is implied and not
> discussed in the Presentation Language section is: "do I need to write
> the length of a vector<0...255>?". It seems logical that it will
> always be 1 byte and that it might be an implicit length. It would be

I'm not sure I understand your point here about why this is an edge
case. In order for the encoding to be unambiguous, every variable
length vectory must always be prefixed by the length, even if in
a particular message it is zero length. This section seems pretty
clear to me. Note that unconditional statement about encoding
followed by the last sentence about empty vectors.

   Variable-length vectors are defined by specifying a subrange of legal
   lengths, inclusively, using the notation <floor..ceiling>.  When
   these are encoded, the actual length precedes the vector's contents
   in the byte stream.  The length will be in the form of a number
   consuming as many bytes as required to hold the vector's specified
   maximum (ceiling) length.  A variable-length vector with an actual
   length field of zero is referred to as an empty vector.

Happy to look at making this clearer, but I'd rather not just
add a clarifying sentence.

> Lack of repetition: The intro diagram tells you what messages are
> encrypted with what key. But after that, no reminders are present. For
> example, look at the Finished message. How do you know that this has
> to be encrypted with the handshake key? It is not written in the
> Finished message section, it is not written in the authentication
> messages parent section and it is not written in the handshake
> protocol parent-parent section either. Only in the overview section.

I'll see what I can do about this. With that said, I'm trying to remove
unnecessary duplication, so it's a compromise.

> How to define application data in an application data message: It is
> not clear how "real" application data should appear in an application
> data message. Encrypted handshakes and alerts are clearly defined, but
> it takes some time to understand that "real" application data is
> written -- without structure or metadata -- into the content field of
> a TLSInnerPlaintext message. The current definition is "content: The
> cleartext of TLSPlaintext.fragment". I think a better one would be
> "content: the byte encoding of a handshake or alert message, or the
> raw bytes of the application's data to send".

Yeah, this seems like a good change. Feel free to send a PR or I can just
do it.

> Unencrypted Application Data: Does it say somewhere in the
> TLSPlaintext message? At the moment the application_data content type
> is present in the declaration of a TLSPlaintext structure, see the
> record layer section

I'm not sure I understand what you're saying here.

> What alert to use: Sometimes it is not mentioned what level of alert
> should be used. For example: "Because TLS 1.3 forbids renegotiation,
> if a server receives a ClientHello at any other time, it MUST
> terminate the connection". I'm guessing it should be a fatal alert,
> but this is open to interpretation.

Hmm... I think this is pretty clear in 6.2.

   Whenever an implementation encounters a fatal error condition, it
   SHOULD send an appropriate fatal alert and MUST close the connection
   without sending or receiving any additional data.  In the rest of
   this specification, the phrase "{terminate the connection, abort the
   handshake}" is used without a specific alert means that the
   implementation SHOULD send the alert indicated by the descriptions

> Why force the signature algorithm extension: If the client does not
> handshake? It would make more sense to continue with the server's
> prefered certificate. Especially since currently, most webservers only
> possess one kind of certificate anyway.

The signature algorithms extension also indicates what to sign with,
and that's not completely determined by the certificate (e.g., with
RSA).

> Resumption_secret: The Key Schedule section mentions a
> resumption_secret but nowhere in the spec does this keyword
> re-appear. It should be mentioned somewhere that this is the secret to
> be used as PSK in a future resumption handshake.

Good catch. This is actually called the resumption master
secret. I'll clean this up.

> Auth_modes: there is a single mention of "auth_modes" in the
> Pre-Shared Key Extension section. No definition of this variable is to
> be found.

Yeah, this is a holdover from when this field exists. I fixed that.

> Computing Finished keys: The section on how to compute Finished keys
> should be in the cryptographic computation section in my opinion. It
> should probably be added to the key schedule diagram as well.

This made the key schedule too hard to read (I note you complain
about that below) so I'm reluctant to add it. I'd be interested
in other people's opinions about whether these computations would
be better in this section.

> Computation of the Binders: From the relevant section: "Each entry in
> the binders list is computed as an HMAC over the portion of the
> ClientHello up to and including the PreSharedKeyExtension.identities
> field. That is, it includes all of the ClientHello but not the binder
> list itself. The length fields for the message (including the overall
> length, the length of the extensions block, and the length of the
> “pre_shared_key” extension) are all set as if the binder were
> present". There are several things that are unclear to me and I think
> they should be mentioned here: the fake binder should be of size
> hash.size, it should also say that the HMAC is computed over the
> handshake message containing the client hello, not only the client
> hello structure.

I'll see what I can do here.

> What binder key to use: It is not clear which binder are to be used
> handshake one? The confusion comes from the fact that the old
> handshake's resumption_secret/PSK is to be used in the new handshake,
> it could be thought that the old handshake's binder key should be the
> one used in the new handshake as well. I think it would help if this

I'm not quite following. It's not possible to compute the RMS
at this point in the handshake. The only PSK you could possibly
have is the one from the previous handshake.

> PSK and Cipher Suite: I think the following fact should be emphasized
> in more places: the client needs to advertise the single cipher suite
> they previously used when doing session resumption. It could be added
> to the cipher_suite definition of the client hello section as well as
> in the overview of session resumption.

Thanks. I can add something.

> Implicit Lengths in The Presentation Language: nested structures +
> implicit lengths = trouble. It's easy to forget parsing/writing all
> the different "sizes" when they are not clearly stated in the
> spec. Why not just having a length field in front of these
> variable-length vectors? It would add a bit of overhead but would make
> things clearer in my opinion.

Can you point to the specific structures you are concerned with
here? I think in almost every case we do have length fields. Actually,
a common complaint is we have too many redundant length fields.

> No Help For The Implementer: If you look at the Noise Protocol
> Framework, you can clearly see that the specifications were written
> with implementers in mind: several states, and objects are
> defined. For example, one way of implementing TLS is to have some sort
> of complex state that will remember what state you are in the
> handshake, what is the cryptographic context, etc... here the

Most IETF specs leave the choice of how to do things up
to the implementor. As someone who has implemented a lot of
specs, I actually find it pretty frustrating when the author
gets all prescriptive about implementation. If there are
points that are unclear, I think it's fine to make them clear,
but I'd rather stick to the PDUs.

> Whitelist Of State Transitions: There is nothing about whitelisting
> state transitions. In my opinion there should be a list of allowed
> state transitions that the implementer could use. I made one here, I
> suspect it is still missing some states.

I'm going to add a state diagram in -19. This link seems like
useful input. Thanks.

> How to parse extensions: I think that IANA's consideration section's
> (probably only the extensions included in the RFC should be
> listed). Another useful pragraph: how to parse these extensions,
> making sure that different messages come with their own whitelist of
> extensions.

Good point. I'll make this change.

> Language <-> Bytes Translation: There are no real introduction/no
> Presentation Language section but for someone who discovers the
> specification it takes some time to find out that this section is
> about both the presentation language and its byte encoding. A better
> introduction in this section as well as an emphasis on the byte
> representation in each of the subsections of this section would in my
> opinion benefit the whole document.

I'm not quite sure I understand what you're looking for here. Can
you send a PR?

> Asymmetrical MUSTs: some "MUST" keywords are not symmetrical. What is
> the point of them? For example, the spec says that as a client, you
> MUST use a unique 0x0303 value in the legacy_version field of the
> client hello. But if you do not, there are no problems from the server
> side in the specification and the client hello will still be well
> received. (Is this maybe for forward compatibility?)

Amongst other things. MUST do X but must allow X and Y is pretty
common in IETF spec.

> PSK extension: debatable, but I would rather have the PSK section at
> the very end of the Extensions section as it MUST be the final

Yeah, I can do that.

> No Indication Of Encryption: This is debatable, but I find it
> inelegant that there are no indication in a record that it is
> encrypted. The implementation is supposed to keep track of it.

Actually, at present everything with first byte == application_data
is encrypted and everything else is not.

> What PSK mode does a server support: I think the server should
> server_hello.psk_mode extension or in the new_session_ticket
> message. Like that the client would know if it has to send a key_share
> or not in the resumption. Right now, the only indication of a
> psk_dh_ke is that the server will send a key_share even when presented
> with a PSK. this is debatable.

This was a pretty explicit decision to avoid redundancy, so I think
we have WG consensus here.

> The Key Derivation: The key derivation section is the least clear
> section in this specification. The diagrams kind of help, but are
> limited by the RFC format... I have nothing else to say here, the
> whole section is confusing to say the least. No solution offered :o)

Yeah, not too sure what to do.

Thanks,
-Ekr

On Wed, Dec 14, 2016 at 3:38 PM, David Wong <[email-masked]>
wrote:

>    this has to be encrypted with the handshake key? It is not written in the Finished
>    message <https://tlswg.github.io/tls13-spec/#rfc.section.4.4.3>
>    section, it is not written in the authentication messages
>    <https://tlswg.github.io/tls13-spec/#rfc.section.4.4> parent section
>    and it is not written in the handshake protocol
>    TLSPlaintext message? At the moment the application_data content type
>    is present in the declaration of a TLSPlaintext structure, see the
>    record layer section <https://tlswg.github.io/tls13-spec/#record-layer>
>    5.
>
>    <https://tlswg.github.io/tls13-spec/#rfc.section.7.1> section mentions
>    a resumption_secret but nowhere in the spec does this keyword
>    re-appear. It should be mentioned somewhere that this is the secret to be
>    used as PSK in a future resumption handshake.
>    8.
>
>    <https://tlswg.github.io/tls13-spec/#pre-shared-key-extension>
>    section. No definition of this variable is to be found.
>    9.
>
>    <https://tlswg.github.io/tls13-spec/#rfc.section.4.4.3> should be in
>    the cryptographic computation
>    <https://tlswg.github.io/tls13-spec/#cryptographic-computations>
>    section in my opinion. It should probably be added to the key schedule
>    diagram as well.
>    10.
>
>    resumption_secret is called something like next_handshake.resumption_
>    secret. Or make it clearer from which key schedule the binder key
>    comes from.
>    12.
>
>    *PSK and Cipher Suite*: I think the following fact should be
>    emphasized in more places: the client needs to advertise the single cipher
>    suite they previously used when doing session resumption. It could be added
>    to the cipher_suite definition of the client hello section
>    <https://tlswg.github.io/tls13-spec/#client-hello> as well as in the overview
>    of session resumption
>    implicit lengths = trouble. It's easy to forget parsing/writing all
>    the different "sizes" when they are not clearly stated in the spec. Why not
>    Framework <http://www.noiseprotocol.org/noise.html>, you can clearly
>    see that the specifications were written with implementers in mind: several
>    clear section about this. I understand that everything is in the Presentation
>    *Asymmetrical MUSTs*: some "MUST" keywords are not symmetrical. What
>    is the point of them? For example, the spec says that as a client, you MUST
>    use a unique 0x0303 value in the legacy_version field of the client
>    hello <https://tlswg.github.io/tls13-spec/#client-hello>. But if you
>    do not, there are no problems from the server side in the specification and
>    the client hello will still be well received. (Is this maybe for forward
>    compatibility?)
>
>    <https://tlswg.github.io/tls13-spec/#extensions> as it MUST be the
>    final extension of a client hello.
>    2.
>
>    Like that the client would know if it has to send a key_share or not
>    in the resumption. Right now, the only indication of a psk_dh_ke is
>    that the server will send a key_share even when presented with a PSK. *this
>    is debatable*.
>    4.
>
> _______________________________________________
> TLS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tls
>
>