Skip to content

Commit

Permalink
Fix #165: CVE-2020-25658 - Bleichenbacher-style timing oracle
Browse files Browse the repository at this point in the history
Use as many constant-time comparisons as practical in the
`rsa.pkcs1.decrypt` function.

`cleartext.index(b'\x00', 2)` will still be non-constant-time. The
alternative would be to iterate over all the data byte by byte in
Python, which is several orders of magnitude slower. Given that a
perfect constant-time implementation is very hard or even impossible to
do in Python [1], I chose the more performant option here.

[1]: https://securitypitfalls.wordpress.com/2018/08/03/constant-time-compare-in-python/
  • Loading branch information
sybrenstuvel committed Nov 15, 2020
1 parent 6f59ff0 commit dae8ce0
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 4 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Python-RSA changelog

## Version 4.7 - in development

- Fix #165: CVE-2020-25658 - Bleichenbacher-style timing oracle in PKCS#1 v1.5
decryption code


## Version 4.4 & 4.6 - released 2020-06-12

Expand Down
12 changes: 8 additions & 4 deletions rsa/pkcs1.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import os
import sys
import typing
from hmac import compare_digest

from . import common, transform, core, key

Expand Down Expand Up @@ -251,17 +252,20 @@ def decrypt(crypto: bytes, priv_key: key.PrivateKey) -> bytes:
# Detect leading zeroes in the crypto. These are not reflected in the
# encrypted value (as leading zeroes do not influence the value of an
# integer). This fixes CVE-2020-13757.
if len(crypto) > blocksize:
raise DecryptionError('Decryption failed')
crypto_len_bad = len(crypto) > blocksize

This comment has been minimized.

Copy link
@tomato42

tomato42 Nov 15, 2020

this is operating on publicly known information, it doesn't have to be done in side-channel free manner

This comment has been minimized.

Copy link
@sybrenstuvel

sybrenstuvel Nov 15, 2020

Author Owner

Fair enough.

This comment has been minimized.

Copy link
@tomato42

tomato42 Nov 15, 2020

also, it's probably a good idea to do this check before decryption, as then the attacker can't isolate the signal that comes from conversion between integer and bytes


# If we can't find the cleartext marker, decryption failed.
if cleartext[0:2] != b'\x00\x02':
raise DecryptionError('Decryption failed')
cleartext_marker_bad = not compare_digest(cleartext[:2], b'\x00\x02')

# Find the 00 separator between the padding and the message
try:
sep_idx = cleartext.index(b'\x00', 2)
except ValueError:

This comment has been minimized.

Copy link
@tomato42

tomato42 Nov 15, 2020

Have you checked that bytes.index() with exception handling is more data invariant than bytes.find()? I doubt it, but I haven't checked it actually...

This comment has been minimized.

Copy link
@sybrenstuvel

sybrenstuvel Nov 15, 2020

Author Owner

I think bytes.find() is indeed the nicer option here.

sep_idx = -1
sep_idx_bad = sep_idx < 0

anything_bad = crypto_len_bad | cleartext_marker_bad | sep_idx_bad
if anything_bad:
raise DecryptionError('Decryption failed')

This comment has been minimized.

Copy link
@tomato42

tomato42 Nov 15, 2020

I'm quite sure that just the fact of rising the exception in case of malformed padding is enough to provide good enough signal to make it exploitable

This comment has been minimized.

Copy link
@sybrenstuvel

sybrenstuvel Nov 15, 2020

Author Owner

What would you suggest as a fix? Not raising an exception would break the API in such a way that decryption errors will go unnoticed, which is probably much worse.

This comment has been minimized.

Copy link
@tomato42

tomato42 Nov 15, 2020

change this API so that decryption errors can go unnoticed—this is exactly the protection necessary for Bleichenbacher—and provide a new API for the cases that absolutely can't deal with false decryptions and mark that new API as "totally unsafe; every use gets a free CVE!"


return cleartext[sep_idx + 1:]
Expand Down

0 comments on commit dae8ce0

Please sign in to comment.