Fenrir fixes

[JeremiahM37]

Fixes F-927, F-928, F-929, F-930, F-931, F-932, F-933, F-934, F-937, F-938, F-939, F-940, F-941, F-942, F-1127, F-1129, F-1130, F-1131, F-1192, F-1193, F-1194, F-1195, F-1546, F-1547, F-1548, F-1550, F-1551.

Added a few unit tests for test coverage.

[dgarske]

SUGGEST-2: _Hash.digest() temp copy is never Freed

• File: wolfcrypt/hashes.py:79-97
• Function: _Hash.digest
• Category: bug
• Confidence: Medium

Description: digest() creates a temporary native object via obj = _ffi.new(self._native_type), copies state with memmove, and calls _final. This raw CFFI object is not wrapped in a Python hash class instance, so the new __del__ → wc_*Free() is never called on it. If the hash struct holds resources that wc_*Free would release (beyond what _final already does), this is a resource leak.

Code:

obj = _ffi.new(self._native_type)
_ffi.memmove(obj, self._native_object, self._native_size)
ret = self._final(obj, result)
# obj goes out of scope — wc_*Free never called

Recommendation: Call the appropriate wc_*Free() on the temporary object after _final, or use a proper wolfSSL copy function.

NIT-1: AesGcmStream class-level mutable default attributes

• File: wolfcrypt/ciphers.py:400-402
• Function: AesGcmStream
• Category: convention
• Confidence: High

Description: _aad, _tag_bytes, and _mode are defined as class-level attributes:

_aad = bytes()
_tag_bytes = 16
_mode = None

While bytes() and None are immutable so they're safe, this pattern can be confusing. The __init__ already sets instance-level _tag_bytes, but _aad and _mode rely on class-level defaults. Since _aad is reassigned (never mutated), this works but is inconsistent.

Recommendation: Initialize _aad and _mode in __init__ for clarity.

[dgarske]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 7 total — 6 posted, 2 skipped

Posted findings

• [Medium] HMAC digest() calls wc_HmacFree on memmove'd temporary — potential use-after-free — wolfcrypt/hashes.py:104-111
• [Medium] HMAC copy() creates memmove'd clone — both original and copy share resources, both have __del__ — wolfcrypt/hashes.py:54-71
• [Low] ChaCha size parameter is now completely unused — wolfcrypt/ciphers.py:507
• [Medium] Missing test coverage for _Hmac resource management lifecycle — tests/test_hashes.py
• [Low] Hash __del__ methods lack _native_object existence guard — wolfcrypt/hashes.py:138-139
• [Info] AesGcmStream class-level attributes moved to instance but comment doesn't explain why — wolfcrypt/ciphers.py:396-410

Skipped findings

• [Low] Sha3 __del__ guards _delete but not _native_object
• [Medium] Missing test coverage for _Hmac resource management lifecycle

Review generated by Skoll via openclaw

[dgarske]

🟡 [Medium] HMAC digest() calls wc_HmacFree on memmove'd temporary — potential use-after-free
💡 SUGGEST bug

The PR adds _delete = _lib.wc_HmacFree and __del__ to _Hmac (line 349-352). The inherited _Hash.digest() method creates a temporary copy of the native object. For hash classes (Sha, Sha256, etc.) the _copy function is used (deep copy, safe to free). However, _Hmac does NOT define _copy, so the memmove fallback is used (shallow byte copy). The finally block then calls wc_HmacFree(obj) on this shallow copy. If the Hmac struct contains pointers to heap-allocated resources (e.g., in FIPS, async, or hardware crypto builds), freeing the temporary invalidates those resources in the original object too, causing use-after-free on subsequent operations. Before this PR, the temporary was never freed (a leak, but safe). The trade is a leak for a potential corruption.

Suggestion:

Suggested change

	try:
	Consider adding a wolfSSL `wc_HmacCopy`-equivalent if one is available, or skip calling `_delete` when `_copy` is not available (i.e., when memmove was used). For example:
	```python
	try:
	ret = self._final(obj, result)
	if ret < 0:
	raise WolfCryptError("Hash finalize error (%d)" % ret)
	finally:
	# Only free if we used _copy (deep copy); memmove'd copies share resources
	if hasattr(self, '_copy'):
	delete = getattr(self, '_delete', None)
	if delete:
	delete(obj)

[JeremiahM37]

digest() now only calls _delete on the temp when _copy was used

[dgarske]

🟡 [Medium] HMAC copy() creates memmove'd clone — both original and copy share resources, both have __del__
💡 SUGGEST bug

The PR adds __del__ → wc_HmacFree to _Hmac. The inherited _Hash.copy() calls self.new("") which runs wc_HmacInit on the new object, then memmove overwrites it (since _Hmac has no _copy). This: (a) leaks any resources allocated by wc_HmacInit for the copy (overwritten by memmove), and (b) creates two objects that share the same internal resources. When both are garbage collected, wc_HmacFree is called on each — a potential double-free. The existing test at test_hashes.py:176 exercises this path (copy = hash_obj.copy()) for HMAC, so it works in standard builds where Hmac has no heap pointers, but it is architecturally fragile.

Suggestion:

Suggested change

	"""
	If wolfSSL does not provide `wc_HmacCopy`, consider implementing a Python-level copy that re-initializes and re-feeds the data, or document that memmove is safe for all supported Hmac configurations.

[JeremiahM37]

When copy() falls back to memmove (no _copy available), the copy is marked with _shallow_copy = True and its del skips the _delete call to prevent double-free of shared resources.

[dgarske]

🔵 [Low] ChaCha size parameter is now completely unused
💡 SUGGEST api

The PR changes ChaCha.__init__ to validate and store the actual key length (len(self._key)) instead of the size parameter. This is correct behavior, but the size parameter remains in the function signature (def __init__(self, key="", size=32)) and is never referenced. Callers passing size=16 with a 32-byte key would silently get key_size=32 (the actual length) instead of the old behavior (key_size=16). The parameter should be deprecated or removed for clarity.

Suggestion:

Suggested change

	def __init__(self, key="", size=32):
	Either remove `size` from the signature (breaking change) or add a deprecation warning when `size` is explicitly passed. At minimum, add a comment explaining why it's kept.

[JeremiahM37]

Added a comment explaining size is kept for backwards compatibility

[dgarske]

🔵 [Low] Hash __del__ methods lack _native_object existence guard
🔧 NIT convention

The __del__ methods on Sha, Sha256, Sha384, Sha512, and _Hmac directly access self._native_object without guarding against AttributeError. If __init__ raises before self._native_object is assigned, __del__ will raise during garbage collection. By contrast, AesGcmStream.__del__ in ciphers.py correctly uses hasattr(self, '_native_object'), and Sha3.__del__ uses getattr(self, '_delete', None). The pattern is inconsistent.

Suggestion:

Suggested change

	def __del__(self):
	```python
	def __del__(self):
	if hasattr(self, '_native_object'):
	self._delete(self._native_object)

[JeremiahM37]

All hash class del methods now use hasattr(self, '_native_object') for consistency with AesGcmStream and Sha3

[dgarske]

⚪ [Info] AesGcmStream class-level attributes moved to instance but comment doesn't explain why
🔧 NIT style

The PR removes class-level attributes _aad, _tag_bytes, and _mode and moves them to instance-level initialization in __init__. This is a cleaner pattern, but the removed class attributes (_aad = bytes(), _tag_bytes = 16, _mode = None) were actually safe as defaults since they're immutable/None and would be shadowed by instance assignments. A brief inline comment noting these are per-instance state would help readability.

Recommendation: Minor: consider adding a brief comment noting these are intentionally instance attributes.

[JeremiahM37]

Added an inline comment noting these are per-instance state