Conversation
62bfa5f to
cb3e8bb
Compare
cb3e8bb to
da7a0b4
Compare
dgarske
left a comment
There was a problem hiding this comment.
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 calledRecommendation: 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 = NoneWhile 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.
| 64: _lib.wc_Sha3_512_Free, | ||
| } | ||
|
|
||
| def __del__(self): |
There was a problem hiding this comment.
BLOCK-1: Sha3.del will call None(...) on invalid digest size
- File:
wolfcrypt/hashes.py:235-237 - Function:
Sha3.__del__ - Category: bug
- Confidence: High
Description: When Sha3 is constructed with an invalid size (e.g., Sha3(size=42)), the __init__ sets self._delete = self._SHA3_FREE.get(size) which returns None for unrecognized sizes. Then self._init() returns -1, and WolfCryptError is raised. However, Python still calls __del__ on the partially-constructed object during cleanup. The guard if hasattr(self, '_delete') returns True because the attribute exists (it's just set to None), causing self._delete(self._native_object) → None(...) → TypeError to be raised during garbage collection.
Code:
def __del__(self):
if hasattr(self, '_delete'):
self._delete(self._native_object)Recommendation: Add a truthiness check:
| def __del__(self): | |
| def __del__(self): | |
| if hasattr(self, '_delete') and self._delete: | |
| self._delete(self._native_object) |
| _delete = _lib.wc_ShaFree | ||
|
|
||
| def __del__(self): | ||
| self._delete(self._native_object) |
There was a problem hiding this comment.
SUGGEST-1: _Hash.copy() may double-free internal resources with new __del__ methods
- File:
wolfcrypt/hashes.py:54-65(pre-existing) + lines 119-122, 145-148, 171-174, 197-200 (new__del__) - Function:
_Hash.copy,Sha.__del__,Sha256.__del__, etc. - Category: bug
- Confidence: Medium
Description: The copy() method creates a new hash via self.new("") (which calls wc_Init*), then overwrites its native struct via _ffi.memmove. Both the original and the copy now have identical internal byte representations. When both are garbage collected, the newly-added __del__ methods call wc_*Free() on both. If the wolfSSL hash struct contains pointers to dynamically allocated memory (e.g., in hardware-accelerated or SHA3 implementations), this would cause a double-free.
Before this diff, no Free was ever called, so the memmove-based copy pattern was safe. The addition of __del__ + Free changes that contract.
Code:
# _Hash.copy() — creates duplicate native state
def copy(self):
copy = self.new("")
_ffi.memmove(copy._native_object, self._native_object, self._native_size)
return copyRecommendation: Verify that wc_Sha*Free() functions are safe to call on memmove-duplicated structs. If not, copy() should use the proper wolfSSL copy/clone API (e.g., wc_Sha256Copy) rather than raw memmove. Same concern applies to _Hash.digest() which creates a temp memmove copy that never gets Freed.
| return _ffi.buffer(ciphertext)[:] | ||
|
|
||
| def encrypt_oaep(self, plaintext, label=""): | ||
| if not self._hash_type: |
There was a problem hiding this comment.
SUGGEST-3: Missing test coverage for encrypt_oaep/decrypt_oaep hash_type validation
- File:
wolfcrypt/ciphers.py:765-766, 958-959 - Function:
RsaPublic.encrypt_oaep,RsaPrivate.decrypt_oaep - Category: test
- Confidence: High
Description: The new guard if not self._hash_type: raise WolfCryptError(...) in both encrypt_oaep and decrypt_oaep is not covered by tests. There are no test cases that verify calling OAEP methods without a hash_type raises the expected error.
Recommendation: Add tests that construct RsaPublic/RsaPrivate without hash_type and verify that encrypt_oaep/decrypt_oaep raise WolfCryptError.
| if ret < 0: | ||
| raise WolfCryptError("Init error (%d)" % ret) | ||
|
|
||
| def __del__(self): |
There was a problem hiding this comment.
SUGGEST-4: Missing test for AesGcmStream.__del__ resource cleanup
- File:
wolfcrypt/ciphers.py:424-426 - Function:
AesGcmStream.__del__ - Category: test
- Confidence: Low
Description: The new __del__ method calls wc_AesFree to clean up AES state, which is good. However, there's no test verifying resource cleanup, and the hasattr guard is sound. This is a low-priority gap since the pattern is straightforward.
Recommendation: Consider a test that creates and immediately deletes many AesGcmStream objects to verify no resource leaks occur.
| raise WolfCryptError("Public key generate error (%d)" % ret) | ||
| ret = _lib.wc_ed25519_import_public(pubkey, self.size, | ||
| self.native_object); | ||
| if ret < 0: |
There was a problem hiding this comment.
SUGGEST-5: Ed25519/Ed448 decode_key error check added but only on else branch
- File:
wolfcrypt/ciphers.py:1536-1539, 1738-1741 - Function:
Ed25519Private.decode_key,Ed448Private.decode_key - Category: question
- Confidence: Medium
Description: The diff adds error checking after wc_ed25519_import_public / wc_ed448_import_public in the else branch (when pub is not provided). Previously the return value was silently ignored. This is a good fix. The if pub branch already had error checking via wc_ed25519_import_private_key, so no issue there.
Recommendation: No action needed — noting this is a correct fix.
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.