Re: [TLS] PR#625: Change alert requirements

[Hubert Kario <[e-mail]>]

On Monday, 5 September 2016 11:02:57 CEST Eric Rescorla wrote:
> PR: https://github.com/tlswg/tls13-spec/pull/625

Finally found some time to take a look at it.

So in general I like the change to "abort the handshake with a <type> alert", 
the "send a fatal <type> alert and close the connection" was a bit clunky. I 
was actually planning to do something like this myself.

Few other items carried over from my proposed PR[1]:

---

I still think we should include generic advice for handling parser errors in 
the parser (Presentation Language/Vectors and Constructed Types) section; 
something like this:

   Peers that receive a encoding of a vector in a message with a length that 
   does not match specification MUST abort the connection with a 
   "decode_error" alert unless more specific section states otherwise.

and this:

   Peers that receive messages that doesn't match the expected constructed 
   type in expected message MUST abort the connection with a "decode_error" 
   alert unless more specific section states otherwise.

---

I think it's a bit confusing to have this sentence:

   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.

in the Server Hello section. Everything before the very last "and" is more 
applicable to ClientHello.

--- 

I think that this sentence in Handshake Protocol:

   Sending handshake messages in an unexpected order results in an
   "unexpected_message" fatal error.

should use a MUST and be prescribed to the receiving side, for example:

   Peer that receives handshake messages in unexpected order MUST abort the
   handshake with an "unexpected_message" alert.

---

Description what should client do when the ServerHello.cipher_suite is the one 
it did not advertise in ClientHello.cipher_suites is missing.

So is description of how to handle ServerHello.version being higher than 
ClientHello.max_supported_version.

At least, I don't see it in Server Hello section.

---

missing_extension description in Alert Protocol probably should be extended 
from:

   Sent by endpoints that receive a hello message not containing an extension 
   that is mandatory to send for the offered TLS version.

to:

   Sent by endpoints that receive a hello message not containing an extension 
   that is mandatory to send for the offered TLS version or the offered key 
   exchange mechanisms.

(thinking of handling situations where "key_share" or "pre_shared_key" is 
missing)

---

Hello Extensions section doesn't specify how to handle messages with duplicate 
extensions (e.g. second "key_share" extension added in the second ClientHello 
message in connection)

---

Signature Algorithms doesn't say how to handle RSA-SSA signatures with salt 
length that doesn't match hash size.

---

Key Share doesn't say how to handle server_share that doesn't match 
client_shares.

---

Finished section doesn't describe what to do if the contents don't match the 
expected value.

Should it be illegal_parameter or bad_record_mac is more appropriate?

---

Record Layer doesn't describe what to do if a record with zero-length payload 
and handshake or alert type is received.

---

Record Layer includes

   legacy_record_version : (...) This field is deprecated and MUST be ignored
   for all purposes.

Record Layer Protection does not.

---

1 - https://github.com/tlswg/tls13-spec/pull/621
-- 
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