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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
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.

Reproducer:
openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt -
nodes -batch
openssl s_server -key localhost.key -cert localhost.crt

In different console:
pip install --pre tlslite-ng
git clone https://github.com/tomato42/tlsfuzzer.git
cd tlsfuzzer
PYTHONPATH=. python scripts/test-invalid-session-id.py
--
Regards,
Hubert Kario
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

_______________________________________________
openssl-bugs-mod mailing list
[hidden email]
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

signature.asc (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
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.

The problem most likely happens with SSLv2 backwards compatible ClientHello as
well, but that seems to be easier to fix... or maybe it's time to just drop
that compatibility code for v1.1?

Cheers


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
On Thursday 08 October 2015 17:19:06 Alessandro Ghedini via RT wrote:
> The problem most likely happens with SSLv2 backwards compatible
> ClientHello as well, but that seems to be easier to fix... or maybe
> it's time to just drop that compatibility code for v1.1?

There is quite a bit of clients that do send SSLv2 backwards compatible
Client Hello, dropping it completely, even though it allows to
relatively safely negotiate TLS connections, is probably going one step
too far.

I don't plan to work on SSLv2 Client Hello fuzzing in near future
though.
--
Regards,
Hubert Kario
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

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

signature.asc (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Viktor Dukhovni
In reply to this post by Rich Salz via RT
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.

Can be rejected, and perhaps even should be rejected, but I don't
see a MUST here.  It seems there's little harm in tolerating longer
session ids (which never match, so are effectively ignored).

So yes, I support adding a check for this (likely above the PACKET
layer), but I don't think this has much urgency and likely should
not be back-ported to stable releases.

--
        Viktor.
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Kurt Roeckx
In reply to this post by Rich Salz via RT
On Thu, Oct 08, 2015 at 05:19:06PM +0000, Alessandro Ghedini via RT wrote:
> The problem most likely happens with SSLv2 backwards compatible ClientHello as
> well, but that seems to be easier to fix... or maybe it's time to just drop
> that compatibility code for v1.1?

I would love to have dropped that too, but 0.9.8 still sends such
client hello.  I think we're stuck with having to support that for
a while longer.


Kurt

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
On Thu, Oct 08, 2015 at 05:19:06PM +0000, Alessandro Ghedini via RT wrote:
> The problem most likely happens with SSLv2 backwards compatible ClientHello as
> well, but that seems to be easier to fix... or maybe it's time to just drop
> that compatibility code for v1.1?

I would love to have dropped that too, but 0.9.8 still sends such
client hello.  I think we're stuck with having to support that for
a while longer.


Kurt


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
In reply to this post by Rich Salz via RT
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.

In any case I wrote a minimal patch that makes the tlsfuzzer test pass (it may
even work for SSLv2 as well):
https://github.com/openssl/openssl/pull/437

Cheers


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
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.

Cheers


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Hubert Kario
In reply to this post by Viktor Dukhovni
On Thursday 08 October 2015 17:37:12 Viktor Dukhovni 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.
>
> Can be rejected, and perhaps even should be rejected, but I don't
> see a MUST here.  It seems there's little harm in tolerating longer
> session ids (which never match, so are effectively ignored).
RFC 5246:
   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
      message is always fatal and should never be observed in
      communication between proper implementations (except when messages
      were corrupted in the network)

(yes, it's not a MUST either, but I'm afraid that this was simply "too
obvious" for designers of protocol)

> So yes, I support adding a check for this (likely above the PACKET
> layer), but I don't think this has much urgency and likely should
> not be back-ported to stable releases.

oh, sure, that particular problem isn't a serious issue, but I'm going
through all fields and all messages so that we have full coverage (e.g.
for valgrind)

also, accepting bigger session id's means that the session cache code is
exercised in ways that it usually isn't, that's not a good thing to
happen (even if it handles them correctly, as it seems, defence in depth
is a good idea anyway)
--
Regards,
Hubert Kario
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
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
In reply to this post by Rich Salz via RT
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.

Cheers


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz 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


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
On Fri, Oct 09, 2015 at 05:02:47pm +0000, Alessandro Ghedini via RT wrote:
> On Thu, Oct 08, 2015 at 07:57:21pm +0000, Alessandro Ghedini via RT wrote:
> > 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?

Ping? FYI I just rebased my patch at [0] on top of the state machine rewrite
commits in master (in fact I've rebased all my patches on master).

Cheers

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


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev