Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
+79 −5
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2607,14 +2607,14 @@ int wolfSSH_ProcessBuffer(WOLFSSH_CTX* ctx,
return WS_UNIMPLEMENTED_E;
}
#endif /* WOLFSSH_CERTS */
if (ret < 0) {
if (ret <= 0) {
if (type == BUFTYPE_PRIVKEY) {
/* wc_KeyPemToDer may have written partial key material;
* zeroize before free on the private-key path. */
WS_FORCEZERO(der, inSz);
}
WFREE(der, heap, dynamicType);
return WS_BAD_FILE_E;
return WS_PARSE_E;
Comment thread
yosuke-wolfssl marked this conversation as resolved.
}
derSz = (word32)ret;
}
Expand Down
4 changes: 2 additions & 2 deletions src/ssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2459,9 +2459,9 @@ static int DoPemCert(const byte* in, word32 inSz, byte** out, word32* outSz,

ret = wc_CertPemToDer(in, (int)inSz, der, (int)inSz, CERT_TYPE);
if (ret <= 0) {
WLOG(WS_LOG_DEBUG, "PEM to DER of certificate failed.");
WLOG(WS_LOG_DEBUG, "PEM certificate body would not decode.");
WFREE(der, heap, DYNTYPE_CERT);
return WS_BAD_FILE_E;
return WS_PARSE_E;
}
derSz = (word32)ret;

Expand Down
71 changes: 71 additions & 0 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,32 @@ static int load_file(const char* filename, byte** buf, word32* bufSz)
#endif


#ifdef WOLFSSH_CERTS

/* PEM shapes that carry a header the sniff accepts but a body no decoder
* will take, so the failure lands in the decoder rather than the sniff. */
static const char badPemCert[] =
"-----BEGIN CERTIFICATE-----\n"
"!!!! this is not base64 !!!!\n"
"-----END CERTIFICATE-----\n";
static const char noBodyPemCert[] = "-----BEGIN CERTIFICATE-----\n";
/* Under one full base64 group, so the body decodes to nothing rather than
* failing, and wolfSSL answers 0 for it instead of a negative code. */
static const char zeroLenPemCert[] =
"-----BEGIN CERTIFICATE-----\n"
"MI\n"
"-----END CERTIFICATE-----\n";

#ifndef WOLFSSH_NO_SERVER
static const char badPemKey[] =
"-----BEGIN PRIVATE KEY-----\n"
"!!!! this is not base64 !!!!\n"
"-----END PRIVATE KEY-----\n";
#endif /* WOLFSSH_NO_SERVER */

#endif /* WOLFSSH_CERTS */


static void test_wolfSSH_CTX_UseCert_buffer(void)
{
#ifdef WOLFSSH_CERTS
Expand Down Expand Up @@ -704,6 +730,18 @@ static void test_wolfSSH_CTX_UseCert_buffer(void)
AssertIntEQ(WS_BAD_FILETYPE_E,
wolfSSH_CTX_UseCert_buffer(ctx, cert, certSz, 99));

/* Content the caller declared PEM but that will not decode is malformed
* input, not a file that would not read. */
AssertIntEQ(WS_PARSE_E,
wolfSSH_CTX_UseCert_buffer(ctx, (const byte*)badPemCert,
(word32)WSTRLEN(badPemCert), WOLFSSH_FORMAT_PEM));
AssertIntEQ(WS_PARSE_E,
wolfSSH_CTX_UseCert_buffer(ctx, (const byte*)noBodyPemCert,
(word32)WSTRLEN(noBodyPemCert), WOLFSSH_FORMAT_PEM));
AssertIntEQ(WS_PARSE_E,
wolfSSH_CTX_UseCert_buffer(ctx, (const byte*)zeroLenPemCert,
(word32)WSTRLEN(zeroLenPemCert), WOLFSSH_FORMAT_PEM));

free(cert);
cert = NULL;

Expand Down Expand Up @@ -834,6 +872,24 @@ static void test_wolfSSH_ReadCert_buffer(void)
free(cert);
cert = NULL;

/* Keeping the header sends these past the sniff and into the decoder,
* where a body that will not decode is a parse failure. No file was
* opened on this path, so it must not be reported as a file error. */
AssertIntEQ(WS_PARSE_E, wolfSSH_ReadCert_buffer((const byte*)badPemCert,
(word32)WSTRLEN(badPemCert),
&out, &outSz, &outType, &outTypeSz, &flavor, NULL));
AssertNull(out);

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] New out-param assertions in test_wolfSSH_ReadCert_buffer pass vacuously · Weak or missing assertions

out, outSz, outType, outTypeSz and flavor are already NULL/0/UNKNOWN entering these calls, cleared by the preceding half-PEM rejection at line 870. The five checks would pass even if wolfSSH_ReadCert_buffer left every out param untouched, so they pin nothing.

Fix: Poison the out params with sentinels before each call, as the existing block at lines 934-946 does.

AssertIntEQ(outSz, 0);
AssertNull(outType);
AssertIntEQ(outTypeSz, 0);
AssertIntEQ(flavor, WOLFSSH_CERT_FLAVOR_UNKNOWN);

AssertIntEQ(WS_PARSE_E, wolfSSH_ReadCert_buffer((const byte*)noBodyPemCert,
(word32)WSTRLEN(noBodyPemCert),
&out, &outSz, &outType, &outTypeSz, &flavor, NULL));
AssertNull(out);
AssertIntEQ(flavor, WOLFSSH_CERT_FLAVOR_UNKNOWN);

AssertIntEQ(0, load_file("./keys/server-cert.der", &cert, &certSz));
#ifndef WOLFSSH_NO_ECDSA_SHA2_NISTP256
AssertIntEQ(WS_SUCCESS, wolfSSH_ReadCert_buffer(cert, certSz,
Expand Down Expand Up @@ -1058,6 +1114,15 @@ static void test_wolfSSH_CTX_AddRootCert_file(void)
/* The cert manager rejects a non-CA in wolfSSL's codes; this path maps. */
AssertIntEQ(WS_PARSE_E,
wolfSSH_CTX_AddRootCert_file(ctx, "./keys/server-key-ecc.der"));

/* The buffer entry point, tested here because it shares this one's
* decoder: a PEM body that will not decode is a parse failure. */
AssertIntEQ(WS_PARSE_E,
wolfSSH_CTX_AddRootCert_buffer(ctx, (const byte*)badPemCert,
(word32)WSTRLEN(badPemCert), WOLFSSH_FORMAT_PEM));
AssertIntEQ(WS_PARSE_E,
wolfSSH_CTX_AddRootCert_buffer(ctx, (const byte*)zeroLenPemCert,
(word32)WSTRLEN(zeroLenPemCert), WOLFSSH_FORMAT_PEM));
#ifdef WOLFSSH_TEST_OSSH_CERT_FILE
AssertIntEQ(0, writeTmpFile(osshCertPath, osshCertLine,
WSTRLEN(osshCertLine)));
Expand Down Expand Up @@ -1118,6 +1183,12 @@ static void test_wolfSSH_CTX_UsePrivateKey_buffer_pem(void)
key = NULL;
#endif /* WOLFSSH_NO_ECDSA */

/* A key PEM shares the certificate PEM's decoder, and answers the same
* way when its body will not decode. */
AssertIntEQ(WS_PARSE_E,

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] New ret = 0 rejection is unpinned on the private-key arm · Missing edge-case coverage on a function the PR also changed

The PR widens wolfSSH_ProcessBuffer's PEM guard from ret < 0 to ret <= 0 (src/internal.c:2610), newly rejecting a zero-length decode. Only zeroLenPemCert pins that for BUFTYPE_CERT/BUFTYPE_CA; badPemKey exercises only the negative-return path, so reverting the guard on the BUFTYPE_PRIVKEY arm still passes and lets derSz == 0 reach IdentifyAsn1Key.

Fix: Add a zero-length-body PEM private key fixture (a -----BEGIN PRIVATE KEY----- block under one base64 group) asserting WS_PARSE_E.

wolfSSH_CTX_UsePrivateKey_buffer(ctx, (const byte*)badPemKey,
(word32)WSTRLEN(badPemKey), WOLFSSH_FORMAT_PEM));

wolfSSH_CTX_free(ctx);
#endif /* WOLFSSH_CERTS && !WOLFSSH_NO_SERVER */
}
Expand Down
5 changes: 4 additions & 1 deletion wolfssh/ssh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,10 @@ WOLFSSH_API int wolfSSH_ReadKey_file(const char* name,

#if defined(WOLFSSH_CERTS) || defined(WOLFSSH_OSSH_CERTS)
/* Decodes a PEM/DER X.509 cert or OpenSSH cert line, detected from content.
* Caller frees out via heap; on failure every out param is cleared. */
* Caller frees out via heap; on failure every out param is cleared. Returns
* WS_BAD_FILETYPE_E if the content is not a certificate form, and WS_PARSE_E
* if it is but will not decode. WS_BAD_FILE_E comes only from the _file entry

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ [Info] New ssh.h note misstates when WS_BAD_FILE_E is returned · Incorrect error handling

The note says WS_BAD_FILE_E comes from the _file entry point "only when the file itself will not read", but wolfSSH_ReadCert_file also returns it for a NULL name (src/ssh.c:2753) with no file access — behaviour pinned at tests/api.c:990.

Fix: Extend the note to say a NULL file name also answers WS_BAD_FILE_E.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ [Info] New ssh.h return-code contract is contradicted by pinned behaviour · SSH protocol violations

The added comment states WS_BAD_FILE_E comes from the _file entry point "only when the file itself will not read", but wolfSSH_ReadCert_file also returns it for a NULL name (src/ssh.c:2754, pinned at tests/api.c:990). It also omits WS_INVALID_ALGO_ID and WS_UNIMPLEMENTED_E, which the buffer form returns for decodable certificate content (tests/api.c:864, 958).

Fix: Note that a NULL name also yields WS_BAD_FILE_E and that WS_INVALID_ALGO_ID / WS_UNIMPLEMENTED_E are possible for unsupported key types.

* point, and only when the file itself will not read. */
WOLFSSH_API int wolfSSH_ReadCert_buffer(const byte* in, word32 inSz,
byte** out, word32* outSz, const byte** outType, word32* outTypeSz,
byte* flavor, void* heap);
Expand Down
Loading