Repository navigation
Conversation
|
@claude review |
|
There was a problem hiding this comment.
🟡 Changes recommended
The new copy tests can crash or fail in supported C-only x86_64 configurations, and the documentation misleadingly suggests disabling runtime checks relaxes allocator alignment requirements.
3 open issues
What changed in this PR
This PR adds x86_64 context-alignment checks to wolfCrypt so affected initialization and copy APIs return BAD_ALIGN_E instead of risking aligned-store crashes, addressing the integration problem reported in curl.
Changes:
- Add shared alignment macros and guards across selected cryptographic APIs and backends.
- Wipe rejected misaligned contexts safely during cleanup and add regression tests.
- Document alignment errors and custom allocator requirements.
| File | Description |
|---|---|
wolfssl/wolfcrypt/types.h |
Define alignment helpers and platform guards. |
wolfcrypt/src/sha256.c |
Guard SHA-256/224 initialization, copy, and cleanup. |
wolfcrypt/src/port/kcapi/kcapi_hmac.c |
Guard KCAPI HMAC initialization and cleanup. |
wolfcrypt/src/port/kcapi/kcapi_hash.c |
Guard KCAPI SHA initialization and copy. |
wolfcrypt/src/port/devcrypto/devcrypto_hash.c |
Guard devcrypto SHA-256 initialization and copy. |
wolfcrypt/src/hmac.c |
Guard HMAC initialization, copy, and cleanup. |
wolfcrypt/src/ed25519.c |
Guard Ed25519 initialization and cleanup. |
wolfcrypt/src/curve25519.c |
Guard Curve25519 initialization and cleanup. |
wolfcrypt/src/aes.c |
Guard AES initialization and cleanup. |
tests/api.c |
Add misaligned-context regression tests. |
doc/dox_comments/header_files/sha256.h |
Document SHA initialization and copy alignment errors. |
doc/dox_comments/header_files/memory.h |
Document custom allocator alignment requirements. |
doc/dox_comments/header_files/ed25519.h |
Document Ed25519 initialization alignment errors. |
doc/dox_comments/header_files/curve25519.h |
Document Curve25519 initialization alignment errors. |
doc/dox_comments/header_files/aes.h |
Document AES initialization alignment errors. |
Review effort: Balanced
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12035
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/aes.h, doc/dox_comments/header_files/curve25519.h, doc/dox_comments/header_files/ed25519.h, doc/dox_comments/header_files/memory.h, doc/dox_comments/header_files/sha256.h
Coverage: 10 of 10 in-scope changed file(s) opened by the reviewer
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Max
0f05edf to
f18ce07
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12035
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): .wolfssl_known_macro_extras, doc/dox_comments/header_files/aes.h, doc/dox_comments/header_files/curve25519.h, doc/dox_comments/header_files/ed25519.h, doc/dox_comments/header_files/memory.h, doc/dox_comments/header_files/sha256.h
Coverage: 3 of 4 in-scope changed file(s) opened by the reviewer; not opened: tests/api.c
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
wc_Sha256, Aes, Hmac, curve25519_key and ed25519_key are 16-byte aligned types in default builds. A caller that places one at an offset that is only 8-byte aligned, as curl's Curl_HMAC_init() did by putting the hash contexts after a 24-byte header, crashes on x86_64: the compiler zeroes the context with aligned SSE stores and the CPU faults. Add WC_ALIGNOF() and WC_IS_ALIGNED() to types.h, plus WC_CTX_IS_ALIGNED(), which checks a context pointer against its type's alignment capped at 16, the alignment malloc() guarantees on x86_64. WC_ALIGNOF() prefers C11 _Alignof, since GCC's __alignof__ reports the preferred rather than the required alignment on some targets. The init functions for these types now return BAD_ALIGN_E before touching a misaligned context, following the existing BAD_ALIGN_E checks on AES-NI and MMCAU buffers. This includes the crypto callback only SHA-256/SHA-224 init, the KCAPI SHA-256, SHA-224 and HMAC init and the devcrypto SHA-256 init, whose contexts keep the aligned fields. The AF_ALG SHA-256 init and copy and the SE050 SHA-256/SHA-224 init check as well: their contexts are not aligned types, but they share the generic free, which returns early on a context below its type's alignment and would leak their sockets or SE050 state. KCAPI wc_HmacSetKey() checks too, so an Hmac keyed without init never owns a handle its free skips. PSA has its own hash free and needs no check. wc_Sha256Copy(), wc_Sha224Copy() and wc_HmacCopy(), including the KCAPI and devcrypto versions, reject a misaligned destination the same way. A rejected context is left unmodified. The matching free functions wipe a misaligned context with ForceZero(), which does not assume the type's alignment, and return without touching its fields, so the usual cleanup that calls free after a failed init does not run on uninitialized memory. Before this change that memory was at least zeroed by init, and builds whose compiler emits unaligned stores did not crash. The wipe keeps key material from a context that was keyed without init, such as an Aes passed straight to wc_AesSetKey(). The check is limited to x86_64 user space, the only place a misaligned context faults. Cortex-M builds that force WOLFSSL_USE_ALIGN get less than 16-byte alignment from malloc(), so a check on other CPUs would reject contexts wolfSSL allocates itself. Kernel builds compile without SSE code generation and are excluded as well. So are freestanding builds (__STDC_HOSTED__ is 0), whose RTOS or bare-metal malloc() may align to 8 only, and static memory builds, whose blocks are only as aligned as the configured bucket sizes. Define WC_NO_CTX_ALIGN_CHECK to disable it; that removes the check, not the alignment the types need. The wolfSSL_SetAllocators() doxygen states the 16-byte requirement. The API test covers init and copy into a misaligned context, and checks that a rejected context is left untouched by init and copy and zeroed by free. The copy cases run only for types aligned to 16, since copy frees an accepted destination first. It is skipped for FIPS and selftest builds, which take these init functions from frozen sources without the check. Forming a misaligned pointer is already undefined behavior, so this is best effort; current gcc and clang keep the check.
f18ce07 to
43cd172
Compare
|
Jenkins retest this please |
Description
On x86_64,
wc_Sha256,Aes,Hmac,curve25519_keyanded25519_keyare 16-byte aligned types, and the compiler may zero them with aligned SSE stores. A context at an 8-byte offset, as in curl's HMAC code (curl/curl#23690), crashes inside init; init now returnsBAD_ALIGN_Einstead.Changes
WC_ALIGNOF,WC_IS_ALIGNEDandWC_CTX_IS_ALIGNEDto types.hForceZeroand returnWC_NO_CTX_ALIGN_CHECK_Alignof,__alignof__,__alignof) compile the check out;ALIGN16is empty there too, so the types are not over-alignedBAD_ALIGN_Ein doxygen and the 16-byte allocator requirement inwolfSSL_SetAllocatorsLimitations
Testing
test_wc_CtxInit_misalignedin tests/api.c runs init, copy and free on contexts 8 bytes past a 16-byte boundary