Fix mutable arguments passed as default arguments.

[roberthdevries]

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]

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.

[JeremiahM37]

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=None — wolfcrypt/ciphers.py:1192-1207
• [Medium] Test does not cover the default rng=None path for EccPrivate.make_key — tests/test_ciphers.py:616-629

Review generated by Skoll

[JeremiahM37]

🟣 [Critical] Use-after-free: EccPrivate.make_key stores dangling RNG pointer when rng=None

When EccPrivate.make_key is called without an explicit rng argument, the new code creates a temporary Random() as a local variable (line 1193). This RNG's native pointer is then stored inside the returned ECC key via wc_ecc_set_rng(ecc.native_object, rng.native_object) (line 1203) for timing resistance. When make_key returns, rng goes out of scope and becomes eligible for garbage collection. Random.__del__ calls wc_FreeRng, freeing the native RNG memory. The ECC key object now holds a dangling pointer to freed memory. Any subsequent operation that relies on the stored RNG (e.g., shared_secret, signing operations when ECC timing resistance is enabled) will dereference freed memory — a use-after-free.

Before this PR, the rng=Random() default was evaluated once at class definition time. That single Random instance lived as long as the module was loaded, so the stored pointer remained valid. The mutable default was acting as a de-facto module-level singleton. This PR removes that lifetime guarantee without accounting for the wc_ecc_set_rng call that persists the pointer beyond the function scope.

@classmethod
    def make_key(cls, size, rng=None):
        if rng is None:
            rng = Random()          # temporary, GC'd after return
        ecc = cls()
        ret = _lib.wc_ecc_make_key(rng.native_object, size, ecc.native_object)
        ...
        if _lib.ECC_TIMING_RESISTANCE_ENABLED and ...:
            ret = _lib.wc_ecc_set_rng(ecc.native_object, rng.native_object)
            # ^^^ stores raw C pointer to rng inside ecc
        return ecc
        # rng goes out of scope -> __del__ -> wc_FreeRng -> dangling pointer in ecc

Recommendation: Pin the Random instance to the EccPrivate object (ecc._rng = rng) after calling wc_ecc_set_rng, so the Python object's lifetime is tied to the key and the native RNG pointer remains valid. This is needed only in EccPrivate.make_key because it is the only changed function that stores the RNG pointer past the function scope.

[JeremiahM37]

🟠 [Medium] Test does not cover the default rng=None path for EccPrivate.make_key

The updated test now passes an explicit rng=rng to both EccPrivate.make_key calls. This means the default rng=None path (which is the one with the dangling pointer bug) is never tested with subsequent key usage. The only other call site (EccPrivate.make_key(1024) at line 520) tests an error path and doesn't exercise the timing resistance code. Adding a test that calls make_key without an explicit rng and then uses the key (e.g., shared_secret or sign) would catch the use-after-free.

def test_ecc_make_shared_secret():
    rng = Random()
    a = EccPrivate.make_key(32, rng=rng)   # explicit rng, safe
    ...
    b = EccPrivate.make_key(32, rng=rng)   # explicit rng, safe

Recommendation: Add a test case that calls EccPrivate.make_key() without an explicit rng and then performs operations on the returned key to verify the RNG lifetime is handled correctly.

[JeremiahM37]

Same issue applies when a caller supplies their own rng — since wc_ecc_set_rng retains the pointer, the caller's Random() can also go out of scope after make_key returns:

ecc = EccPrivate.make_key(32, rng=Random())
# Random __del__ runs, frees the WC_RNG
ecc.shared_secret(...)  # dangling pointer

The ecc._rng = rng assignment needs to be unconditional (after this wc_ecc_set_rng call), not just in the if rng is None branch.