[openssl-dev] [openssl.org #4623] OpenSSL master regression in handling malformed Client Key Exchange messages in RSA key exchange

[Hubert Kario via RT]

the issue is present in master 0ed26acce328ec16a3aa and looks to have been
introduced in commit:

$ git bisect bad
5b8fa431ae8eb5a18ba913494119e394230d4b70 is the first bad commit
commit 5b8fa431ae8eb5a18ba913494119e394230d4b70
Author: David Benjamin <***@google.com>
Date: Thu Jun 16 14:15:19 2016 -0400

Make RSA key exchange code actually constant-time.

Using RSA_PKCS1_PADDING with RSA_private_decrypt is inherently unsafe.
The API requires writing output on success and touching the error queue
on error. Thus, although the padding check itself is constant-time as of
294d1e36c2495ff00e697c9ff622856d3114f14f, and the logic after the
decryption in the SSL code is constant-time as of
adb46dbc6dd7347750df2468c93e8c34bcb93a4b, the API boundary in the middle
still leaks whether the padding check succeeded, giving us our
much-loved Bleichenbacher padding oracle.

Instead, PKCS#1 padding must be handled by the caller which uses
RSA_NO_PADDING, in timing-sensitive code integrated with the
Bleichenbacher mitigation. Removing PKCS#1 padding in constant time is
actually much simpler when the expected length is a constant (and if
it's not a constant, avoiding a padding oracle seems unlikely), so just
do it inline.

Signed-off-by: Kurt Roeckx <***@roeckx.be>
Reviewed-by: Rich Salz <***@openssl.org>

GH: #1222

:040000 040000 bd98bdbeaeddbc22f8a37324aaa1f1a527712a02
9cc68f63ca3b34ed1f13b05029c3b4bb968a21cb M ssl

when the OpenSSL receives a Client Key Exchange message that has the encrypted
premaster secret comprised only of zero bytes, or equal to server's modulus,
the server just aborts the connection without sending an Alert message

OpenSSL 1.0.2 (b746aa3fe05b5b) and 1.0.1 (6adf409c7432b9) do send Alert
messages in this situation

Reproducer:
===========
(requires Python 2.6, 3.2 or later)
git clone https://github.com/tomato42/tlsfuzzer.git
pushd tlsfuzzer
git clone https://github.com/warner/python-ecdsa .python-ecdsa
ln -s .python-ecdsa/ecdsa ecdsa
git clone https://github.com/tomato42/tlslite-ng.git .tlslite-ng
pushd .tlslite-ng
popd
ln -s .tlslite-ng/tlslite tlslite
popd
openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt \
-nodes -batch -subj /CN=localhost
openssl s_server -www -key localhost.key -cert localhost.crt
# in another terminal, same directory
PYTHONPATH=tlsfuzzer python
tlsfuzzer/scripts/test-invalid-rsa-key-exchange-messages.py
--
Regards,
Hubert Kario
Senior 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

[Stephen Henson via RT]

Post by Hubert Kario via RT
the issue is present in master 0ed26acce328ec16a3aa and looks to have
been

I tried what I thought was a fix for this which is to simply delete the lines:

if (decrypt_len < 0)
goto err;

from ssl/statem/statem_srvr.c

However your reproducer still indicates errors. I checked the message logs and
it should be now sending as many alerts as the original. The difference however
is that some of them will be sent immediately whereas originally they would be
at the end of the handshake.

Could your reproducer possibly not be expecting this?

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org

[Hubert Kario via RT]

Post by Stephen Henson via RT

Post by Hubert Kario via RT
the issue is present in master 0ed26acce328ec16a3aa and looks to have
been

I tried what I thought was a fix for this which is to simply delete the
if (decrypt_len < 0)
goto err;
from ssl/statem/statem_srvr.c
However your reproducer still indicates errors. I checked the message logs
and it should be now sending as many alerts as the original. The difference
however is that some of them will be sent immediately whereas originally
they would be at the end of the handshake.
Could your reproducer possibly not be expecting this?

yes, it expects to be hitting the Bleichenbacher workaround - use of different
premaster secret in case of problems with CKE message - as it's the same
behaviour OpenSSL, NSS and GnuTLS exhibit

[Hubert Kario via RT]

Post by Stephen Henson via RT

Post by Hubert Kario via RT
the issue is present in master 0ed26acce328ec16a3aa and looks to have
been

I tried what I thought was a fix for this which is to simply delete the
if (decrypt_len < 0)
goto err;
from ssl/statem/statem_srvr.c
However your reproducer still indicates errors. I checked the message logs
and it should be now sending as many alerts as the original. The difference
however is that some of them will be sent immediately whereas originally
they would be at the end of the handshake.
Could your reproducer possibly not be expecting this?
when the OpenSSL receives a Client Key Exchange message that has the
encrypted
premaster secret comprised only of zero bytes, or equal to server's modulus,
the server just aborts the connection without sending an Alert message

from the DHE/ECDHE bug reports

the expected behaviour is to continue the connection, but the server should
select a premaster secret that was not provided by the client, instead OpenSSL
just closes the connection

[David Benjamin via RT]

Post by Stephen Henson via RT

Post by Stephen Henson via RT

Post by Hubert Kario via RT
the issue is present in master 0ed26acce328ec16a3aa and looks to have
been

I tried what I thought was a fix for this which is to simply delete the
if (decrypt_len < 0)
goto err;
from ssl/statem/statem_srvr.c
However your reproducer still indicates errors. I checked the message

logs

Post by Stephen Henson via RT
and it should be now sending as many alerts as the original. The

difference

Post by Stephen Henson via RT
however is that some of them will be sent immediately whereas originally
they would be at the end of the handshake.
Could your reproducer possibly not be expecting this?
when the OpenSSL receives a Client Key Exchange message that has the
encrypted
premaster secret comprised only of zero bytes, or equal to server's

modulus,

Post by Stephen Henson via RT
the server just aborts the connection without sending an Alert message

from the DHE/ECDHE bug reports
the expected behaviour is to continue the connection, but the server should
select a premaster secret that was not provided by the client, instead
OpenSSL
just closes the connection

I don't believe this test is correct if the encrypted premaster is equal to
the server's modulus. Such an encrypted premaster is a publicly invalid RSA
ciphertext. It is perfectly reasonable to reject it early, should unpadded
RSA_decrypt fail. The timing sensitive portion is the padding check itself,
which this code should correctly defer failure of, to avoid the padding
oracle.

The reason public decrypt failures did not trigger early alerts before was
because the old code was a little sloppy about error-handling. In addition
to not being constant time, it squashed together codepaths for publicly
invalid keys and the timing sensitive check.

It is true that it no longer sends an alert on public failures. Probably
worth fixing. Meh.

David

[Kurt Roeckx]

Post by David Benjamin via RT

Post by Stephen Henson via RT

Post by Stephen Henson via RT

Post by Hubert Kario via RT
the issue is present in master 0ed26acce328ec16a3aa and looks to have
been

I tried what I thought was a fix for this which is to simply delete the
if (decrypt_len < 0)
goto err;
from ssl/statem/statem_srvr.c
However your reproducer still indicates errors. I checked the message

logs

Post by Stephen Henson via RT
and it should be now sending as many alerts as the original. The

difference

Post by Stephen Henson via RT
however is that some of them will be sent immediately whereas originally
they would be at the end of the handshake.
Could your reproducer possibly not be expecting this?
when the OpenSSL receives a Client Key Exchange message that has the
encrypted
premaster secret comprised only of zero bytes, or equal to server's

modulus,

Post by Stephen Henson via RT
the server just aborts the connection without sending an Alert message

from the DHE/ECDHE bug reports
the expected behaviour is to continue the connection, but the server should
select a premaster secret that was not provided by the client, instead
OpenSSL
just closes the connection

I don't believe this test is correct if the encrypted premaster is equal to
the server's modulus. Such an encrypted premaster is a publicly invalid RSA
ciphertext. It is perfectly reasonable to reject it early, should unpadded
RSA_decrypt fail. The timing sensitive portion is the padding check itself,
which this code should correctly defer failure of, to avoid the padding
oracle.

I think that the test suite should allow for either of 2
behaviours in case of the public failures:
- It continues with a random premaster secret.
- It generates an error.

From what I understand it now complains that we don't do the first,
so I think it should get changed to allow both behaviours.

Post by David Benjamin via RT
It is true that it no longer sends an alert on public failures. Probably
worth fixing. Meh.

I don't care for which behaviour we go, and I guess that if we go
for generating an error we should send an alert.

I don't see the point in wasting time for what most likely seems
to be an attack.

What I don't understand currently is that it also rejected a
ciphertext with all 0's? Were it too many 0's?


Kurt

[Kurt Roeckx via RT]

Post by David Benjamin via RT

Post by Stephen Henson via RT

Post by Stephen Henson via RT

Post by Hubert Kario via RT
the issue is present in master 0ed26acce328ec16a3aa and looks to have
been

I tried what I thought was a fix for this which is to simply delete the
if (decrypt_len < 0)
goto err;
from ssl/statem/statem_srvr.c
However your reproducer still indicates errors. I checked the message

logs

Post by Stephen Henson via RT
and it should be now sending as many alerts as the original. The

difference

Post by Stephen Henson via RT
however is that some of them will be sent immediately whereas originally
they would be at the end of the handshake.
Could your reproducer possibly not be expecting this?
when the OpenSSL receives a Client Key Exchange message that has the
encrypted
premaster secret comprised only of zero bytes, or equal to server's

modulus,

Post by Stephen Henson via RT
the server just aborts the connection without sending an Alert message

from the DHE/ECDHE bug reports
the expected behaviour is to continue the connection, but the server should
select a premaster secret that was not provided by the client, instead
OpenSSL
just closes the connection

I don't believe this test is correct if the encrypted premaster is equal to
the server's modulus. Such an encrypted premaster is a publicly invalid RSA
ciphertext. It is perfectly reasonable to reject it early, should unpadded
RSA_decrypt fail. The timing sensitive portion is the padding check itself,
which this code should correctly defer failure of, to avoid the padding
oracle.

I think that the test suite should allow for either of 2
behaviours in case of the public failures:
- It continues with a random premaster secret.
- It generates an error.

From what I understand it now complains that we don't do the first,
so I think it should get changed to allow both behaviours.

Post by David Benjamin via RT
It is true that it no longer sends an alert on public failures. Probably
worth fixing. Meh.

I don't care for which behaviour we go, and I guess that if we go
for generating an error we should send an alert.

I don't see the point in wasting time for what most likely seems
to be an attack.

What I don't understand currently is that it also rejected a
ciphertext with all 0's? Were it too many 0's?


Kurt

[Hubert Kario via RT]

----- Original Message -----

Sent: Saturday, July 23, 2016 2:50:24 PM
Subject: Re: [openssl-dev] [openssl.org #4623] OpenSSL master regression in handling malformed Client Key Exchange
messages in RSA key exchange

Post by David Benjamin via RT

Post by Stephen Henson via RT

Post by Stephen Henson via RT

Post by Hubert Kario via RT
the issue is present in master 0ed26acce328ec16a3aa and looks to have
been

I tried what I thought was a fix for this which is to simply delete the
if (decrypt_len < 0)
goto err;
from ssl/statem/statem_srvr.c
However your reproducer still indicates errors. I checked the message

logs

Post by Stephen Henson via RT
and it should be now sending as many alerts as the original. The

difference

Post by Stephen Henson via RT
however is that some of them will be sent immediately whereas
originally
they would be at the end of the handshake.
Could your reproducer possibly not be expecting this?
when the OpenSSL receives a Client Key Exchange message that has the
encrypted
premaster secret comprised only of zero bytes, or equal to server's

modulus,

Post by Stephen Henson via RT
the server just aborts the connection without sending an Alert message

from the DHE/ECDHE bug reports
the expected behaviour is to continue the connection, but the server
should
select a premaster secret that was not provided by the client, instead
OpenSSL
just closes the connection

I don't believe this test is correct if the encrypted premaster is equal to
the server's modulus. Such an encrypted premaster is a publicly invalid RSA
ciphertext. It is perfectly reasonable to reject it early, should unpadded
RSA_decrypt fail. The timing sensitive portion is the padding check itself,
which this code should correctly defer failure of, to avoid the padding
oracle.

I think that the test suite should allow for either of 2
- It continues with a random premaster secret.
- It generates an error.
From what I understand it now complains that we don't do the first,
so I think it should get changed to allow both behaviours.

I can change it to do that, thing is, that this behaviour will not be shared
with NSS or GnuTLS (and likely other implementations), so it makes
fingerprinting OpenSSL easier.

Post by David Benjamin via RT
It is true that it no longer sends an alert on public failures. Probably
worth fixing. Meh.

I don't care for which behaviour we go, and I guess that if we go
for generating an error we should send an alert.

yes, if you opt for aborting early, to quote RFC, you MUST send an alert

I don't see the point in wasting time for what most likely seems
to be an attack.

you don't have to actually perform the RSA operation, but IMHO the handshake
should continue

What I don't understand currently is that it also rejected a
ciphertext with all 0's? Were it too many 0's?

yes, there are few tests with 0's, some of them will have too many 0's :)
--
Regards,
Hubert Kario
Quality Engineer, QE BaseOS Security team
Email: ***@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic