Skip to content

Fix mutable arguments passed as default arguments.#101

Open
roberthdevries wants to merge 2 commits intowolfSSL:masterfrom
roberthdevries:fix-passing-mutable-argument-as-default
Open

Fix mutable arguments passed as default arguments.#101
roberthdevries wants to merge 2 commits intowolfSSL:masterfrom
roberthdevries:fix-passing-mutable-argument-as-default

Conversation

@roberthdevries
Copy link
Copy Markdown
Contributor

Function defaults are evaluated once, when the function is defined.

The same mutable object is then shared across all calls to the function. If the object is modified, those modifications will persist across calls, which can lead to unexpected behavior.

Function defaults are evaluated once, when the function is defined.

The same mutable object is then shared across all calls to the function.
If the object is modified, those modifications will persist across calls,
which can lead to unexpected behavior.
@roberthdevries roberthdevries force-pushed the fix-passing-mutable-argument-as-default branch from ee90b05 to 786dd94 Compare April 11, 2026 16:55
@roberthdevries
Copy link
Copy Markdown
Contributor Author

One test case test_ecc_make_shared_secret had to be fixed as it apparently relies on using the same random generator.
Reusing the one from the default argument obviously works, but is a dangerous (because implicit and invisible) dependency between the two calls to make a key. In the test case this is now explicit.

Copy link
Copy Markdown
Contributor

@JeremiahM37 JeremiahM37 left a comment

Choose a reason for hiding this comment

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

Skoll Code Review

Scan type: review
Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)

Posted findings

  • [Critical] Use-after-free: EccPrivate.make_key stores dangling RNG pointer when rng=Nonewolfcrypt/ciphers.py:1192-1207
  • [Medium] Test does not cover the default rng=None path for EccPrivate.make_keytests/test_ciphers.py:616-629

Review generated by Skoll

Comment thread wolfcrypt/ciphers.py Outdated
Comment thread tests/test_ciphers.py
Comment thread wolfcrypt/ciphers.py Outdated
Add a test for the case where no random number generator is passed
to EccPrivate.
Copy link
Copy Markdown
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped

Posted findings

  • [High] EccPrivate.make_key: locally-created RNG freed while ECC key still holds pointer via wc_ecc_set_rngwolfcrypt/ciphers.py:1188-1207
  • [Medium] No test exercises the default rng=None path for successful EccPrivate.make_keytests/test_ciphers.py:616-629

Review generated by Skoll via openclaw

Comment thread wolfcrypt/ciphers.py

class EccPrivate(EccPublic):

def __init__(self, key=None, rng=None):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 [High] EccPrivate.make_key: locally-created RNG freed while ECC key still holds pointer via wc_ecc_set_rng
🚫 BLOCK bug

When rng=None (the new default), make_key creates a local Random() on line 1193. On line 1203, wc_ecc_set_rng(ecc.native_object, rng.native_object) stores the native RNG pointer inside the ECC key for later use by wolfSSL (for timing-resistance operations). After make_key returns, the local rng variable has no remaining references. When Python's garbage collector reclaims it, Random.__del__ calls wc_FreeRng, freeing the native WC_RNG memory. The ECC key now holds a dangling pointer.

In the old code, rng=Random() was evaluated once at function definition time and the single Random instance persisted for the lifetime of the module, so the pointer stored by wc_ecc_set_rng remained valid. This PR's fix for the mutable-default-argument antipattern is correct in general, but breaks the implicit lifetime guarantee that wc_ecc_set_rng depends on.

This is a use-after-free that can cause crashes or undefined behavior when the returned ECC key is subsequently used for operations that consult the stored RNG pointer (e.g., signing with timing resistance enabled).

Suggestion:

Suggested change
def __init__(self, key=None, rng=None):
Store the `Random` instance on the returned `ecc` object so it stays alive as long as the key does:
@classmethod
def make_key(cls, size, rng=None):
if rng is None:
rng = Random()
ecc = cls()
ret = _lib.wc_ecc_make_key(rng.native_object, size,
ecc.native_object)
if ret < 0:
raise WolfCryptError("Key generation error (%d)" % ret)
if _lib.ECC_TIMING_RESISTANCE_ENABLED and (not _lib.FIPS_ENABLED or
_lib.FIPS_VERSION > 2):
ret = _lib.wc_ecc_set_rng(ecc.native_object, rng.native_object)
if ret < 0:
raise WolfCryptError("Error setting ECC RNG (%d)" % ret)
ecc._rng = rng # prevent GC from freeing the RNG
return ecc

Comment thread tests/test_ciphers.py
@@ -613,11 +614,12 @@ def test_ecc_sign_verify_raw(ecc_private, ecc_public):


def test_ecc_make_shared_secret():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 [Medium] No test exercises the default rng=None path for successful EccPrivate.make_key
💡 SUGGEST test

The test was updated to pass an explicit rng=rng to EccPrivate.make_key, which is good practice. However, there is now no test that exercises the default rng=None path for a successful make_key call. The only other call (EccPrivate.make_key(1024)) is expected to fail before reaching wc_ecc_set_rng. Adding a test that calls EccPrivate.make_key(32) without an rng argument and then uses the resulting key (e.g., for signing or shared secret) would catch the use-after-free regression described above and verify the default path works correctly.

Suggestion:

Suggested change
def test_ecc_make_shared_secret():
Add a test like:
def test_ecc_make_key_default_rng():
key = EccPrivate.make_key(32) # uses default rng=None path
# Force GC to expose dangling pointer if RNG not anchored
import gc; gc.collect()
pub = EccPublic()
pub.import_x963(key.export_x963())
signature = key.sign(b"test message")
assert pub.verify(b"test message", signature)

@dgarske
Copy link
Copy Markdown
Contributor

dgarske commented Apr 16, 2026

@roberthdevries please also resolve conflicts. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants