Skip to content

Fix handling of multiple certs in a single DER in wolfSSL_d2i_X509. - #12073

Open
kareem-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
kareem-wolfssl:zd22757
Open

kareem-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
kareem-wolfssl:zd22757

Conversation

@kareem-wolfssl

@kareem-wolfssl kareem-wolfssl commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Description

The trailing data rejection broke this function. Trailing data check should be disabled for it as it processes one cert at a time and advances the input pointer.
If there is trailing junk, the next call will catch it.

Fixes zd#22757

Testing

Provided test case from user, as well as built in and added tests.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@kareem-wolfssl
kareem-wolfssl requested a balanced review from Copilot November 10, 2026 00:50
@kareem-wolfssl kareem-wolfssl self-assigned this Nov 10, 2026
The trailing data rejection broke this function.  Trailing data
check should be disabled for it as it processes one cert at a time
and advances the input pointer.
If there is trailing junk, the next call will catch it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain for the change restoring concatenated DER certificate decoding.

No open issues 🎉

What changed in this PR

Restores wolfSSL_d2i_X509 support for concatenated DER certificates by allowing trailing data while decoding one certificate and advancing the input pointer past it.

Changes:

  • Enables trailing data for wolfSSL_d2i_X509 while preserving strict handling in the exact-length certificate and request APIs.
  • Adds regression tests for concatenated certificates, pointer advancement, truncated input, and trailing junk.
File Description
tests/​api/​test_ossl_x509_io.h Declares and registers the new regression test.
tests/​api/​test_ossl_x509_io.c Tests certificate decoding boundaries and trailing-data behavior.
src/​x509.c Adds a trailing-data option to the shared decoder and enables it for wolfSSL_d2i_X509.

Review effort: Balanced

@kareem-wolfssl

Copy link
Copy Markdown
Contributor Author

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. It turns off a strict trailing-data check on a public certificate-parsing API, so a human look is still worthwhile.

What was reviewed:

  • How far *in moves: derCert->length comes from AllocCopyDecodedDer(). On the template path it is capped at srcIdx, and on the non-template path at the rebounded maxIdx. Either way the pointer moves past one certificate and not past the trailing bytes.
  • Who is affected: only wolfSSL_d2i_X509() gets allowTrailing=1. wolfSSL_X509_d2i(), wolfSSL_X509_d2i_ex() and wolfSSL_X509_REQ_d2i() still pass 0, and the existing CSR exemption is unchanged.
  • Heap: hardcoding NULL changes nothing, because the old path through wolfSSL_X509_d2i() also passed NULL.
Extended reasoning...

The change is in src/x509.c. It adds an allowTrailing parameter to the static d2i_X509orX509REQ() helper, sets it only for the OpenSSL-compat wolfSSL_d2i_X509(), and adds a test for concatenated DER certificates. It reuses the existing DecodedCert allowTrailing bitfield, which the TRUSTED CERTIFICATE path already uses to skip the asn.c trailing-data rejection. The security-sensitive surface is X.509 DER parsing: the change relaxes the strict trailing-data check for one public API. The diff is small and follows an existing pattern, but because it loosens a parsing strictness check, I deferred instead of approving.

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

MemBrowse Memory Report · Explore what changed →

gcc-arm-cortex-m4-openssl-compat

  • FLASH: .text +64 B (+0.0%, 801,572 B / 1,048,576 B, total: 76% used)

zephyr-frdm_rw612

  • FLASH: datas +16 B, log_const_area -8 B, rodata -8 B, text +3,688 B (+1.3%, 286,152 B / 67,108,864 B, total: 0% used)
  • RAM: bss +49 B, datas +16 B (+0.1%, 58,423 B / 983,040 B, total: 6% used)

No memory changes detected for:


Catch memory regressions in CI with MemBrowse. Free for open source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants