Make the random generator of _Rsa and RsaPublic configurable.#93
Make the random generator of _Rsa and RsaPublic configurable.#93roberthdevries wants to merge 1 commit intowolfSSL:masterfrom
Conversation
|
Okay to test. Contributor agreement on file. |
|
@roberthdevries please resolve the merge conflict. Thanks! Awesome work on these cleanups. |
This was the only class that has hardcoded the random number generator in the implementation.
3f5fd16 to
193d327
Compare
|
Merge conflict is now resolved. |
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 1 skipped
Posted findings
- [High] Mutable default argument
rng=Random()in_Rsa.__init__—wolfcrypt/ciphers.py:653 - [Medium]
RsaPrivate.__init__not updated to accept/forwardrngparameter —wolfcrypt/ciphers.py:849-851 - [Medium] No tests for the new
rngparameter —tests/test_ciphers.py
Skipped findings
- [Medium] No tests for the new
rngparameter
Review generated by Skoll via openclaw
| _hash_type = None | ||
|
|
||
| def __init__(self): | ||
| def __init__(self, rng=Random()): |
There was a problem hiding this comment.
🟠 [High] Mutable default argument rng=Random() in _Rsa.__init__
🚫 BLOCK bug
The default argument rng=Random() is a classic Python mutable-default-argument bug. Random() is evaluated once at class definition time, and the same object is reused for every call that omits rng. This means all RsaPrivate instances (whose __init__ calls _Rsa.__init__(self) without providing rng on line 851) will share the exact same Random object. If one instance is garbage-collected and its Random object cleaned up, every other instance sharing that default will hold a dangling reference. Even without cleanup issues, sharing a single RNG across independent key objects is a correctness and isolation problem. Ironically, the RsaPublic subclass does this correctly with the rng=None / if rng is None: rng = Random() pattern — the base class should use the same approach.
Suggestion:
| def __init__(self, rng=Random()): | |
| def __init__(self, rng=None): | |
| self.native_object = _ffi.new("RsaKey *") | |
| ret = _lib.wc_InitRsaKey(self.native_object, _ffi.NULL) | |
| if ret < 0: # pragma: no cover | |
| raise WolfCryptError("Invalid key error (%d)" % ret) | |
| if rng is None: | |
| rng = Random() | |
| self._random = rng |
| _Rsa.__init__(self, rng) | ||
|
|
||
| idx = _ffi.new("word32*") | ||
| idx[0] = 0 |
There was a problem hiding this comment.
🟡 [Medium] RsaPrivate.__init__ not updated to accept/forward rng parameter
💡 SUGGEST api
The PR's stated goal is to make the random generator of _Rsa and RsaPublic configurable. However, RsaPrivate.__init__ (line 849) was not updated to accept an rng parameter, and its call to _Rsa.__init__(self) on line 851 still relies on the default. Since RsaPrivate extends RsaPublic, users would reasonably expect RsaPrivate to also support a configurable RNG. Additionally, RsaPrivate.make_key (line 829) and RsaPublic.from_pem (line 718) also don't forward an rng parameter, leaving the configurability incomplete.
Suggestion:
| idx[0] = 0 | |
| def __init__(self, key=None, hash_type=None, rng=None): # pylint: disable=super-init-not-called | |
| if rng is None: | |
| rng = Random() | |
| _Rsa.__init__(self, rng) # pylint: disable=non-parent-init-called |
This was the only class that has hardcoded the random number generator in the implementation.