Fix various issues - #11029
Conversation
|
There was a problem hiding this comment.
🟡 Not ready to approve
It includes an accidentally committed .rej artifact and CMake still needs to export/check HAVE_SYS_UN_H to avoid public ABI/layout mismatches for consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR addresses multiple correctness and robustness issues across certificate name/IP checking, PKCS#7 SignedData streaming verification, and big-integer cleanup paths, with accompanying API tests and build-system adjustments to keep public header feature macros consistent between library and consumers.
Changes:
- Hardened PKCS#7 SignedData streaming parsing to avoid accepting truncated footers and to correctly error when required
signerInfosis missing; added tests to cover the cases. - Updated IP alt-name checking to respect the caller-provided length (avoid reliance on NUL termination) and added an OpenSSL-compat test for bounded inputs.
- Made several bigint cleanup paths safer by zero-initializing heap-allocated
mp_intstructs when initialization may be skipped; updated build configuration to exportHAVE_SYS_TIME_H.
File summaries
| File | Description |
|---|---|
| wolfssl/internal.h | Extends internal CheckIPAddr prototype to accept an explicit input length. |
| src/internal.c | Updates CheckIPAddr implementation and internal call sites to pass explicit lengths. |
| src/x509.c | Updates X509 host/IP checking to pass bounded lengths into IP-alt-name checks. |
| tests/api/test_ossl_x509.c | Adds a bounded (non-NUL-terminated) host-name test to verify length-respecting behavior. |
| wolfcrypt/src/pkcs7.c | Adjusts streaming stage windowing/capping to avoid stalling and to reject missing signerInfos. |
| tests/api/test_pkcs7.c | Removes NO_PKCS7_STREAM gating for a truncation test; adds a new “no signerInfos” regression test. |
| tests/api/test_pkcs7.h | Declares/registers the new PKCS#7 regression test. |
| wolfcrypt/src/srp.c | Zero-initializes heap-allocated mp_int temporaries so cleanup is safe when init is skipped. |
| wolfcrypt/src/ecc.c | Zero-initializes mp_int temporaries in a verification-faults path so cleanup is safe on early error. |
| wolfcrypt/src/rsa.c | Zero-initializes heap-allocated mp_int temporaries so cleanup is safe when init is skipped. |
| wolfcrypt/src/rsa.c.rej | Adds a patch reject artifact file in-tree (should not be committed). |
| configure.ac | Adjusts header checks and exports HAVE_SYS_UN_H/HAVE_SYS_TIME_H via CPP flags for public header layout consistency. |
| CMakeLists.txt | Ensures in-tree CMake builds propagate HAVE_SYS_TIME_H to match installed headers. |
| cmake/options.h.in | Exports HAVE_SYS_TIME_H via generated options header for consumers. |
| cmake/config.in | Removes HAVE_SYS_TIME_H from CMake-generated config header (now exported via options header). |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
7d62180 to
e03197e
Compare
|
Retest this please |
e03197e to
5c3f92e
Compare
803d3c1 to
4aba977
Compare
|
Jenkins retest this please |
4aba977 to
b66f8e8
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 9 total — 3 posted, 6 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] Streaming SignedData verifier now reports truncated 6-11 byte footers as successfully verified —
wolfcrypt/src/pkcs7.c:7513-7550 - [Low] Added mp_int zeroing in rsa.c is dead code - the failure path already frees and NULLs —
wolfcrypt/src/rsa.c:5089-5094, 5362-5369 - [Low] settings.h silently discards a detected header result with asymmetric Windows guards —
wolfssl/wolfcrypt/settings.h:410-420
Skipped findings
- [Medium]
New PKCS#7 tests do not cover the 1..11 byte residual window the change introduces - [Medium]
New residual arithmetic is unsound for indefinite-length bundles and is skipped entirely when maxLen totalRd - [Medium]
HAVE_SYS_UN_H / HAVE_SYS_TIME_H propagation is an intentional ABI change that needs a release note - [Medium]
callbacks.h typedefs struct timeval without including sys/time.h, now exposed to every application TU - [Low]
New PKCS#7 code inserted between a doc comment and the function it documents - [Low]
Bare brace block introduced solely to declare a variable
Review generated by Skoll
b66f8e8 to
f71a221
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 9 total — 6 posted, 3 skipped
6 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] Moving sys/time.h out of AC_CHECK_HEADERS silently breaks the AC_CHECK_DECLS prologue and drops HAVE_GETTIMEOFDAY from config.h —
configure.ac:224-231 - [Medium] New WC_PKCS7_VERIFY_STAGE3_FOOTER reads from in and ignores the separate footer buffer in2 —
wolfcrypt/src/pkcs7.c:7664-7680 - [Medium] New footer-parser tests are gated behind RSA/USE_CERT_BUFFERS_2048 even though most of them need neither —
tests/api/test_pkcs7.c:7063-7147 - [Medium] pkcs7_verify_chunked setup failures are indistinguishable from a successful rejection —
tests/api/test_pkcs7.c:7026-7047 - [Low] New enumerator inserted mid-enum in the public PKCS7_STATE, renumbering all later states —
wolfssl/wolfcrypt/pkcs7.h:105 - [Low] XSTRLEN(ssl-param-ipasc) computed twice in DoVerifyCallback —
src/internal.c:16071-16074
Skipped findings
- [Low]
Doc comment for test_wc_PKCS7_VerifySignedData_NoDigestParams is now detached from its function - [Info]
Stray unrelated blank line in wc_CheckProbablePrime_ex - [Info]
New brace scope opened purely to declare a test-local variable
Review generated by Skoll
8e724fa to
5d586a1
Compare
…to options.h as they are used in the library. Zero heap mp_int structs on allocation, before any error path. Thanks to Christos Papakonstantinou (Cantina Security) for the reports.
5d586a1 to
3321e43
Compare
|
The reported PKCS7 issue was fixed in #11017, so I dropped the PKCS7 changes from this branch. |
|
Jenkins retest this please |
Description
Fixes zd#22229
Testing
Built in tests, provided reproducers
Checklist