Skip to content

Commit

Permalink
Fix #164: Add padding length check as described by PKCS#1 v1.5
Browse files Browse the repository at this point in the history
According to PKCS#1 v1.5, the padding should be at least 8 bytes long.
See https://tools.ietf.org/html/rfc8017#section-7.2.2 step 3 for more info.
  • Loading branch information
sybrenstuvel committed Nov 15, 2020
1 parent dae8ce0 commit f878c37
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 1 deletion.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -4,6 +4,8 @@

- Fix #165: CVE-2020-25658 - Bleichenbacher-style timing oracle in PKCS#1 v1.5
decryption code
- Add padding length check as described by PKCS#1 v1.5 (Fixes
[#164](https://github.com/sybrenstuvel/python-rsa/issues/164))


## Version 4.4 & 4.6 - released 2020-06-12
Expand Down
7 changes: 6 additions & 1 deletion rsa/pkcs1.py
Expand Up @@ -262,7 +262,12 @@ def decrypt(crypto: bytes, priv_key: key.PrivateKey) -> bytes:
sep_idx = cleartext.index(b'\x00', 2)
except ValueError:
sep_idx = -1
sep_idx_bad = sep_idx < 0
# sep_idx indicates the position of the `\x00` separator that separates the
# padding from the actual message. The padding should be at least 8 bytes
# long (see https://tools.ietf.org/html/rfc8017#section-7.2.2 step 3), which
# means the separator should be at least at index 10 (because of the
# `\x00\x02` marker that preceeds it).
sep_idx_bad = sep_idx < 10

anything_bad = crypto_len_bad | cleartext_marker_bad | sep_idx_bad
if anything_bad:
Expand Down
33 changes: 33 additions & 0 deletions tests/test_pkcs1.py
Expand Up @@ -183,3 +183,36 @@ def test_apppend_zeroes(self):
signature = signature + bytes.fromhex('0000')
with self.assertRaises(rsa.VerificationError):
pkcs1.verify(message, signature, self.pub)


class PaddingSizeTest(unittest.TestCase):
def test_too_little_padding(self):
"""Padding less than 8 bytes should be rejected."""

# Construct key that will be small enough to need only 7 bytes of padding.
# This key is 168 bit long, and was generated with rsa.newkeys(nbits=168).
self.private_key = rsa.PrivateKey.load_pkcs1(b'''
-----BEGIN RSA PRIVATE KEY-----
MHkCAQACFgCIGbbNSkIRLtprxka9NgOf5UxgxCMCAwEAAQIVQqymO0gHubdEVS68
CdCiWmOJxVfRAgwBQM+e1JJwMKmxSF0CCmya6CFxO8Evdn8CDACMM3AlVC4FhlN8
3QIKC9cjoam/swMirwIMAR7Br9tdouoH7jAE
-----END RSA PRIVATE KEY-----
''')
self.public_key = rsa.PublicKey(n=self.private_key.n, e=self.private_key.e)

cyphertext = self.encrypt_with_short_padding(b'op je hoofd')
with self.assertRaises(rsa.DecryptionError):
rsa.decrypt(cyphertext, self.private_key)

def encrypt_with_short_padding(self, message: bytes) -> bytes:
# This is a copy of rsa.pkcs1.encrypt() adjusted to use the wrong padding length.
keylength = rsa.common.byte_size(self.public_key.n)

# The word 'padding' has 7 letters, so is one byte short of a valid padding length.
padded = b'\x00\x02padding\x00' + message

This comment has been minimized.

Copy link
@tomato42

tomato42 Nov 15, 2020

that's not correct plaintext: unless the plaintext has the same length as the public key, the check will fail because the first two bytes aren't \x00\x02 after decryption, not because the 8th byte was \x00, so I don't think it exercises the code path we want...


payload = rsa.transform.bytes2int(padded)
encrypted_value = rsa.core.encrypt_int(payload, self.public_key.e, self.public_key.n)
cyphertext = rsa.transform.int2bytes(encrypted_value, keylength)

return cyphertext

0 comments on commit f878c37

Please sign in to comment.