Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: none
Failed targets: wolfcrypt-src, wolfcrypt-bugs
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Error: CalledProcessError
6c11cd5 to
5a03400
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
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.
5a03400 to
667312f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
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.
667312f to
8b7f6a8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
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.
|
Jenkins retest this please |
8b7f6a8 to
e364887
Compare
|
Jenkins retest this please |
e364887 to
cc50474
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
ParseCert() guarded its RSA public-key copy with !WOLFSSL_NO_MALLOC, while StoreKey() guards the equivalent copy for every non-RSA key with !WC_ASN_NO_HEAP. Those are not the same condition: WC_ASN_NO_HEAP is auto-defined only when WOLFSSL_NO_MALLOC and NO_WOLFSSL_MEMORY are set without XMALLOC_USER or WOLFSSL_STATIC_MEMORY, so a static-memory build defines WOLFSSL_NO_MALLOC yet still has a working allocator. In such a build the copy was skipped, cert->pubKeyStored stayed 0, and FillSigner() therefore never populated signer->publicKey/pubKeySize. ParseCertRelative() then passed a NULL key and a zero key size to ConfirmSignature(), which rejects them with BAD_FUNC_ARG before its WOLFSSL_ENTER. The effect was that no certificate issued by an RSA CA could be verified against it - wolfSSL_CertManagerVerifyBuffer() and TLS peer validation alike - while ECC, Ed25519, Ed448 and ML-DSA CAs worked, because those keys travel through StoreKey(). Use WC_ASN_NO_HEAP in all three guards, including the one on the ptr declaration. FreeDecodedCert() and FreeSigner() already key off pubKeyStored, so ownership and freeing are unchanged. Point the MC/DC white-box guard for this block at WC_ASN_NO_HEAP too. It still keyed off WOLFSSL_NO_MALLOC, so in a static-memory build the copy is now compiled and executed while the section covering it fell back to its stub, and the coverage claim was inaccurate for the one configuration this fixes.
cc50474 to
1123d4a
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
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.
1123d4a to
e702498
Compare
|
Jenkins retest this please |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
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.
With WOLFSSL_NO_MALLOC there is no system heap to fall back on, so an allocation the compatibility layer makes with a NULL heap has to come out of the static pool the test loads. The test only nominated that pool as the global heap hint when OPENSSL_EXTRA was off, so in the combination of the two those allocations had no source at all and openssl_test() failed. Set the hint in that combination too, and give it a pool sized for the compatibility layer on top of the algorithm tests. gTestMemory was sized for the wolfCrypt tests alone and those allocations exhausted it - first in wolfSSL_CRYPTO_malloc(), then, as the pool was enlarged, further along in wolfSSL_X509_load_certificate_file(). Every other configuration keeps the size it had. The new arm goes ahead of the FrodoKEM and ML-DSA arms rather than after them, because those fire first for exactly the builds that need the most. With --enable-mldsa the 576 KB arm won and the RSA test died with -125, and 1 MB only moved the failure to openssl_pkey1_test(), where an exhausted pool surfaces as a NULL from wolfSSL_X509_get_pubkey() and no error code at all. FrodoKEM fails the same way on its own 1 MB arm. Both pass at 2 MB, so the arm asks for 2 MB when either is enabled and 1 MB otherwise. Claim the hint only when it is unset, and drop it only while it is still ours. Nothing in tree installs one before wolfcrypt_test() runs, but under OPENSSL_EXTRA the hint is never handed back, so a program embedding wolfcrypt_test() as a smoke test would otherwise lose its own pool for the rest of the process. This is the first-wins rule the examples already use. The matching teardown stays restricted to !OPENSSL_EXTRA on purpose. gTestMemory has static storage duration, so the hint cannot dangle, and testsuite runs the echo server after wolfcrypt_test() returns: the echo server has no pool of its own, and this is the one that outlives it. testsuite/testsuite.test now passes with --enable-staticmemory -DWOLFSSL_NO_MALLOC. scripts/unit.test still fails there, at seven API tests this change does not address. BENCH_EMBEDDED gets a #error rather than the enlarged pool. That combination is self-contradictory - openssl_pkey0_test() is not gated on BENCH_EMBEDDED, so it still asks for about 1 MB, which is not something to hand a target that has declared itself embedded. Refusing at build time with the override named beats either silently reserving a megabyte or failing at run time in RSA_new(), which is what a 14 KB pool does today. WOLFSSL_STATIC_MEMORY_TEST_SZ remains the override and is checked first, so such a target can still pick its own size. The arm is also restricted to !WOLFCRYPT_ONLY. The compatibility layer tests are gated on !WOLFCRYPT_ONLY themselves, so a crypt-only build never runs openssl_pkey0_test() and has no reason to reserve a megabyte for it - and with BENCH_EMBEDDED it met the #error above over a test it does not compile. Crypt-only keeps the size it had. Pin the behaviour the asn.c fix restores while it is here: cert_no_malloc_test() only asserted the in-place layout under WC_ASN_NO_HEAP, so nothing covered the copy-out side. Add the mirror assertion - pubKeyStored set, publicKey outside the source DER. It fails on the pre-fix guard and passes after it, and unlike fill_signer_twice_test() it is not gated on NO_FILESYSTEM, which a real static-memory target turns off. Carry ParseCert()'s whole predicate rather than WC_ASN_NO_HEAP alone: the copy-out is also skipped under NO_WOLFSSL_CM_VERIFY without WOLFSSL_DYN_CERT, and the assertion must not claim otherwise there.
With WOLFSSL_NO_MALLOC there is no allocator behind a NULL-heap XMALLOC, so an allocation made outside any CTX or SSL object can only be served from the global heap hint. wolfSSL_Init() makes such an allocation: under OPENSSL_EXTRA it seeds the compatibility-layer RNG, whose _InitRng() call allocates with a NULL heap. That returned NULL, wolfSSL_Init() reported WC_INIT_E, and every wolfSSL_CTX_new_ex() that triggered the lazy init failed, so neither example could establish a connection even though the pool it had loaded was large enough. Have each example nominate its own pool, which is what the hint is for. The server could not do that before: it created its CTX through wolfSSL_CTX_load_static_memory(), which loads the pool and creates the CTX in one call, so wolfSSL_Init() ran before the caller ever saw the hint. Load the pool with wc_LoadStaticMemory() and create the CTX afterwards, as the client already does. Claim the hint only when none is set, and drop it again on the way out. Both pools are local to the example's own function, and testsuite runs the wolfCrypt test, both examples and the echo server in one process, so an example that overwrote the hint would leave it pointing at a pool that dies the moment the example returns. scripts/resume.test, scripts/tls13.test and testsuite/testsuite.test go from failing to passing with --enable-staticmemory -DWOLFSSL_NO_MALLOC. Restrict the claim to the standalone programs (!NO_MAIN_DRIVER). testsuite and unit.test compile both examples with NO_MAIN_DRIVER and run server_test on a spawned thread beside client_test, so an in-harness claim would publish one thread's automatic-storage pool as the process allocator, let the other thread allocate from it, and then revoke it when the owning frame unwound. In those builds the harness's own long-lived pool is the one that belongs in the hint. Check ctx after wolfSSL_CTX_new_ex() rather than letting the following IO-pool load report a CTX allocation failure as "unable to load static memory".
The no-malloc workflow never built OPENSSL_EXTRA, so nothing in CI reached the allocations this series fixes: all five configs omit --enable-opensslextra, and the two that do enable static memory stop at the algorithm tests. The pool sizes were verified by hand, which leaves them free to drift as footprints grow, and an exhausted pool surfaces as wolfSSL_X509_get_pubkey() returning NULL with no error code. Add two entries. opensslextra-staticmemory is the configuration the series targets. cryptonly-opensslextra-embedded pins the crypt-only exclusion: without it the BENCH_EMBEDDED #error refuses a build whose compatibility tests are not compiled, so a regression there is a build failure rather than a silent over-reservation. Both run testwolfcrypt rather than make check, like the entries beside them: scripts/unit.test still fails seven API tests in this configuration for reasons this series does not address.
e702498 to
e50186f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
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.
| /* No malloc, so the compatibility layer's NULL-heap allocations come | ||
| * out of this pool on top of what the algorithm tests need. */ | ||
| #ifdef BENCH_EMBEDDED | ||
| #error "openssl_pkey0_test() needs about 1MB of pool, far more \ |
There was a problem hiding this comment.
Embedded compatibility builds without RSA are rejected · Logic errors
The new BENCH_EMBEDDED error also fires with NO_RSA, although openssl_pkey0_test() and openssl_pkey1_test() compile their large RSA bodies out. This rejects a supported reduced build before tests run.
Related earlier finding on this pull request (similar but distinct): Both use the BENCH_EMBEDDED/OpenSSL rejection branch, but -2 concerns WOLFCRYPT_ONLY omitting all compatibility tests; this concerns NO_RSA/NO_SHA compiling PKEY test bodies out. The affected configuration and required conditional exclusion differ.
Suggested fix: Gate the rejection on !defined(NO_RSA) && !defined(NO_SHA) to match the PKEY test bodies.
| ret = WC_TEST_RET_ENC_NC; | ||
| } | ||
| } | ||
| #elif !defined(NO_WOLFSSL_CM_VERIFY) || defined(WOLFSSL_DYN_CERT) |
There was a problem hiding this comment.
RSA copy-out assertion is absent from the target CI build · Missing edge-case coverage on a function the PR also changed
cert_no_malloc_test() is omitted from the new allocator-backed CI build because it defines neither WOLFSSL_TEST_CERT nor USE_CERT_BUFFERS_2048; the no-heap entry takes the other arm, so this assertion is never exercised.
Related earlier finding on this pull request (similar but distinct): Both concern the RSA copy-out assertion in cert_no_malloc_test, but -1 fixes an incorrect assertion predicate under NO_WOLFSSL_CM_VERIFY, while this finding concerns CI flags that never exercise the assertion. The faulting operation and patch are distinct.
Suggested fix: Enable WOLFSSL_TEST_CERT and USE_CERT_BUFFERS_2048 in the allocator-backed static-memory CI job.
4 participants
WOLFSSL_NO_MALLOCremoves only the stdio malloc fall-back, so a build that also has--enable-staticmemorystill has a working allocator. Two places treat that configuration as having no allocator at all: one makes every RSA CA useless for chain verification, the other stops any TLS connection being established. They are independent and sit in different files. A third commit sizes the wolfCrypt test pool somake checkexercises the configuration.Found while running wolfCert's CI matrix, which builds it.
1. RSA public key never reaches the CA Signer
ParseCert()guarded its RSA public-key copy with!defined(WOLFSSL_NO_MALLOC), whileStoreKey()guards the equivalent copy for every non-RSA key with!defined(WC_ASN_NO_HEAP). Only the latter means "genuinely no allocator", which is what the copy depends on, so a static memory build skipped it despite having somewhere to copy to.cert->pubKeyStoredtherefore stayed 0,FillSigner()never populatedsigner->publicKey/pubKeySize, andConfirmSignature()was handed a NULL key and zero size, which it rejects withBAD_FUNC_ARG. No certificate issued by an RSA CA could be verified against it, throughwolfSSL_CertManagerVerifyBuffer()or TLS peer validation alike. ECC, Ed25519, Ed448 and ML-DSA CAs were unaffected, because those keys travel throughStoreKey().The fix uses
WC_ASN_NO_HEAPin all three guards inParseCert(), including the one on thechar* ptrdeclaration.FreeDecodedCert()andFreeSigner()already key offpubKeyStored, so ownership is unchanged.2. Loading a static pool never set the global heap hint
An allocation made outside any
WOLFSSL_CTXorWOLFSSLobject can only be served from the global heap hint, and loading a pool never set it.wolfSSL_Init()makes exactly such an allocation: underOPENSSL_EXTRAit seeds the compatibility layer RNG with a NULL heap. That returned NULL,wolfSSL_Init()reportedWC_INIT_E, and everywolfSSL_CTX_new_ex()that triggered the lazy init failed, so no TLS connection could be established at all regardless of pool size.The two bundled examples reach this from different entry points, the server through
wolfSSL_CTX_load_static_memory()and the client through a directwc_LoadStaticMemory(), so the fix goes inwc_LoadStaticMemory_ex(), which both funnel through. It applies only when no hint has been set yet, so an application managing the hint itself keeps control.Pools loaded with
WOLFMEM_IO_POOLorWOLFMEM_IO_POOL_FIXEDare skipped.wc_partition_static_memory()puts every chunk of such a pool on the heap's io list and leaves the general buckets empty, so adopting one would give a global allocator that fails every general request, and would then stop a general pool loaded afterwards from taking the hint. Load order does not matter.wc_UnloadStaticMemory()now clears the hint when it refers to the heap being unloaded. The hint lives inside the caller's pool buffer, which the caller may reuse once the pool is gone, and the mutex it reaches through has just been destroyed. That part is not gated onWOLFSSL_NO_MALLOC: an application setting the global hint by hand has always been able to leave it dangling the same way.Reviewers may recall 98a19f9, which removed commented-out
wolfSSL_SetGlobalHeapHint()calls from both examples because the pool "does not handle all memory used on default build". That concern does not apply here: the change is confined toWOLFSSL_NO_MALLOC, where the alternative to serving those allocations from the pool is failing them outright. Builds with a system heap are unaffected.3. wolfCrypt test pool sized for the configuration
Once the pool serves the compatibility layer's NULL-heap allocations,
gTestMemorywas too small andopenssl_test()failed on exhaustion. TheWOLFSSL_NO_MALLOCandOPENSSL_EXTRAcombination now gets 1 MB; every other configuration keeps the size it had.Testing
With
--enable-staticmemory -DWOLFSSL_NO_MALLOC,make checkgoes from 0 of 5 passing to 3 of 5:resume.test,tls13.testandtestsuiteall move from fail to pass. A standalonewolfSSL_CertManagerVerifyBuffer()on an RSA CA in that build goes from -173 toWOLFSSL_SUCCESS, and wolfCert's integration suite passes against it with its local workaround for defect 1 removed.--enable-staticmemorywithoutWOLFSSL_NO_MALLOC- the configuration the unload change newly touches - passesmake check5 of 5. The default configuration passesmake checkandtestwolfcrypt, and a true no-allocator build (-DWOLFSSL_NO_MALLOC -DNO_WOLFSSL_MEMORY) compiles with behaviour unchanged by design. Both predicates in 1 and 2 are unchanged for those two cases, so onlyWOLFSSL_NO_MALLOCbuilds that do have an allocator change behaviour.Two failures deliberately left in place
unit.testreaches a structural limit rather than a sizing one:test_wc_ChaCha20Poly1305_Encrypt_Decrypt_Big()asks for a single 16384 byte buffer whileLARGEST_MEM_BUCKETis 16128 for this feature set. A static pool is partitioned into fixed size buckets, so no pool size satisfies that request, and raising the ceiling changes the layout for every static memory user.pem.testfails becauseexamples/pem/pem.cnever loads a pool at all, so itsXMALLOC(..., NULL, ...)has no source in a build with no system heap.Neither is a regression.