Skip to content

x509: read the signed certificate DER from the x509 object - #292

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_9852
Open

x509: read the signed certificate DER from the x509 object#292
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_9852

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

wolfCLU_certSetup() aliased derBuf to inBuf, a pointer into the heap buffer holding the input PEM, then handed it to wolfSSL_i2d_X509(). Per the OpenSSL convention, i2d_* copies into a caller-supplied buffer with no bounds parameter:

pt = derBuf;                              /* points into the input PEM */
derBufSz = wolfSSL_i2d_X509(x509, &pt);   /* unbounded XMEMCPY */

The destination capacity is fixed by the input PEM length, while the amount written is the DER re-encoded after wolfCLU_setExtensions() and wolfSSL_X509_sign(). Nothing relates the two. A CSR PEM is roughly 1.37x its DER, so a plain conversion fits, but every extension added through -extfile/-extensions grows the output while inBufSz stays fixed. The i2d return value was also unchecked, so a negative result could reach wolfSSL_BIO_write() as a length.

Fix (src/x509/clu_cert_setup.c)

No new allocation is needed: the block only ever reads derBuf, and wolfSSL_i2d_X509() is wolfSSL_X509_get_der() plus a copy.

  • wolfSSL_X509_get_der() returns the object's own DER, which wolfSSL_X509_sign() refreshes. This function already uses it for -fingerprint.
  • const byte *derBuf makes the read-only use compiler-enforced.
  • ret == WOLFCLU_SUCCESS now guards both output branches.

Closes f-9852.

Tests

TestX509ReqLargeExtensions signs a CSR carrying a twelve-entry subjectAltName supplied by -extfile, covering both output forms:

Case Output path exercised
test_extfile_larger_than_input_pem_der_out wolfSSL_BIO_write()
test_extfile_larger_than_input_pem_pem_out wc_DerToPem()

Both skip when the build has no cert extensions or alt names.

Verification

  • Pre-fix, ASan reports a 1967-byte write into the 904-byte input buffer and both new tests fail; post-fix both pass.
  • make check: 23 passed, 2 skipped, 0 failed under ASan + UBSan, no sanitizer reports.
  • Clean under the project's -Werror warning set.

- wolfCLU_certSetup() takes the re-encoded certificate from
  wolfSSL_X509_get_der() instead of encoding it into inBuf with
  wolfSSL_i2d_X509(), and errors out when that returns NULL or a
  non-positive length.
- derBuf is a const byte pointer, and the pt local that carried the
  i2d pointer increment is removed.
- The DER and PEM output branches run only while ret is
  WOLFCLU_SUCCESS.
- tests/x509/x509-req-test.py adds TestX509ReqLargeExtensions, which
  signs a CSR with a twelve entry subjectAltName section supplied by
  -extfile and checks that both the DER and the PEM output form are
  longer than the input PEM and carry the names, skipping when the
  build has no cert extensions or alt names.

Issue: F-9852
@yosuke-wolfssl yosuke-wolfssl self-assigned this Sep 4, 2026
Copilot AI lite review requested due to automatic review settings September 4, 2026 06:09

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

The fix removes the unsafe unbounded write by switching to wolfSSL_X509_get_der() and is covered by new targeted tests that exercise both affected output paths under large extension growth.

Pull request overview

This pull request fixes a buffer overflow risk in the x509 -req output path by stopping reuse of the input PEM buffer as the destination for wolfSSL_i2d_X509() (which performs an unbounded copy), and instead reading the signed certificate’s DER directly from the WOLFSSL_X509 object. It also adds regression tests that ensure the signed certificate output can exceed the input CSR PEM size when large extensions are applied via -extfile/-extensions.

Changes:

  • Replace wolfSSL_i2d_X509() writing into the input buffer with wolfSSL_X509_get_der() (borrowed DER + size) and add error handling for invalid DER length/pointer.
  • Guard both DER and PEM output branches on ret == WOLFCLU_SUCCESS to prevent using an invalid length/pointer after DER retrieval failure.
  • Add TestX509ReqLargeExtensions to exercise both DER output (wolfSSL_BIO_write) and PEM output (wc_DerToPem) paths when extensions make the output larger than the input CSR PEM.
File summaries
File Description
src/x509/clu_cert_setup.c Stops writing DER into the input PEM buffer; reads DER from the X509 object and adds error/flow guards to prevent bad-length writes.
tests/x509/x509-req-test.py Adds regression tests that sign a CSR with oversized SAN extensions and verify both DER/PEM output paths handle outputs larger than the input CSR PEM.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-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.

Fenrir Automated Review — PR #292

Scan targets checked: wolfclu-bugs, wolfclu-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

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.

4 participants