Fix SSL_get0_raw_cipherlist() #2280
Conversation
SSL_get0_raw_cipherlist() was a little too "raw" in the case of an SSLv2 compat ClientHello. In 1.0.2 and below, during version negotiation, if we received an SSLv2 compat ClientHello but actually wanted to do SSLv3+ then we would construct a "fake" SSLv3+ ClientHello. This "fake" ClientHello would have its ciphersuite list converted to the SSLv3+ format. It was this "fake" raw list that got saved away to later be returned by a call to SSL_get0_raw_cipherlist(). In 1.1.0+ version negotiation works differently and we process an SSLv2 compat ClientHello directly without the need for an intermediary "fake" ClientHello. This meant that the raw ciphersuite list being saved was in the SSLv2 format. Any caller of this function would not expect that and potentially overread the returned buffer by one byte. Fixes openssl#2189
I think that would be a good idea. Possibly separate the two intents of that build into two builds. |
|
Err the whole point of SSL_get0_raw_cipherlist is that you get exactly what the client sent you without any further processing by OpenSSL. The bug is assuming it will always return 2. |
Yeah, I get that - but that is not the way it has ever worked when dealing with an SSLv2 compat ClientHello (and we're not actually going to do SSLv2). In versions prior to 1.1.0 we converted an SSLv2 cipher list into SSLv3 format before saving away the raw cipherlist data. This is just restoring the behaviour to how it has always been rather than crashing. The only time it was 3 in <=1.0.2 was if we do real SSLv2 (which obviously doesn't apply in 1.1.0+). |
BTW, I did consider an alternative solution where we save the encoding type in the SSL object rather than doing the conversion - but that would mean you would get different behaviour between 1.0.2 and 1.1.0, which is why I went with this approach. |
|
Ugh it's unfortunate that it never worked. There might be cases where getting the "true" raw format is useful e.g. seeing if the peer sent any "real" SSLv2 ciphers. |
So do you have no objection to this solution? |
|
No, no objection, just a pity it didn't work originally. Hopefully no one will send SSLv2 compatible client hellos in the foreseeable furture. |
SSL_get0_raw_cipherlist() was a little too "raw" in the case of an SSLv2 compat ClientHello. In 1.0.2 and below, during version negotiation, if we received an SSLv2 compat ClientHello but actually wanted to do SSLv3+ then we would construct a "fake" SSLv3+ ClientHello. This "fake" ClientHello would have its ciphersuite list converted to the SSLv3+ format. It was this "fake" raw list that got saved away to later be returned by a call to SSL_get0_raw_cipherlist(). In 1.1.0+ version negotiation works differently and we process an SSLv2 compat ClientHello directly without the need for an intermediary "fake" ClientHello. This meant that the raw ciphersuite list being saved was in the SSLv2 format. Any caller of this function would not expect that and potentially overread the returned buffer by one byte. Fixes #2189 Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #2280) (cherry picked from commit 07afdf3)
SSL_get0_raw_cipherlist() was a little too "raw" in the case of an SSLv2 compat ClientHello. In 1.0.2 and below, during version negotiation, if we received an SSLv2 compat ClientHello but actually wanted to do SSLv3+ then we would construct a "fake" SSLv3+ ClientHello. This "fake" ClientHello would have its ciphersuite list converted to the SSLv3+ format. It was this "fake" raw list that got saved away to later be returned by a call to SSL_get0_raw_cipherlist(). In 1.1.0+ version negotiation works differently and we process an SSLv2 compat ClientHello directly without the need for an intermediary "fake" ClientHello. This meant that the raw ciphersuite list being saved was in the SSLv2 format. Any caller of this function would not expect that and potentially overread the returned buffer by one byte. Fixes #2189 Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #2280)
|
Pushed. Thanks. |
Checklist
Description of change
SSL_get0_raw_cipherlist() was a little too "raw" in the case of an SSLv2
compat ClientHello. In 1.0.2 and below, during version negotiation, if
we received an SSLv2 compat ClientHello but actually wanted to do SSLv3+
then we would construct a "fake" SSLv3+ ClientHello. This "fake" ClientHello
would have its ciphersuite list converted to the SSLv3+ format. It was
this "fake" raw list that got saved away to later be returned by a call to
SSL_get0_raw_cipherlist().
In 1.1.0+ version negotiation works differently and we process an SSLv2
compat ClientHello directly without the need for an intermediary "fake"
ClientHello. This meant that the raw ciphersuite list being saved was in
the SSLv2 format. Any caller of this function would not expect that and
potentially overread the returned buffer by one byte.
Fixes #2189
Note: There is a test that fails due to this issue in the 1.1.0 branch. However it does not get triggered in master even though the test is there. The reason is that the test in question gets skipped in no-shared builds, and the asan travis configuration (where this occurs) is marked as no-shared in master but not in 1.1.0. We might want to consider changing that in master. Not part of this PR though.