Allow serial number 0 for root CA certificates#9567
Allow serial number 0 for root CA certificates#9567jackctj117 wants to merge 11 commits intowolfSSL:masterfrom
Conversation
kareem-wolfssl
left a comment
There was a problem hiding this comment.
Changes look good.
Can you add some test cases with a CA and leaf cert with a serial of 0?
|
@kareem-wolfssl it looks like the failures are looking for an expected failure due to a self-signed CA certificate with serial number 0. |
Yes, you will need to update the failing test |
|
@jackctj117 looks like some valid unit test failures you will need to look into. All related to |
|
Jenkins retest this please. History lost. |
1 similar comment
|
Jenkins retest this please. History lost. |
|
Jenkins retest this please |
1 similar comment
|
Jenkins retest this please |
|
Jenkins retest this |
|
Jenkins retest this please |
tests/api/test_asn.c
Outdated
| ExpectIntNE(wolfSSL_CertManagerVerify(cm, eeSerial0File, | ||
| WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); |
There was a problem hiding this comment.
This should test for the expected error code.
tests/api/test_asn.c
Outdated
| ExpectIntNE(wolfSSL_CertManagerLoadCA(cm, selfSignedNonCASerial0File, NULL), | ||
| WOLFSSL_SUCCESS); |
There was a problem hiding this comment.
Same as above, check for the expected error code.
|
Jenkins retest this please. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
wolfcrypt/src/asn.c:23904
- The log message for
cert->serialSz == 0is confusing: it suggests a certificate might legitimately have “no serial number”, but X.509 certificates always have a serial number and RFC 5280 requires it to be present. Consider rewording to clearly state that an empty/zero-length serial is invalid, and (if appropriate in this file) emit the verbose error macro consistently with other parse failures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if !defined(WOLFSSL_NO_ASN_STRICT) && !defined(WOLFSSL_PYTHON) && \ | ||
| !defined(WOLFSSL_ASN_ALLOW_0_SERIAL) | ||
| /* Check for serial number of 0. RFC 5280 section 4.1.2.2 requires | ||
| * positive serial numbers. However, allow zero for self-signed CA | ||
| * certificates (root CAs) being loaded as trust anchors since they | ||
| * are explicitly trusted and some legacy root CAs in real-world | ||
| * trust stores have serial number 0. */ | ||
| if ((ret == 0) && (cert->serialSz == 1) && (cert->serial[0] == 0)) { | ||
| if (!(type == CA_TYPE && cert->isCA && cert->selfSigned) | ||
| #ifdef WOLFSSL_CERT_REQ | ||
| && !cert->isCSR | ||
| #endif | ||
| ) { | ||
| WOLFSSL_MSG("Error serial number of 0 for non-root certificate"); | ||
| ret = ASN_PARSE_E; | ||
| } | ||
| } | ||
| if (ret < 0) { | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Serial-0 enforcement is now only done in ParseCertRelative(). Other parsing entry points that call DecodeToKey()/wc_GetPubX509()/DecodeCertInternal() directly (e.g., certificate loading in src/ssl_load.c) will no longer reject a zero serial even in strict mode. This can reintroduce acceptance of non-root certs with serial 0 in parts of the API; consider adding an equivalent strict check in those code paths (or refactoring the check into a shared helper that supports the root-CA exception).
| #if !defined(NO_CERTS) && !defined(NO_FILESYSTEM) && !defined(NO_RSA) | ||
| /* Test that root CA certificates with serial number 0 are accepted, | ||
| * while non-root certificates with serial 0 are rejected (issue #8615) */ | ||
|
|
||
| #if !defined(WOLFSSL_NO_ASN_STRICT) && !defined(WOLFSSL_PYTHON) && \ | ||
| !defined(WOLFSSL_ASN_ALLOW_0_SERIAL) | ||
| WOLFSSL_CERT_MANAGER* cm = NULL; | ||
| const char* rootSerial0File = "./certs/test-serial0/root_serial0.pem"; | ||
| const char* rootNormalFile = "./certs/test-serial0/root.pem"; | ||
| const char* eeSerial0File = "./certs/test-serial0/ee_serial0.pem"; | ||
| const char* eeNormalFile = "./certs/test-serial0/ee_normal.pem"; |
There was a problem hiding this comment.
This test loads/verifies PEM files via wolfSSL_CertManagerLoadCA/Verify, but it’s only guarded by NO_FILESYSTEM/NO_RSA. In builds without PEM-to-DER support (WOLFSSL_PEM_TO_DER), these APIs return NOT_COMPILED_IN and the test will fail. Please guard this test with WOLFSSL_PEM_TO_DER (or switch fixtures to DER and use WOLFSSL_FILETYPE_ASN1).
Fixes #8615
This pull request updates the logic for validating X.509 certificate serial numbers in
wolfcrypt/src/asn.c. The main change is to improve compliance with RFC 5280 while allowing for real-world exceptions involving root CAs. The previous strict check for zero serial numbers is now more nuanced, permitting serial number 0 for self-signed CA certificates but still rejecting it for other certificates.Certificate serial number validation improvements:
Testing
Tested with certificates generated using OpenSSL to verify all scenarios: