Subject: [TLS] Malformed Finished handling

From: Hubert Kario <[email-masked]>
Date: Wed, 04 Jul 2018 11:54:17 +0200

Despite this, is it correct to terminate a connection with "illegal_parameter" 
upon receiving a Finished handshake message with a 100 byte payload? or a 20 
byte payload? My opinion is that it is not, "decode_error" is more specific so 
it should be used instead.

Specification says the following on the matter:

The draft 28 specifies the Finished message as having following structure:

      struct {
          opaque verify_data[Hash.length];
      } Finished;

At multiple places it also talks about handling messages that have sizes that 
don't match their structure as requiring termination of connection with 
"decode_error".

The generic situation in Section 6: 

   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 "decode_error" alert.

as description of the alert in Section 6.2:

   decode_error  A message could not be decoded because some field was
      out of the specified range or the length of the message was
      incorrect.  This alert is used for errors where the message does
      not conform to the formal protocol syntax.

In specific about Client Hello, in Section 4.1.2:

   If negotiating a version of TLS prior to 1.3, 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.

And specific handling of Certificate from server in Section 4.4.2.4:

   If the server supplies an empty Certificate message, the client MUST
   abort the handshake with a "decode_error" alert.

Description of "illegal_parameter" in Section 6:

   Peers which receive a message which is syntactically correct but
   semantically invalid (e.g., a DHE share of p - 1, or an invalid enum)
   MUST terminate the connection with an "illegal_parameter" alert.

Alert description in Section 6.2:

   illegal_parameter  A field in the handshake was incorrect or
      inconsistent with other fields.  This alert is used for errors
      which conform to the formal protocol syntax but are otherwise
      incorrect.

(it's also mentioned in over a dozen places in the draft)
-- 
Regards,
Hubert Kario
Senior Quality Engineer, QE BaseOS Security team
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 115, 612 00  Brno, Czech Republic

---

From: Eric Rescorla <[email-masked]>
Date: Wed, 4 Jul 2018 06:00:18 -0700

I think it's a close call, because the length is sort of external to the
language. That's why, for instance, NSS sends "illegal_parameter". So,
absent specific text about this value, I think this is something we can
leave to the implementations.

-Ekr

On Wed, Jul 4, 2018 at 2:54 AM, Hubert Kario <[email-masked]> wrote:

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

---

From: Hubert Kario <[email-masked]>
Date: Wed, 04 Jul 2018 15:36:19 +0200

On Wednesday, 4 July 2018 15:00:18 CEST Eric Rescorla wrote:
> language.

which language? the decode_error alert description literally says "length of 
the message was incorrect."

> That's why, for instance, NSS sends "illegal_parameter". So,

but the text is explicit, if a message continues past message boundary, the 
correct response is decode_error. Decode_error is also consistent with the way 
above-expected length Client Hello MUST be handled.

Just because the description of the message uses a single opaque array doesn't 
make this message syntactically any different from other Handshake protocol 
message.

if the interpretation of "I know this _message_ _length_ is wrong because of 
some other values I negotiated before, so I'll send illegal_parameter" was 
correct, then overflow_error, decrypt_error and probably few others would also 
need to be replaced with illegal_parameter...

---

From: "Salz, Rich" <[email-masked]>
Date: Wed, 4 Jul 2018 13:46:04 +0000

I think the rigorousness of error codes is not at the same level as the rest of the document.  I'm fine with that.  I can understand why people developing test suites are frustrated. To me, it's like people arguing about whether the "list of errno values" in a syscall manpage must be completely comprehensive or not.  Old-timers might remember when djb used to rail against the fact that close(2) could set errno to ENOSPACE when using AFS.

The WG spans a wide variety of views on errors and alerts. Some want to have only one, "bad packet," alert. Some want to add a textual explanation option.  Woolgathering aside, I think the first sentence I wrote is 100% accurate.

---

From: Eric Rescorla <[email-masked]>
Date: Wed, 4 Jul 2018 09:54:10 -0700

> the message was incorrect."

The protocol definition language.

-Ekr

> > > upon receiving a Finished handshake message with a 100 byte payload?
> or a
> > > 20
> > > At multiple places it also talks about handling messages that have
> sizes
> > > that

---

From: Martin Thomson <[email-masked]>
Date: Thu, 5 Jul 2018 10:06:45 +1000

> it should be used instead.

When there are multiple problems with a message, why do we need to
accept just one of the possible alerts?  That assumes either a very
particular order of processing, or a strict precedence order for
errors.  Like Rich says, there is a degree of imprecision in our error
reporting, but - for me - that's OK.

---

From: Hubert Kario <[email-masked]>
Date: Mon, 09 Jul 2018 13:38:43 +0200

On Thursday, 5 July 2018 02:06:45 CEST Martin Thomson wrote:
> > Despite this, is it correct to terminate a connection with
> > "illegal_parameter" upon receiving a Finished handshake message with a
> > 100 byte payload? or a 20 byte payload? My opinion is that it is not,
> > "decode_error" is more specific so it should be used instead.
> 

but there are no multiple problems with the message in question

no part of the TLS standards states that the implementation should check if 
the length of the handshake message lies within expected bounds

but there *is* a section that explicitly calls out that the implementation 
needs to verify if the handshake message matches the header, *after* parsing

---

From: Hubert Kario <[email-masked]>
Date: Mon, 09 Jul 2018 13:29:28 +0200

> The protocol definition language.

Sorry, I really don't see it. There are multiple references to "length" in the 
draft, and as far as I can tell, all are quite consistent.
 
> > > > "illegal_parameter"
> > > > The draft 28 specifies the Finished message as having following
> > 
> > structure:
> > > >       struct {
> > > >       
> > > >    semantically invalid (e.g., a DHE share of p - 1, or an invalid
> > 
> > enum)
> >

---

From: Hubert Kario <[email-masked]>
Date: Mon, 09 Jul 2018 13:27:23 +0200

On Wednesday, 4 July 2018 15:46:04 CEST Salz, Rich wrote:
> >    some other values I negotiated before, so I'll send illegal_parameter"
> >    was  correct, then overflow_error, decrypt_error and probably few
> >    others
> >    would also need to be replaced with illegal_parameter...
>   
> I think the rigorousness of error codes is not at the same level as the rest
> of the document.  I'm fine with that.  I can understand why people
> developing test suites are frustrated. 

It's not about developing test suites. It's about software the test suites are 
running against.

Inconsistent behaviour is usually a sign of a bug (I already had to deal with 
two security bugs, including one remote crash, that were/could have been found 
by verifying the sending of alerts and description of alerts), if not a 
security issue by itself (see ROBOT).

To reiterate: I'm not doing it because it brings me perverse pleasure or 
because I'm obsessive about details or because it makes writing test suites 
easier (it doesn't, that's why first tests I was writing didn't do that), I 
insist on specific, explicit and correct error handling
_because it finds bugs_ 

And I think we can all agree that a secure implementation of TLS needs not to 
have bugs. And we all primarily want to see such implementations, don't we?