Repository navigation
Release 4.5.post1: CVE-2020-25658 for the Python 2.7 line - #4
Conversation
Fix sybrenstuvel#165: CVE-2020-25658 - Bleichenbacher-style timing oracle 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/ (cherry picked from commit 81cda70)
(cherry picked from commit 4a26fa2)
The fork's earlier backport (PR #1) ported upstream dae8ce0 only. Upstream 4.7 refined it in two follow-ups, ported here: - 341e5c4: the ciphertext length check raises at once. Length and block size are public, so they need no side-channel-free comparison. - f254895: bytes.find() instead of bytes.index() with a ValueError handler, so a missing separator takes the same path as a present one. Also drops the unused 'import sys' that PR #1 added, and adds regression tests that every malformed cleartext (bad 00 02 marker, no separator, over-long ciphertext) still raises DecryptionError through the combined check. Python 2.7: hmac.compare_digest exists from 2.7.7. (cherry picked from commit ca9f489)
… (upstream f878c37) RFC 8017 section 7.2.2 step 3 requires at least 8 bytes of padding, so the 00 separator must sit at index 10 or later. decrypt() accepted any separator after the marker. Part of upstream 4.7's decryption hardening alongside the CVE-2020-25658 fix. Upstream's test, without its Python 3 type annotation. (cherry picked from commit 4d9889e)
… 2.7 line Sets the version to 4.5.post1 and restores the 4.3 & 4.5 changelog section that the CVE-2020-25658 backport (cherry-picked from version-4.3.x) had rewritten, with a 4.5.post1 section on top. (cherry picked from commit 987fe91)
ezequielp-activestate
left a comment
There was a problem hiding this comment.
Approve: 0 blockers.
🔴 Blockers
None.
Checked
- The base
4834b903bbis the upstreamversion-4.5tag. This PR's tree is byte-identical to #3's head (987fe91). decrypt()inrsa/pkcs1.pynow matches upstream 4.7–4.9 line for line:- the immediate length check (341e5c4);
compare_digeston the marker (dae8ce0);find()instead ofindex()(f254895);sep_idx < 10(f878c37 / sybrenstuvel#164);- a single combined check.
hmac.compare_digestexists on Python 2.7.7+.- Upstream history from
version-4.5toversion-4.9forkey.py,pkcs1.py,core.pyandtransform.pyhas no further security fixes to carry. The threading fix (88418f0) and the pickle fix (eb6ddb6) are for blinding-factor caching, which 4.7 introduced and 4.5 doesn't have. - OSV for rsa 4.5 lists only CVE-2020-25658 (GHSA-xrx6-fmxq-rjj2 / PYSEC-2020-100).
- Python 2.7:
- PR: 97 passed.
- Base with this PR's
test_pkcs1.pyadded: onlyPaddingSizeTest::test_too_little_paddingfails (DecryptionError not raised).
setup.py sdist bdist_wheelgivesrsa-4.5.post1.tar.gzandrsa-4.5.post1-py2.py3-none-any.whl, andrsa.__version__is4.5.post1.
⚪ Notes
- Upstream 8ed5071 ("Choose blinding factor relatively prime to N") isn't included. It's a correctness fix for a very unlikely case, not a security fix, so it's optional.
Not assessed
- I didn't measure decrypt timing. As the PR says, the CVE tests check behaviour, not timing.
hmac.compare_digest arrived in Python 2.7.7, but setup.py accepts any 2.7. On 2.7.0 to 2.7.6 the CVE-2020-25658 backport's import made `import rsa` fail outright. rsa._compat now supplies a constant-time fallback when hmac lacks it, and pkcs1 imports it from there.
swagile1
left a comment
There was a problem hiding this comment.
1 blocker: a test docstring that claims more than the test shows. The CVE fix itself is a faithful copy of upstream and holds up under testing. Reviewed at head 807eb28 against version-4.5.x.
Evidence
decrypt()matches upstreamversion-4.7line for line, apart from type annotations. Upstreamdae8ce0,f254895,341e5c4andf878c37all exist and are contained inversion-4.7.- Full suite: 97 passed on Python 2.7.18 and 97 on Python 3.8, both in Docker.
- Mutation tests: I broke each new check in a scratch copy, and the right test failed each time:
- padding check set back to
< 0→PaddingSizeTestfails - marker check set to
False→test_bad_markerfails - early length check deleted →
test_prepend_zeroesandtest_crypto_longer_than_blockfail
- padding check set back to
- Each commit on its own passes on 2.7 (92 → 92 → 96 → 97 tests).
- No check from the old code was dropped: the marker, separator and length checks all survive.
🔴 Blockers
tests/test_pkcs1.py:188[importance 3 / effort 1]: the docstring says "every malformed cleartext still fails, through one combined check."test_crypto_longer_than_blockcontradicts it: since341e5c4, that case raises at the early length check (rsa/pkcs1.py:241), not at the combined check. "Every" also overreaches: short padding is tested inPaddingSizeTest. Suggested wording: "CVE-2020-25658: each malformed block still raises DecryptionError after the checks were restructured."
🟡 Should fix (follow-up, not blocking)
- No CI runs on this fork
[2 / 2]:.travis.ymlis the only CI config, and the branch reports no checks. The security tag will be cut on a local run. A small GitHub Actions job with a 2.7 container would cover this release and the next backport. This was true before this PR.
⚪ Nits
rsa/pkcs1.py:33[1 / 1]:hmac.compare_digestarrived in Python 2.7.7, butsetup.py:53still allows>=2.7. On 2.7.0–2.7.6,import rsawould now fail outright. Raising the floor to>=2.7.7would match the code.tests/test_pkcs1.py:193[1 / 1]:encrypt_rawduplicatesPaddingSizeTest.encrypt_with_short_padding.test_crypto_longer_than_blockoverlapsExtraZeroesTest.test_prepend_zeroes.test_valid_padding_decryptsoverlapsBinaryTest.test_enc_dec.
CHANGELOG.txt:4,rsa/__init__.py:31[1 / 1]: the 4.5.post1 changelog heading has no release date, unlike every other entry, and__date__still says2020-06-12.- Commit sequence
[1 / 1]:6ba6f6eimportstyping. Bare Python 2.7 has notypingmodule, soimport rsafails at that commit.- The same commit adds a stray
## Version 4.4 & 4.6heading to the changelog, which807eb28later repairs. - Squashing
6ba6f6eandcd3b88ftogether would make every commit work forgit bisect. Keeping them separate for provenance is a reasonable choice too.
Worth knowing for the release
A consumer pinned to rsa==4.5 will not pick up 4.5.post1, because under Python's version rules ==4.5 excludes post-releases. Only >=/~= ranges, or builds from our inventory, get the fix.
Not assessed
- The sdist/wheel build claim in the PR body.
- Actual constant-time behavior: no unit test can measure it, so that claim rests on parity with upstream.
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved spelling nit remains in rsa/pkcs1.py:254: change “preceeds” to “precedes” in the marker explanation.
1 open issue
What changed in this PR
Prepares rsa 4.5.post1 as a Python 2.7 security release, completing the backport of upstream PKCS#1 v1.5 decryption fixes.
Changes:
- Combines malformed-padding checks and rejects padding shorter than eight bytes.
- Adds a
compare_digestfallback for Python versions before 2.7.7 and regression tests. - Updates package versions and documents the security release.
| File | Description |
|---|---|
tests/test_pkcs1.py |
Adds valid-block and malformed-padding regression tests. |
tests/test_compat.py |
Tests the digest-comparison fallback. |
setup.py |
Sets package version to 4.5.post1. |
rsa/pkcs1.py |
Backports decryption checks and minimum padding validation. |
rsa/_compat.py |
Provides a fallback when hmac.compare_digest is unavailable. |
rsa/__init__.py |
Updates the exposed library version. |
CHANGELOG.txt |
Documents the security backports. |
Review effort: Balanced
| # 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). |
There was a problem hiding this comment.
Leaving this as is on purpose. This comment is copied verbatim from upstream f878c37, which still spells it "preceeds" in 4.7 to 4.9. Keeping decrypt() byte-identical to upstream lets anyone check the backport with a plain diff against version-4.7.
|
Correction to my review above: I reviewed
|
…ate test, date the release - The class docstring now says what its tests show: a bad marker or a missing separator fails through the combined check. A ciphertext longer than the key fails earlier, at the length check. - Drop test_crypto_longer_than_block: ExtraZeroesTest.test_prepend_zeroes already covers it, and still fails when the length check is removed. - Note why test_valid_padding_decrypts stays: it is the control for block(). - Date the 4.5.post1 changelog entry and __date__ (2026-11-10).
|
Thanks both.
@ezequielp-activestate: on upstream |
swagile1
left a comment
There was a problem hiding this comment.
Approve: 0 blockers. Re-review at head 6aacc00; my last review saw 807eb28 (with a correction comment for 5207034). The only change since is 6aacc00, whose diff I read in full; I checked the earlier tree only through the test runs below.
🔴 Blockers
None.
Earlier findings, checked against the code
- 🔴
DecryptionFailureTestdocstring: fixed.tests/test_pkcs1.py:188now claims only a bad marker and a missing separator. Both fail throughanything_bad(rsa/pkcs1.py:257). A missing separator givesfind()= -1, sosep_idx_badcatches it. The docstring sends the length case toExtraZeroesTestand short padding toPaddingSizeTest, which is correct. - ⚪ Duplicate
test_crypto_longer_than_block: dropped. I disabled the length check atrsa/pkcs1.py:240on a scratch copy.ExtraZeroesTest::test_prepend_zeroesstill fails (DecryptionError not raised,tests/test_pkcs1.py:93), so nothing lost coverage. - ⚪
test_valid_padding_decryptsandencrypt_raw: kept, and the reasons hold. The control test proves the hand-builtblock()decrypts. That is the premise of the two failure tests, andtest_enc_deccannot show it.PaddingSizeTestis upstreamf878c37verbatim. - ⚪ Changelog date and
__date__: fixed (CHANGELOG.txt:4,rsa/__init__.py:31). - ⚪
compare_digestbefore 2.7.7: fixed in5207034, as noted in my earlier comment. - ⚪ Commit sequence: keeping
6ba6f6eandcd3b88fseparate for provenance is fine. - 🟡 No CI on the fork: now tracked as PROD-2446.
- Copilot's "preceeds" thread: leaving it is the right call.
version-4.7:rsa/pkcs1.py:269spells it the same way. Apart from the type annotations on the signature,decrypt()diffs clean againstversion-4.7.
Evidence (Python 2.7.18, python:2.7-slim)
- Full suite: 99 passed.
- Mutations, one at a time, on
tests/test_pkcs1.py. Each fails exactly one test:- length check off:
test_prepend_zeroesfails; sep_idx_bad = False: one test fails;cleartext_marker_bad = False: one test fails.
- length check off:
- Upstream
8ed5071("Choose blinding factor relatively prime to N") is3bf7b2e, an ancestor ofversion-4.5, so the base already carries it.
⚠️ Not assessed
- Decrypt timing was not measured; the tests pin behavior only.
- The
compare_digestfallback has not run on a real 2.7.0–2.7.6 interpreter. - The THR review tooling handles only TheHomeRepot PRs, so this review was done by hand against its checklist.

rsa 4.5.post1: Python 2.7 security release
rsa 4.5 is the last release that supports Python 2.7. It is still exposed to CVE-2020-25658 (GHSA-xrx6-fmxq-rjj2, high), a Bleichenbacher-style timing oracle in PKCS#1 v1.5 decryption. Upstream fixed it only in 4.7, which needs Python 3. The fix was started on
version-4.3.xin 2023 (#1, #2) but never released; that branch still says version 4.3.This PR is based on the
version-4.5tag (new base branchversion-4.5.x), so its history contains upstream's 4.5 rather than the 4.3 it was re-tagged from. On top of it are the two 2023 commits fromversion-4.3.x, cherry-picked, then this release's four. It replaces #3: same tree as #3's head up to 807eb28; 5207034 was added after self-review.version-4.3.x(#1: upstreamdae8ce0; #2: drop thetypingimport), cherry-picked.decrypt()to upstream 4.7's final form. #1 ported upstreamdae8ce0only; this adds341e5c4(the public length check raises at once) andf254895(find()instead ofindex()/ValueError, so a missing separator takes the same path). Drops the unusedimport sysfrom #1. Adds regression tests that every malformed cleartext still raisesDecryptionError.f878c37): reject padding shorter than 8 bytes (RFC 8017 7.2.2 step 3). Upstream's test, without its Python 3 annotation.hmac.compare_digestarrived in 2.7.7, butpython_requiresaccepts any 2.7, so on 2.7.0 to 2.7.6 the backport madeimport rsafail.rsa._compatnow supplies a constant-time fallback whenhmaclacks it;pkcs1imports it from there.DecryptionFailureTestdocstring, drops a test that duplicatedExtraZeroesTest.test_prepend_zeroes, dates the 4.5.post1 changelog entry and__date__.4.5.post1inrsa/__init__.pyandsetup.py. The 4.3 & 4.5 changelog section, which #1 had rewritten, is restored, with a 4.5.post1 section on top.Advisories against rsa
OSV lists nothing against 4.7 through 4.9.1, so no later upstream fix needs carrying.
Verification (Python 2.7.18)
tests/: 99 passed (Python 2.7.18). Upstream 4.5 has 92; the 7 new tests account for the rest (4 intest_pkcs1.py, 3 for thecompare_digestfallback).hmac.compare_digestbeforeimport rsaon 2.7.18:pkcs1picks up_compare_digestand an encrypt/decrypt round trip works. Not run on a real 2.7.0-2.7.6 interpreter.test_bad_marker;sep_idx < 0failstest_too_little_padding; dropping the separator check failstest_no_separatorandtest_too_little_padding; making the fallback always return True failsTestCompareDigest.PaddingSizeTestfails without thepkcs1.pychange and passes with it.setup.py sdist bdist_wheelgivesrsa-4.5.post1.tar.gzandrsa-4.5.post1-py2.py3-none-any.whl.rsa.__version__is4.5.post1.After merge: tag
version-4.5.post1(the repo's tag style), a GitHub release (--latest=false), inventoryrsa 4.5.0.1, and a CVE-scanner override row for CVE-2020-25658.