-
Notifications
You must be signed in to change notification settings - Fork 923
Allow serial number 0 for root CA certificates #9567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
kareem-wolfssl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, check for the expected error code.
|
Jenkins retest this please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| /* 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) 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 (!(cert->isCA && cert->selfSigned) | ||
| #ifdef WOLFSSL_CERT_REQ | ||
| && !cert->isCSR | ||
| #endif | ||
| ) { |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serial-0 exception is currently keyed off cert->isCA && cert->selfSigned, but selfSigned is determined by issuer/subject name hash equality (not by verifying the signature with the certificate's own public key). This makes the exception apply to any self-issued CA certificate (and in all parse contexts), not specifically to explicitly-trusted root CAs as the comment describes. Consider tightening the condition to only allow serial 0 when parsing a trust anchor context (e.g., CA_TYPE / TRUSTED_PEER_TYPE) or when the certificate is cryptographically self-signed, so nonconforming non-root/self-issued certs remain rejected under strict ASN settings.
| #if !defined(NO_CERTS) && !defined(NO_FILESYSTEM) && !defined(NO_RSA) && \ | ||
| defined(WOLFSSL_CERT_GEN) && defined(WOLFSSL_CERT_EXT) | ||
| /* Test that root CA certificates with serial number 0 are accepted, |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test only loads/verifies existing PEM files via the cert manager, but it is gated on WOLFSSL_CERT_GEN. Certificate generation support shouldn't be required for these APIs, so this guard will skip the test in builds that still include ASN/X.509 parsing/verification. Consider dropping defined(WOLFSSL_CERT_GEN) from the preprocessor condition (keeping only the features actually required, e.g., filesystem/certs/extensions) to improve coverage across configurations.
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: