Repository navigation
Conversation
|
d9375ce to
5cbb30a
Compare
5cbb30a to
3f83509
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
ASN.1 representation detection can corrupt valid raw values, and several new bounds checks remain incomplete.
Review effort: Balanced
Findings: 2
Open (6)
Validate curve_idx upper bounds before indexing ecc_sets · New Validate INTEGER length and data bounds before decoding or copying · New Avoid misclassifying raw INTEGER values as DER · New Test custom-curve rejection paths for point operations · New Fix grammatical predicate in test comment · New Add DSA tests for null and short digests · New
What changed in this PR
Strengthens OpenSSL compatibility APIs against malformed ASN.1, PKCS#8, DSA, and EC inputs.
Changes:
- Adds DSA and EC input validation.
- Preserves and bounds-checks PKCS#8 header offsets.
- Expands ASN.1 and EVP regression coverage.
| File | Description |
|---|---|
wolfcrypt/src/evp.c |
Validates DSA inputs and tracks PKCS#8 offsets. |
src/ssl_asn1.c |
Handles DER and raw ASN.1 integer data. |
src/pk.c |
Bounds-checks PKCS#8 header offsets. |
src/pk_ec.c |
Rejects unsupported EC curve indices. |
tests/api/test_ossl_asn1.c |
Extends ASN.1 integer tests. |
tests/api/test_evp_pkey.c |
Adds PKCS#8 offset regression tests. |
tests/api/test_evp_pkey.h |
Registers new EVP tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3f83509 to
358a7dd
Compare
b15c985 to
397c2c7
Compare
|
Jenkins: retest this please |
397c2c7 to
a74ae15
Compare
|
rebased to latest master |
|
@claude review |
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/x509.c— Apps reading a certificate's pathLenConstraint with ASN1_INTEGER_get(bc->pathlen) now always get -1, including pathlen:0 in WOLFSSL_QT/HAPROXY builds, which got 0 before. The pathlen producers at x509.c:965 and x509.c:2690 store the value inlengthover zeroed intData, with dataIsRaw = 0. The new helper treats that as malformed DER. Fix: make every pathlen producer write a real INTEGER (wolfSSL_ASN1_INTEGER_set) and read it back with ASN1_INTEGER_get, keeping x509.c:1642, x509.c:2197 and the tests that read pathlen->length in step. Otherwise the asn_public.h claim that every producer writes DER does not hold.Why this was flagged
wolfSSL_X509_get_ext_d2i(BASIC_CONSTRAINTS) builds bc->pathlen at x509.c:2684-2690 with wolfSSL_ASN1_INTEGER_new() and
a->length = (int)x509->pathLength. data points at an all-zero intData and dataIsRaw stays 0. X509_get_ext / ext->obj->pathlen does the same at x509.c:965, and wolfSSL_ASN1_INTEGER_dup copies it at x509.c:2198. Several tests confirm this length-holds-the-value convention: test_ossl_x509_ext.c:1266, 1285 and 1332, and test_ossl_asn1.c:981. The new wolfSSL_ASN1_INTEGER_get returns WOLFSSL_FATAL_ERROR when length == 0, which is pathlen:0. For other lengths, wolfssl_asn1_integer_value (ssl_asn1.c:1050) sees data[0] = 0, not ASN_INTEGER, rejects it as DER, and to_BN fails, so the result is -1. On the base branch under WOLFSSL_QT/WOLFSSL_HAPROXY, to_BN fell back to idx=0, len=length and returned 0, which was correct for the common pathLenConstraint 0. Qt reports this value as pathLenConstraint, and -1 conventionally means no limit. wolfSSL_ASN1_INTEGER_cmp on two such objects also changes: equal pathlens compared 0 on base and now return -1.Verification: nit. Trigger: a WOLFSSL_QT/WOLFSSL_HAPROXY build where an app calls ASN1_INTEGER_get(bc->pathlen) on a certificate with pathLenConstraint 0. src/x509.c:2690 stores the pathlen in
lengthand leaves the data zeroed. On the base, ASN1_INTEGER_get returned 0. After this PR, wolfSSL_ASN1_INTEGER_get checkselse if (a->length == 0) { ret = WOLFSSL_FATAL_ERROR; }, so pathlen 0 now returns -1. -
🟣
src/ssl_asn1.c— Pre-existing: printing or DER-encoding a CRL revoked serial, or a Qt/HAProxy X509_get_serialNumber() result, still gives garbage.wolfSSL_i2d_ASN1_INTEGERandwolfSSL_i2a_ASN1_INTEGER(ssl_asn1.c:1747) still assume a DER layout and ignoredataIsRaw. Fix: route both through wolfssl_asn1_integer_value(). i2d should write tag+length+value when dataIsRaw is set. i2a should hex-encode the value octets from the returned idx/len.Why this was flagged
RevokedCertToRevoked (x509.c:11882) and X509_get_serialNumber under WOLFSSL_QT/WOLFSSL_HAPROXY (x509.c:12053) set dataIsRaw = 1, so data holds only the value octets. wolfSSL_i2d_ASN1_INTEGER (ssl_asn1.c:1482) copies a->data/a->length unchanged as if it were a full DER INTEGER, so its output has no tag or length. wolfSSL_i2a_ASN1_INTEGER (ssl_asn1.c:1747) starts at idx=1 and calls GetLength on data[1]. For a raw serial 01 05 that means 0x05 is read as a length, so the call fails or skips the first value byte. The PR added the flag and moved get_length, get0_data, cmp, i2c and to_BN onto the helper but left these two encoders out. Raw-layout objects therefore still serialize and print wrongly, exactly as on the base branch.
Verification: Pre-existing. Trigger: an application calls wolfSSL_i2d_ASN1_INTEGER or wolfSSL_i2a_ASN1_INTEGER on a serial whose
dataholds only the value octets.wolfSSL_i2d_ASN1_INTEGER(ssl_asn1.c:1482) writes no 0x02 tag or length.wolfSSL_i2a_ASN1_INTEGER(ssl_asn1.c:1747) treats data[1] as a DER length. The base already stored bare values, so it fails the same way.
a74ae15 to
6442b14
Compare
|
@claude review |
There was a problem hiding this comment.
🟡 Changes recommended
Active issues include unequal comparisons of equivalent ASN.1 integers, ECC test link and assertion failures in certain no-software-ECC configurations, and a DSA test that incorrectly expects success when the digest is below the configured minimum size.
3 open issues
6 resolved since last review
Validate INTEGER length and data bounds before decoding or copying Validate curve_idx upper bounds before indexing ecc_sets Avoid misclassifying raw INTEGER values as DER Add DSA tests for null and short digests Fix grammatical predicate in test comment Test custom-curve rejection paths for point operations
Review effort: Balanced
6442b14 to
ef8a533
Compare
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved helper symbol breaks a supported build configuration, the new INTEGER layout rejects library-produced values, and the DSA baseline test fails with a supported digest-size override.
3 open issues
3 resolved since last review
Review effort: Balanced
03097bb to
e34233c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The integer comparison fallback equates distinct numeric limits by storage length when parsing fails, breaking comparison equivalence and potentially misidentifying path length constraints.
1 open issue
3 resolved since last review
Review effort: Balanced
Don't support custom curves in EC code - add checks. Maintain PKCS#8 header offset in PK and EVP layers. wolfSSL_i2c_ASN1_INTEGER: validate the input data before getting length EVP DSA: validate inputs before sign and verify
e34233c to
8297dc9
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive ASN.1 layout and private-key export changes span multiple feature configurations, requiring maintainer review and sanitizer-backed validation across layouts and reduced-feature builds.
No open issues 🎉
1 resolved since last review
Review effort: Balanced



Description
Don't support custom curves in EC code - add checks.
Maintain PKCS#8 header offset in PK and EVP layers.
wolfSSL_i2c_ASN1_INTEGER: validate the input data before getting length EVP DSA: validate inputs before sign and verify
Thanks to Anthropic for the reports.
Testing
./configure --disable-shared --enable-opensslall --enable-all
Tests add for some fixes.