Skip to content

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

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/errorCode
Open

Report a failed PEM decode as WS_PARSE_E, not WS_BAD_FILE_E#1151
yosuke-wolfssl wants to merge 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.

Site Was Is
DoPemCert(), src/ssh.c WS_BAD_FILE_E WS_PARSE_E
wolfSSH_ProcessBuffer() PEM arm, src/internal.c WS_BAD_FILE_E WS_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.

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.

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.

File Description
wolfssh/ssh.h Documents the intended separation between file I/O failures and decode/parse failures for cert reading APIs.
src/ssh.c Updates the certificate PEM decode failure to return WS_PARSE_E and clarifies the debug log message.
src/internal.c Updates generic PEM decode failure handling in wolfSSH_ProcessBuffer() to return WS_PARSE_E.
tests/api.c Adds 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 thread src/internal.c

@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 #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 thread tests/api.c
Comment thread wolfssh/ssh.h Outdated
Comment thread tests/api.c
Comment thread wolfssh/ssh.h Outdated
Comment thread tests/api.c
Comment thread wolfssh/ssh.h Outdated
Comment thread tests/api.c
Comment thread wolfssh/ssh.h Outdated

@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 #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 thread tests/api.c
@yosuke-wolfssl

Copy link
Copy Markdown
Contributor Author

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

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