Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Fri, Oct 09, 2015 at 05:02:47pm +, Alessandro Ghedini via RT wrote: > On Thu, Oct 08, 2015 at 07:57:21pm +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 07:57:21pm +, Alessandro Ghedini via RT wrote: > On Thu, Oct 08, 2015 at 06:26:27pm +, Alessandro Ghedini via RT wrote: > > On Thu, Oct 08, 2015 at 06:14:00pm +, Alessandro Ghedini via RT wrote: > > > On Thu, Oct 08, 2015 at 05:19:06pm +, Alessandro Ghedini via RT wrote: > > > > On Thu, Oct 08, 2015 at 04:12:50pm +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 05:19:06PM +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 04:12:50PM +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 05:19:06PM +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 04:12:50pm +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
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 signature.asc Description: PGP signature ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 06:14:00pm +, Alessandro Ghedini via RT wrote: > On Thu, Oct 08, 2015 at 05:19:06pm +, Alessandro Ghedini via RT wrote: > > On Thu, Oct 08, 2015 at 04:12:50pm +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 05:19:06pm +, Alessandro Ghedini via RT wrote: > On Thu, Oct 08, 2015 at 04:12:50pm +, 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
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thursday 08 October 2015 17:37:12 Viktor Dukhovni wrote: > On Thu, Oct 08, 2015 at 04:12:50PM +, 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 signature.asc Description: This is a digitally signed message part. ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4080] Malformed Client Hello messages are accepted (session_id length)
On Thu, Oct 08, 2015 at 06:26:27pm +, Alessandro Ghedini via RT wrote: > On Thu, Oct 08, 2015 at 06:14:00pm +, Alessandro Ghedini via RT wrote: > > On Thu, Oct 08, 2015 at 05:19:06pm +, Alessandro Ghedini via RT wrote: > > > On Thu, Oct 08, 2015 at 04:12:50pm +, 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
