Allow serial number 0 for root CA certificates

[jackctj117]

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:

• Moved and updated the check for serial numbers of 0 to allow self-signed CA (root) certificates to have a serial number of 0, while still treating serial 0 as an error for all other certificates. This better aligns with RFC 5280 and real-world trust store practices. [1] [2]
• Removed the previous check that always treated a serial number of 0 as an error, regardless of certificate type.

Testing

Tested with certificates generated using OpenSSL to verify all scenarios:

• Root CA with serial 0 loads successfully (previously failed, now passes)
• End-entity cert with serial 0 is correctly rejected
• Normal end-entity cert signed by root CA with serial 0 verifies successfully
• Self-signed non-CA cert with serial 0 is correctly rejected

[kareem-wolfssl]

Changes look good.

Can you add some test cases with a CA and leaf cert with a serial of 0?

[jackctj117]

@kareem-wolfssl it looks like the failures are looking for an expected failure due to a self-signed CA certificate with serial number 0.

[kareem-wolfssl]

@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 test_MakeCertWith0Ser since we no longer expect a root CA with serial 0 to fail.

[dgarske]

@jackctj117 looks like some valid unit test failures you will need to look into. All related to --enable-asn=original.

FAILURES:
   390: test_SerialNumber0_RootCA

[jackctj117]

Jenkins retest this please. History lost.

[jackctj117]

Jenkins retest this please. History lost.

[jackctj117]

Jenkins retest this please

[jackctj117]

Jenkins retest this please

[jackctj117]

Jenkins retest this

[jackctj117]

Jenkins retest this please

[kareem-wolfssl]

Jenkins retest this please

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

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.

[Copilot]

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 == 0 is 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.

[dgarske]

@jackctj117 please resolve merge conflicts. Thanks

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

wolfcrypt/src/asn.c:1

• Avoid returning directly from the middle of ParseCertRelative(). This function typically accumulates errors in ret and exits through a common path (often needed for consistent cleanup, logging, and state handling). Instead of return ASN_PARSE_E;, set ret = ASN_PARSE_E; and follow the function’s existing error-exit pattern (e.g., goto exit; / break; depending on structure) so resources/state are handled uniformly.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[SparkiDev]

retest this please

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

wolfcrypt/src/asn.c:1

• The PR description is focused on certificate serial-number validation, but this hunk also changes BIT STRING parsing behavior by removing the previous compile-time guard around the zero-length check. If the guard existed to preserve behavior for selftests/FIPS (or specific configurations), this could be an unintended behavior change; consider restoring the conditional compilation (or narrowing the change) unless this is explicitly part of the intended fix.
  wolfcrypt/src/asn.c:1
• The comment states the exception is for certs “being loaded as trust anchors”, but the condition implemented here only checks cert->isCA && cert->selfSigned (i.e., it’s not tied to a “trust-anchor load” context). Either adjust the comment to match the actual behavior, or tighten the condition so the exception is only applied in the trust-anchor loading path (to avoid misleading future maintainers).
  wolfcrypt/src/asn.c:1
• The “serial number == 0” policy appears implemented in multiple places (e.g., both decoding and relative parsing paths) with slightly different context checks and error handling (ret = ASN_PARSE_E vs return ASN_PARSE_E). Consider centralizing this into a shared helper (or a single policy function) so future tweaks don’t drift and so messaging/behavior stays consistent across code paths.
  tests/api.c:1
• This change makes the test depend on RSA (!defined(NO_RSA)) where it previously depended on ECC (defined(HAVE_ECC)). On configurations that build ECC but disable RSA, this test will no longer compile/run, reducing coverage for serial-0 certificate generation/decoding scenarios. If the intent is just to switch algorithms, consider supporting both (conditional RSA/ECC branches) so the test continues to exercise the behavior across common build configurations.
  tests/api.c:1
• This change makes the test depend on RSA (!defined(NO_RSA)) where it previously depended on ECC (defined(HAVE_ECC)). On configurations that build ECC but disable RSA, this test will no longer compile/run, reducing coverage for serial-0 certificate generation/decoding scenarios. If the intent is just to switch algorithms, consider supporting both (conditional RSA/ECC branches) so the test continues to exercise the behavior across common build configurations.
  tests/api.c:1
• This change makes the test depend on RSA (!defined(NO_RSA)) where it previously depended on ECC (defined(HAVE_ECC)). On configurations that build ECC but disable RSA, this test will no longer compile/run, reducing coverage for serial-0 certificate generation/decoding scenarios. If the intent is just to switch algorithms, consider supporting both (conditional RSA/ECC branches) so the test continues to exercise the behavior across common build configurations.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ColtonWilley]

Jenkins retest this please

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment here says the serial-0 exception is for root CAs “being loaded as trust anchors”, but DecodeCertInternal() doesn’t have enough context to know how the cert will be used and will allow serial 0 for any self-signed CA that reaches this code path. Consider rewording to avoid implying it is limited to trust-anchor loading (or explicitly note that additional call-site checks may further restrict acceptance).