Fenrir fixes#118
Merged
Merged
Conversation
AesSiv._prepare_associated_data only checked for str and bytes when deciding whether the input was a single associated-data block, so bytearray and memoryview fell through to the "list of blocks" branch. Iterating those types yields integers, which t2b() then turned into ASCII decimal byte-strings (b'16', b'17', ...), producing many bogus blocks instead of one. The C SIV computation succeeded over wrong associated data, so encryption/decryption silently produced an incorrect tag rather than raising. This broke interoperability between callers passing the same content as different buffer types. Match the set of types accepted by t2b() by including bytearray and memoryview in the isinstance check. Add a parametrized test that reuses the OpenSSL KAT vectors with bytes, bytearray, and memoryview wrappers; a round-trip test would not have caught this since both sides mangle identically. F-1981
Random.__init__ used _lib.INVALID_DEVID as a default parameter value, but INVALID_DEVID is only declared in the CFFI cdef inside `if features["ML_KEM"] or features["ML_DSA"]`. Since default parameters are evaluated at module import time, any wolfSSL build that lacks both ML-KEM and ML-DSA (e.g. via USE_LOCAL_WOLFSSL pointing at a custom build) raises AttributeError on `import wolfcrypt.random`, cascading into wolfcrypt.ciphers and effectively breaking the entire library. Hardcode the default to -2, matching the convention already used by _Hmac._init and AesGcmStream.__init__ (which call wc_HmacInit / wc_AesInit with -2 directly rather than referencing _lib.INVALID_DEVID). Add a build-no-pqc GitHub Actions job that builds wolfSSL with --disable-kyber --disable-dilithium, installs wolfcrypt-py against it via USE_LOCAL_WOLFSSL, and runs an import-smoke step plus the full pytest suite. The smoke step is the explicit regression guard: it imports wolfcrypt.random and instantiates Random(), which would fail at import time if the default ever again references a CFFI symbol that is only conditionally declared. F-2659
ChaCha._set_key ignored its `direction` argument and unconditionally re-keyed both self._enc and self._dec whenever either was allocated. Because _Cipher.encrypt and _Cipher.decrypt only call _set_key the first time their respective context is allocated, doing `encrypt(...); decrypt(...); encrypt(...)` on the same instance silently rewound the encryption stream: the first decrypt() allocated self._dec and re-keyed self._enc back to counter 0, so the second encrypt() produced ciphertext as if starting from the beginning of the keystream rather than continuing it. set_iv() relies on the both-directions reset to change the IV, so that path must be preserved. Honor the direction argument: direction _ENCRYPTION / _DECRYPTION only touches the matching context, while direction 0 (the value set_iv passes) keeps the existing reset-both behavior. Add two regression tests: one that interleaves encrypt/decrypt/encrypt and compares against a baseline two-encrypt sequence to catch any future re-introduction of cross-direction state stomping, and one that locks in set_iv's reset-both semantics so the fix is not later narrowed in a way that breaks IV changes. F-3586
wc_GetPkcs8TraditionalOffset is declared in the wolfSSL public header as taking byte* input (non-const), even though the current implementation only reads from the buffer via ToTraditionalInline_ex. RsaPrivate.__init__ was passing a Python bytes object directly into that non-const parameter on the PKCS#8 fallback path, which lets CFFI hand the C library a writable pointer into immutable storage. There is no observable mutation today, but the contract permits it, and any future wolfSSL change to actually write through the parameter would silently corrupt the caller's bytes object. Copy the key into a CFFI-owned byte array for that one call. The adjacent wc_RsaPrivateKeyDecode calls are unaffected since their cdef is already const byte*. PKCS#8 key load is a one-time cost at construction, so the extra allocation is negligible. Not changing the cdef to const byte* because the wolfSSL public header itself declares the parameter non-const; diverging from the header would cause compile warnings in the CFFI-generated glue code. If wolfSSL ever const-corrects the header upstream, this copy can be dropped. F-2664
scripts/build_ffi.py declared wc_ed448_sign_msg and wc_ed448_verify_msg
with two cdef discrepancies vs the wolfSSL header (ed448.h):
- ctx was typed `byte*` instead of `const byte*`. CFFI was therefore
happy to forward a writable pointer into Python bytes storage.
The wolfSSL functions are const-correct and do not mutate ctx, so
this had no runtime impact, but it widened the surface CFFI was
willing to permit.
- ctx_len was typed `word32` instead of `byte`. RFC 8032 caps Ed448
context at 255 bytes, and wolfSSL's prototype reflects that with a
1-byte parameter. The mismatch happens to work on x86_64 SysV
because the low byte is what the callee reads, but it is still a
real ABI discrepancy and would not be guaranteed on other
platforms or calling conventions.
Update the cdef to const byte* / byte to match the header on both
functions. Add a guard in Ed448Public.verify and Ed448Private.sign so
callers passing a ctx longer than 255 bytes get a clear ValueError
rather than relying on CFFI's narrowing behavior.
ML-DSA's wc_dilithium_sign_ctx_msg / wc_dilithium_verify_ctx_msg cdefs
were also reviewed and already match the wolfSSL header.
F-3086
scripts/build_ffi.py declared qx, qy, and d as `byte*` while the wolfSSL header (ecc.h) declares all three as `const byte*`. wolfSSL is const-correct here and does not mutate the inputs, so there is no observable bug today, but the non-const cdef let CFFI hand a writable pointer into Python bytes storage on EccPublic.decode_key_raw and EccPrivate.decode_key_raw calls. Update the cdef to match the header. F-3087
wc_ecc_import_unsigned takes no length parameters for qx/qy/d: it reads exactly curve_size bytes from each pointer based on curve_id, via mp_read_unsigned_bin in wc_ecc_import_raw_private. The Python decode_key_raw wrappers handed the user-supplied buffers straight through without any length check, so a shorter buffer caused the C library to read past the end of the Python buffer (OOB read of adjacent memory, potentially leaking it into the imported key or segfaulting). A longer buffer silently dropped the extra bytes. Add wc_ecc_get_curve_size_from_id to the CFFI cdef, then in both decode_key_raw methods t2b the inputs, look up the expected curve size, and raise ValueError if any of qx/qy/d does not match. Reject unknown curve_id values with a clear message rather than falling through to wolfSSL with a bogus size. Add test_ecc_decode_key_raw_rejects_wrong_length covering short qx, long qy, short d, unknown curve_id, and the happy path on both EccPublic and EccPrivate. F-3088
There was a problem hiding this comment.
Pull request overview
This PR fixes several correctness and safety issues in the Python bindings around cipher/PKI wrappers and CFFI signatures, and adds CI/test coverage for key regressions across different wolfSSL feature sets.
Changes:
- Fix AES-SIV associated data handling to accept common buffer types without silently changing semantics, and add a regression test.
- Fix ChaCha lazy context allocation so decrypt initialization doesn’t reset the encrypt stream state, and add regression tests.
- Align multiple CFFI cdef signatures with wolfSSL headers (RSA-PSS verify constness, ECC import constness + curve-size helper, Ed448 ctx types) and harden ECC raw key decode with length validation + tests.
- Ensure
wolfcrypt.random.Random()imports/initializes on builds without ML-KEM/ML-DSA by avoiding_lib.INVALID_DEVID, and add a CI job for that build configuration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
wolfcrypt/random.py |
Avoids reliance on _lib.INVALID_DEVID so Random() works on non-PQC builds. |
wolfcrypt/ciphers.py |
Fixes/adjusts AES-SIV AAD handling, ChaCha keying behavior, RSA PKCS8 offset handling, ECC raw decode validation, and Ed448 ctx length validation. |
tests/test_ciphers.py |
Adds regression tests for ECC raw decode validation, AES-SIV AAD buffer types, and ChaCha stream-state behavior. |
scripts/build_ffi.py |
Updates CFFI cdefs to match wolfSSL header constness and parameter types; adds ECC curve-size helper symbol. |
.github/workflows/python-app.yml |
Adds CI job building wolfSSL without PQC to prevent import regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
approved these changes
May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix the following: