Skip to content

Fix silent key truncation on short BIO_write in key encoders - #481

Merged
padelsbach merged 3 commits into
wolfSSL:masterfrom
sameehj:fix/encoder-bio-short-write
Sep 9, 2026
Merged

Fix silent key truncation on short BIO_write in key encoders#481
padelsbach merged 3 commits into
wolfSSL:masterfrom
sameehj:fix/encoder-bio-short-write

Conversation

@sameehj

@sameehj sameehj commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR body

## Summary

The wolfProvider key encoders (RSA, ECC, ECX, DH, ML-DSA) wrote the encoded
key to the output BIO with a single `BIO_write` and treated any positive
return as complete success:

```c
rc = BIO_write(out, keyData, (int)keyLen);
if (rc <= 0) {
    ok = 0;
}

BIO_write is allowed to write fewer bytes than requested (a "short write"),
which returns a positive value smaller than keyLen. The old check only
rejected rc <= 0, so a short write was reported as success while the DER/PEM
output was silently truncated. Application-supplied BIOs (sockets, pipes,
non-blocking or custom sinks) can legitimately short-write, so a caller could
receive and persist a corrupt, incomplete key with no error.

Fix

Add a shared helper wp_write_bio() in src/wp_internal.c that loops until
every byte is written, and use it in all five encoders. The helper mirrors the
existing wp_read_der_bio style (WOLFPROV_ENTER/LEAVE, ok flag, Doxygen
header). Each iteration either advances past the bytes written or fails, so
there is no busy-loop or hang.

Encoder sinks are used synchronously, so a non-positive return is treated as a
hard failure rather than spinning on BIO_should_retry — it fails closed
instead of truncating.

Testing

New deterministic regression test test_ecc_encode_short_write_bio (unit case
143). It builds a custom BIO that accepts exactly one byte per write, encodes a
P-256 key as DER and PEM SubjectPublicKeyInfo through it, and asserts the
captured bytes equal the full reference encoding from OSSL_ENCODER_to_data.
No threads or timing — binary pass/fail.

Before/after (case number: ./test/unit.test --list | grep short_write):

  • Test-only commit (fix reverted): ./test/unit.test 143 fails with
    Short write truncated output: 1 of 91 bytes.
  • Fix applied: ./test/unit.test 143 passes for DER and PEM.
  • Full unit suite: 204/204, 0 failures.

Fenrir

Addresses Fenrir finding 11560 (short-write truncation in key encoders).

The key encoders write DER/PEM output with a single BIO_write and treat
any positive return as success, so a short write silently truncates the
encoding. Add a deterministic test that encodes an EC public key through
a BIO that accepts one byte per write and confirms no bytes are lost.

Fenrir 11560.
BIO_write may write fewer bytes than requested. The key encoders checked
only for a non-positive return, so a short write truncated the DER/PEM
output while still reporting success. Add wp_write_bio, which loops until
all bytes are written, and use it in the RSA, ECC, ECX, DH and ML-DSA
encoders.

Fenrir 11560.

@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 #481

Scan targets checked: wolfprovider-bugs, wolfprovider-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread test/unit.c
The regression test was registered under WP_HAVE_EC_P256 but defined and
declared under WP_HAVE_ECDH && WP_HAVE_EC_P256, so a P-256 build without
HAVE_ECC_DHE failed to compile. Move the test, its helper, and the sink
BIO out of the WP_HAVE_ECDH blocks so all three sites match.

@padelsbach padelsbach 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.

Nice fix, thanks

@padelsbach
padelsbach merged commit 71cbf68 into wolfSSL:master Sep 9, 2026
80 checks passed
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