Skip to content

Commit

Permalink
Attempt to mitigate Bleichenbacher attacks on RSA decryption (#5507)
Browse files Browse the repository at this point in the history
  • Loading branch information
alex authored Oct 26, 2020
1 parent cf9bd6a commit 58494b4
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 15 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ Changelog

.. note:: This version is not yet released and is under active development.

* **SECURITY ISSUE:** Attempted to make RSA PKCS#1v1.5 decryption more constant
time, to protect against Bleichenbacher vulnerabilities. Due to limitations
imposed by our API, we cannot completely mitigate this vulnerability and a
future release will contain a new API which is designed to be resilient to
these for contexts where it is required. Credit to **Hubert Kario** for
reporting the issue. *CVE-2020-25659*
* Support for OpenSSL 1.0.2 has been removed. Users on older version of OpenSSL
will need to upgrade.
* Added basic support for PKCS7 signing (including SMIME) via
Expand Down
1 change: 1 addition & 0 deletions docs/spelling_wordlist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ backend
Backends
backends
bcrypt
Bleichenbacher
Blowfish
boolean
Botan
Expand Down
26 changes: 11 additions & 15 deletions src/cryptography/hazmat/backends/openssl/rsa.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,23 +119,19 @@ def _enc_dec_rsa_pkey_ctx(backend, key, data, padding_enum, padding):

outlen = backend._ffi.new("size_t *", buf_size)
buf = backend._ffi.new("unsigned char[]", buf_size)
# Everything from this line onwards is written with the goal of being as
# constant-time as is practical given the constraints of Python and our
# API. See Bleichenbacher's '98 attack on RSA, and its many many variants.
# As such, you should not attempt to change this (particularly to "clean it
# up") without understanding why it was written this way (see
# Chesterton's Fence), and without measuring to verify you have not
# introduced observable time differences.
res = crypt(pkey_ctx, buf, outlen, data, len(data))
resbuf = backend._ffi.buffer(buf)[: outlen[0]]
backend._lib.ERR_clear_error()
if res <= 0:
_handle_rsa_enc_dec_error(backend, key)

return backend._ffi.buffer(buf)[: outlen[0]]


def _handle_rsa_enc_dec_error(backend, key):
errors = backend._consume_errors_with_text()
if isinstance(key, _RSAPublicKey):
raise ValueError(
"Data too long for key size. Encrypt less data or use a "
"larger key size.",
errors,
)
else:
raise ValueError("Decryption failed.", errors)
raise ValueError("Encryption/decryption failed.")
return resbuf


def _rsa_sig_determine_padding(backend, key, padding, algorithm):
Expand Down

16 comments on commit 58494b4

@kirotawa
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I'm trying to backporting that fix for an version 1.2.3 , in particular because it has that _098 function. I was wondering if it is needed to patch it in order to have a proper backport/fix. I understand you folks don't support old versions and so, but any tips, clues in how to proceed here would be very appreciated.

Thanks in advance

`def _enc_dec_rsa_098(backend, key, data, padding_enum):
if isinstance(key, _RSAPublicKey):
┆ crypt = backend._lib.RSA_public_encrypt
else:
┆ crypt = backend._lib.RSA_private_decrypt

key_size = backend._lib.RSA_size(key._rsa_cdata)                                                                                                                                                                                                              
backend.openssl_assert(key_size > 0)                                                                                                                                                                                                                          
buf = backend._ffi.new("unsigned char[]", key_size)                                                                                                                                                                                                           
res = crypt(len(data), data, buf, key._rsa_cdata, padding_enum)                                                                                                                                                                                               
if res < 0:                                                                                                                                                                                                                                                   
┆   _handle_rsa_enc_dec_error(backend, key)                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                              
return backend._ffi.buffer(buf)[:res]`

@alex
Copy link
Member Author

@alex alex commented on 58494b4 Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to make similar changes to both the 098 and pkey paths.

@kirotawa
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it makes sense. I'll try this. Thanks a bunch!

@Platinumwrist
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything from this line onwards is written with the goal of being as
# constant-time as is practical given the constraints of Python and our
# API. See Bleichenbacher's '98 attack on RSA, and its many many variants.
# As such, you should not attempt to change this (particularly to "clean it
# up") without understanding why it was written this way (see
# Chesterton's Fence), and without measuring to verify you have not
# introduced observable time differences.
res = crypt(pkey_ctx, buf, outlen, data, len(data)) res = crypt(pkey_ctx, buf, outlen, data, len(data))
resbuf = backend._ffi.buffer(buf)[: outlen[0]]
backend._lib.ERR_clear_error()
if res <= 0: if res <= 0:
_handle_rsa_enc_dec_error(backend, key) raise ValueError("Encryption/decryption failed.")

return resbuf

@mattcocciolo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch security cryptograph

@mattcocciolo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch security cryptograph

@austinmhyatt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still being worked on / maintained?
3.4.7 still reports a high severity vuln.

@reaperhulk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever is notifying you is inaccurate. We shipped this in version 3.2.

@austinmhyatt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we can just assume it's inaccurate. It's reported that reverting to 3.2 also opens up another Medium severity vuln. Will there be version released later than 3.4.7 with the fix?

@alex
Copy link
Member Author

@alex alex commented on 58494b4 Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your data source is wrong, this issue has been fixed in all versions since 3.2.

@austinmhyatt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be version released later than 3.4.7 with the fix?

@reaperhulk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t understand what you’re trying to say. Literally every version since 3.2 has had it. All versions past 3.4.7 (which also has it) will, of course, have it.

@alevikpes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@austinmhyatt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @alevikpes yep this is where it's being reported.
so as far we snyk sees, it's not fixed in 3.4.7

@austinmhyatt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this an incomplete fix which won't fly on our end and for probably most people.

@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented on 58494b4 Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is incomplete because removing 100% of the timing differential requires a different API. (This is also only relevant if you're using these APIs in a fashion where they could be oracular.)

Ultimately this is a volunteer run project and no one has stepped up to provide that API. The tragedy here is that if/when we provide that constant-time API the vast, vast majority of callers won't migrate to it, but these scanners will proclaim it "fixed" since they only care about what version you're using, not the actual methods or the manner in which they're used.

For those who need this for genuine security reasons (e.g. implementation of online verification for a protocol, etc), please feel free to propose an API. #6167 is probably the right place to discuss it! We'd love to see this get done, but this thread is not the correct path.

I am locking this, but if people are interested in designing and implementing a constant-time variant API #6167 awaits. 😄

Edit: I have been told that there exist some Python scanners that detect API use. So the above is inaccurate, but the overall point remains since no static analyzer can determine the safety of the use of the "old" API. Marking all uses of it as vulnerable is incorrect, but also the best automation can do...

Please # to comment.