[openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)

[Alessandro Ghedini via RT]

On Thu, Oct 08, 2015 at 07:57:21pm +0000, Alessandro Ghedini via RT wrote:
> On Thu, Oct 08, 2015 at 06:26:27pm +0000, Alessandro Ghedini via RT wrote:
> > On Thu, Oct 08, 2015 at 06:14:00pm +0000, Alessandro Ghedini via RT wrote:
> > > On Thu, Oct 08, 2015 at 05:19:06pm +0000, Alessandro Ghedini via RT wrote:
> > > > On Thu, Oct 08, 2015 at 04:12:50pm +0000, Hubert Kario via RT wrote:
> > > > > The server does not abort connection upon receiving a Client Hello 
> > > > > message with malformed session_id field.
> > > > > 
> > > > > Affects 1.0.1, 1.0.2 and master.
> > > > > 
> > > > > In SSLv3 and all versions of TLS (e.g. RFC 5246), the SessionID is 
> > > > > defined as
> > > > > 
> > > > >       opaque SessionID<0..32>;
> > > > > 
> > > > > that means, that any SessionID longer than 32 bytes creates an 
> > > > > incorrectly formatted Client Hello message, and as such, should be 
> > > > > rejected.
> > > > 
> > > > Looking at the code in master, for non-v2 ClientHello messages the code uses
> > > > the PACKET_get_length_prefixed_1() function to extract the SessionID, however I
> > > > see no way to pass a maximum allowed length to it. I think a new function would
> > > > have to be added to the PACKET_* interface (I can look into this). Haven't
> > > > checked older branches yet.
> > > 
> > > So, it turns out the check was already performed, but this failure didn't cause
> > > the session to be aborted (the original PACKET was advanced anyway though, even
> > > is the session_id isn't actually extracted), I don't know if this was on
> > > purpose though.
> > 
> > Another thing to consider is that the client already aborts when an invalid
> > session_id is received in the ServerHello and sends the ILLEGAL_PARAMETER alert.
> > 
> > My patch doesn't actually send any alert so the check should be moved earlier
> > to allow an alert to be sent, if that is desired.
> 
> FYI, I just pushed another patch that does the above (moving the check and
> sending an alert) which I think is the best option (although, as Hubert pointed
> out, sending the decode_error alert is not a MUST). If that's ok with you, I
> can squash the two commits together and give them a better message, otherwise
> just ignore the second patch.

So, I went ahead and squashed all the commits into one [0] and also added the
SSLv2 check as well. Can someone please have a look?

Anyway, I noticed that the client compares the session_id length against
"sizeof s->session->session_id" (which is SSL_MAX_SSL_SESSION_ID_LENGTH, like I
used in my patch), and also against SSL3_SESSION_ID_SIZE (which is equal to
SSL_MAX_SSL_SESSION_ID_LENGTH). I think this second check is superfluous and
the other one can just use SSL_MAX_SSL_SESSION_ID_LENGTH directly instead of
sizeof (it'd be more obvious).

Cheers

[0] https://github.com/openssl/openssl/pull/437