Skip to content

Report a failed PEM decode as WS_PARSE_E, not WS_BAD_FILE_E - #1151

Merged
ejohnstown merged 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/errorCode
Aug 10, 2026
Merged

Report a failed PEM decode as WS_PARSE_E, not WS_BAD_FILE_E#1151
ejohnstown merged 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/errorCode

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

WS_BAD_FILE_E (-1019) meant two unrelated things: "I could not read the file"
(ReadFileIntoBuffer()) and "the PEM body was malformed" (DoPemCert() in
src/ssh.c, wolfSSH_ProcessBuffer() in src/internal.c).

So wolfSSH_ReadCert_buffer() — a public buffer entry point that opens no file
— answered -1019 for a PEM whose base64 body will not decode. A caller could
not tell "the bytes you gave me are not a certificate" from "your file would
not read" without knowing which form it handed in. WS_PARSE_E (-1005) is
already the answer for malformed input everywhere else in this reader,
including the DER arm two functions away.

Inherited style rather than a new mistake: wolfSSH_ProcessBuffer() has done
this for years and DoPemCert() matched its neighbour when PR #1140 added it.

Fix (src/ssh.c)

"The PEM body would not decode" now answers WS_PARSE_E from every entry
point, for certificate, CA and private-key buffers alike. WS_BAD_FILE_E is
left meaning only file I/O.

SiteWasIs
DoPemCert(), src/ssh.cWS_BAD_FILE_EWS_PARSE_E
wolfSSH_ProcessBuffer() PEM arm, src/internal.cWS_BAD_FILE_EWS_PARSE_E

Measured against the built library, before → after:

ReadCert_buffer [bad body] -1019 -> -1005 ReadCert_file [NULL name] -1019 -> -1019
ReadCert_buffer [header only] -1019 -> -1005 ReadCert_file [missing file] -1019 -> -1019
UseCert_buffer [bad body] -1019 -> -1005 UseCert_file [missing file] -1019 -> -1019
UseCert_buffer [header only] -1019 -> -1005 AddRootCert_file [missing file] -1019 -> -1019
AddRootCert_buf [bad body] -1019 -> -1005
UsePrivateKey_bf [bad body] -1019 -> -1005

wolfssh/ssh.h gained a note naming the split. No caller in the tree
discriminates on these codes; the only == WS_BAD_FILE_E comparison is in
src/wolfsftp.c and never reaches these paths.

Tests (tests/api.c)

This path had no coverage at all, so all six assertions are new pins rather
than moved ones — added to test_wolfSSH_ReadCert_buffer,
test_wolfSSH_CTX_UseCert_buffer, test_wolfSSH_CTX_AddRootCert_file (the
BUFTYPE_CA case, as no _buffer test exists) and
test_wolfSSH_CTX_UsePrivateKey_buffer_pem. The existing half-PEM case stays
pinned at WS_BAD_FILETYPE_E so sniff-rejection and decode-rejection remain
distinguishable.

Verification

  • make check: 11 passed, 0 failed, 1 skipped (network-dependent external.test).
  • Negative control: the new assertions fail at -1019 before the fix.
  • gcc-13 -Werror clean across 6 configs, certs-on and certs-off.

CopilotAI 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.

Pull request overview

This PR refines wolfSSH’s certificate/key loading error semantics so malformed PEM bodies are consistently reported as WS_PARSE_E (parse failure) rather than WS_BAD_FILE_E (file I/O failure), especially for buffer-based APIs that never touch the filesystem.

Changes:

  • Change PEM decode failure returns from WS_BAD_FILE_E to WS_PARSE_E in the PEM certificate decode path (DoPemCert) and the generic buffer processing PEM arm (wolfSSH_ProcessBuffer).
  • Document the intended split of WS_BAD_FILE_E (file read problems only) vs WS_PARSE_E (malformed but recognized content) in the public header.
  • Add negative tests to pin the corrected return codes for malformed PEM certificate/key bodies across several APIs.

Reviewed changes

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

FileDescription
wolfssh/ssh.hDocuments the intended separation between file I/O failures and decode/parse failures for cert reading APIs.
src/ssh.cUpdates the certificate PEM decode failure to return WS_PARSE_E and clarifies the debug log message.
src/internal.cUpdates generic PEM decode failure handling in wolfSSH_ProcessBuffer() to return WS_PARSE_E.
tests/api.cAdds regression tests ensuring malformed PEM bodies return WS_PARSE_E for buffer-based cert/key loading APIs.

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

Comment threadsrc/internal.c

@wolfSSL-Fenrir-botwolfSSL-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 #1151

Scan targets checked:wolfssh-bugs, wolfssh-src

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

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment threadtests/api.c
Comment threadwolfssh/ssh.h Outdated
Comment threadtests/api.c
Comment threadwolfssh/ssh.h Outdated
Comment threadtests/api.c
Comment threadwolfssh/ssh.h Outdated
Comment threadtests/api.c
Comment threadwolfssh/ssh.h Outdated

@wolfSSL-Fenrir-botwolfSSL-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 #1151

Scan targets checked:wolfssh-bugs, wolfssh-src

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

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment threadtests/api.c
@yosuke-wolfssl

Copy link
Copy Markdown
ContributorAuthor

Hi @ejohnstown ,
This is one of the follow-up. Please check this when you have a chance

@ejohnstown
ejohnstown merged commit 53538e8 into wolfSSL:masterAug 10, 2026
154 checks passed
@yosuke-wolfssl
yosuke-wolfssl deleted the fix/errorCode branch August 11, 2026 22:49
Sign up for freeto 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

@yosuke-wolfssl@ejohnstown@wolfSSL-Fenrir-bot