Subject: [TLS] Draft 18 review : Minor remarks and typos

From: Olivier Levillain <[email-masked]>
Date: Tue, 22 Nov 2016 20:09:06 +0100

Hi list,

I am sorry for the very late answer concerning draft 18, but we
(ANSSI) have several remarks after proof-reading the current
specification.

We are sorry for the multiple long messages.

If the WG is interested by some of our concerns/proposals, we would be
glad to propose some PRs.

= Minor remarks =

 - P.14 (section 2): "Everything after this phase is encrypted" =>
   "is protected" (since we also have integrity protection).

 - P.14 (section 2) and P.20 (section 2.3): the psk_key_exchange_modes
   extension is called "pre_shared_key_modes".

 - P.21 (section 2.3): Add a reference to the relevant section for
   "early exporter secret"?

 - P.41 and 42 (4.2.5): The fact that clients can send an empty
   client_shares vector is repeated 30 lines apart.  Is it really
   needed?

 - P.43 (section 4.2.5.2): during the description of ECDHE parameters
   with secp*,  the paragraph
     Although X9.62 supports multiple point formats, any given curve MUST
     specify only a single point format.  All curves currently specified
     in this document MUST only be used with the uncompressed point format
     (the format for all ECDH functions is considered uncompressed).
   seems to imply that all ECDHE points are uncompressed, which is not
   the case.  I would replace "All curves currently specified in this
   document" with "secp256r1, secp384r1 and secp521r1 curves".
   Similarly, I would remove the parenthesis.

 - P.44 (4.2.6): Unless I am mistaken, the identity field in the
   extension is the "ticket" (when using the older terminology).
   However, the term ticket is used in different places afterwards,
   without a proper definition.  It might make sense to add a word in
   the identities definition.

 - P.44 (4.2.6): Similarly, it would be clearer to say "externally
   established PSKs" (as in P.45) instead of "external tickets".

 - P.44 (4.2.6) Similarly, instead of saying that a binder must be
   given "for each PSK offered", it might be clearer to say "for each
   PSK identity offered".  (Sorry for th nitpick, but I believe it is
   important to be crystal clear in such a document).

 - P.46 (4.2.7): "Servers MUST NOT select a key exchange mode that is
   not listed by the client".  As I understand it, the server
   advertises its "selection" by adding the keyshare extension or not.
   It might be useful to add this precision if I am not mistaken.

 - P.48 (4.2.8): "the server MUST have accepted a PSK ciphersuite" =>
   since PSK ciphersuites do not exist anymore, should not it be "have
   accepted to use a PSK key exchange mode" or "have answered with a
   psk extension"?

 - P.53 (4.4): The Certificate message definition contains the
   following text: "Note that certificate-based client authentication
   is not available in the 0-RTT case." It might make more sense to
   move this text elsewhere in the section, and to explain it further:
   does it mean the 1-RTT following the early data must not contain a
   client authentication?

 - P.53 (4.4): What are "the pure 1-RTT cases"?

 - P.54 (4.4.1): Since the specification does not force the
   certificates to be ordered in the Certificate message anymore, what
   is the point of adding a SHOULD about the order?  On the other
   hand, the text might include the requirement that all intermediate
   AC are to be provided in the Certificate message.

 - P. 55 (4.4.1.1): Since it is now possible to add per-certificate
   extension, I believe status_request_v2 does not make any sense now.
   Should not it be clearer to deprecate it with TLS 1.3 and to force
   multiple OCSP stapling to be made exclusively with multiple
   status_request extensions (one for each cert)?

 - P.56 (4.4.1.2): The text "As servers MAY require the presence of
   the "server_name" extension, clients SHOULD send this extension,
   when applicable." should be moved elsewhere (e.g. when discussing
   ClientHello).

 - It is not clear whether a client can authenticate with a
   certificate when the server has chosen PSK authentication. P.49
   (4.3.2) states that "A server which is authenticating with a
   certificate can optionally request a certificate from the client."
   whereas 4.4.2 (P.59) states that "When used with
   non-certificate-based handshakes (e.g., PSK), the client's
   signature [in CertificateVerify] does not cover the server's
   certificate directly."  How should this be interpreted?

 - P.60 (4.4), it is said that "The key used to compute the finished
   message is computed from the Base key defined in Section 4.4 using
   HKDF", but we are *in* section 4.4, so there must be a mistake?

 - P.61 (4.5.1), it is said that "the resumption master secret depends
   on the client's second flight" but this is only true when the
   client authenticates itself with a certificate.  This point is
   related to a point sent by Ben Kaduk
   (https://www.ietf.org/mail-archive/web/tls/current/msg21829.html):
   "In section 4.5.1 we note that when client auth is not used,
   servers MAY compute the remainder of the client-sent messages for
   the transcript so as to issue a NewSessionTicket immediately after
   the server Finished.  Do we want to say anything about why a server
   might wish to do so?"
   (https://www.ietf.org/mail-archive/web/tls/current/msg21829.html).
   to which Ekr answered with "I think I would rather not."

 - P.63 (4.5.3): The definition of the "request_update" field is
   unclear. It is said to "indicate that the recipient of [...] should
   respond".  Should not it say "Indicates *whether* the recipient"

 - P.65 (5.1): "Endpoints supporting other versions" => "earlier"?

 - P.66 (5.2): the content type if the TLS InnerPlaintext must be non
   null for the padding to work.  This is correctly stated in the end
   of the specification where 0 is invalid_RESERVED, but it might be
   useful to add a statement in the type field definition: "Valid
   content types all have non-null values."

 - P.67 (5.2): The exact same text is repeated twice in the page: "An
   endpoint that receives a record that exceeds this length MUST
   terminate the connection with a "record_overflow" alert."

 - P.81 (8.1): RSA signature schemes are specified to be MTI for
   certificates and optionally for CertificateVerify.  The precision
   should also be added for ecdsa_secp256r1-sha256.

 - P.83 and 85 (8.2): "Client" is supposed to mean that "the server
   shall not send them".  Yet, the cookie extension is marked as
   Clear, and can be sent in the HRR message.

 - P.84 (8.2): Since it is said afterwards that truncated_hmac is a
   dangerous extension, it might be logical to mark it as *not*
   recommended?

 - P.84 and 85: I suppose that the order of the extensions in this
   table is given by the extension id.  Yet it might be more practical
   to sort the table by extension name?

 - P.105 (B.4): "Do you verify that the RSA padding doesn't have
   additional data after the hash value?  [FI06]" => the attack was
   about PKCS#1 v1.5, not PSS.  Does it still apply, or should the
   text be amended?

 - P.105 (B.4): I believe the part about ECDSA "k" parameter states
   opposing statements, since deterministic ECDSA leads to a
   non-random k:
      Do you use a strong and, most importantly, properly seeded random
      number generator (see Appendix B.2) when generating Diffie-Hellman
      private values, the ECDSA "k" parameter, and other security-
      critical values?  It is RECOMMENDED that implementations implement
      "deterministic ECDSA" as specified in [RFC6979].
   Would not it be more logical to say: "Alternatively, it is
   RECOMMENDED..."

 - P.113 (D.2): There are missing references concerning the analysis
   of the TLS record layer

= Typos =

 - P.28 (section 4.1.1): a quote is missing after '"psk_key_exchange_modes'
 - P.46 (4.2.7): "A clients MUST provide" => "client"
 - P.54 (4.4.1): "If an extension applies the the entire chain" => "to the"
 - P.58 (4.4.2): "A single 0 byte which servers as the separator" => "serves"
 - P.63 (4.5.3): "If the request_udate" => request_update
 - P.86 (8.2): "and the TLS SignatureAlgorithm Registry to list values 4-233 as "Reserved"" => It should be 223 (as 224-255 are reserved for private use)

Olivier Levillain