Skip to content

Fix multiple reported issues #1084

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
kareem-wolfssl wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fix multiple reported issues #1084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1c61fe4
Add a segment-size bounds check before computing remaining buffer spa…
kareem-wolfssl Jul 1, 2026
3ae9836
Ensure the key blob size is correct in FindKeyId.
kareem-wolfssl Jul 6, 2026
8ad669a
Fix multiple issues in PostLock/PostUnlock.
kareem-wolfssl Jul 6, 2026
fd36f9a
Replace atoi with strtoull in SCP code.
kareem-wolfssl Jul 6, 2026
712fe71
Check DKI languages fit into the buffer.
kareem-wolfssl Jul 6, 2026
9407efa
Add NULL check to wolfSSH_CERTMAN_LoadRootCA_buffer.
kareem-wolfssl Jul 6, 2026
78390be
Rework free space calculation in wstrncat.
kareem-wolfssl Jul 6, 2026
08316f8
Code review feedback
kareem-wolfssl Jul 6, 2026
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
70 changes: 45 additions & 25 deletions src/agent.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@
#endif


#ifndef WOLFSSH_MAX_PASSPHRASE_SZ
#define WOLFSSH_MAX_PASSPHRASE_SZ 256
#endif


/* payloadSz is an estimate, but it shall be greater-than/equal-to
* the actual value. */
static int PrepareMessage(WOLFSSH_AGENT_CTX* agent, word32 payloadSz)
Expand Down Expand Up @@ -370,45 +375,57 @@ static int PostSuccess(WOLFSSH_AGENT_CTX* agent)
static int PostLock(WOLFSSH_AGENT_CTX* agent,
const byte* passphrase, word32 passphraseSz)
{
char pp[32];
word32 ppSz;
char* pp;
int ret = WS_SUCCESS;

(void)agent;
WLOG(WS_LOG_AGENT, "Posting lock to agent %p", agent);
WOLFSSH_UNUSED(agent);

ppSz = sizeof(pp) - 1;
if (passphraseSz < ppSz)
ppSz = passphraseSz;
pp = (char*)WMALLOC(passphraseSz + 1, agent->heap, DYNTYPE_STRING);
if (pp == NULL)
ret = WS_MEMORY_E;
Comment thread
kareem-wolfssl marked this conversation as resolved.

WMEMCPY(pp, passphrase, ppSz);
pp[ppSz] = 0;
WLOG(WS_LOG_AGENT, "Locking with passphrase '%s'", pp);
if (ret == WS_SUCCESS) {
WMEMCPY(pp, passphrase, passphraseSz);
pp[passphraseSz] = 0;
#ifdef SHOW_SECRETS
WLOG(WS_LOG_AGENT, "Locking with passphrase '%s'", pp);
#else
WLOG(WS_LOG_AGENT, "Locking with passphrase");
#endif
ForceZero(pp, passphraseSz);
WFREE(pp, agent->heap, DYNTYPE_STRING);
}

return WS_SUCCESS;
return ret;
}


/* Stub that has not yet been implemented */
static int PostUnlock(WOLFSSH_AGENT_CTX* agent,
const byte* passphrase, word32 passphraseSz)
{
char pp[32];
word32 ppSz;
char* pp;
int ret = WS_SUCCESS;

(void)agent;
WLOG(WS_LOG_AGENT, "Posting unlock to agent %p", agent);
WOLFSSH_UNUSED(agent);

ppSz = sizeof(pp) - 1;
if (passphraseSz < ppSz)
ppSz = passphraseSz;
pp = (char*)WMALLOC(passphraseSz + 1, agent->heap, DYNTYPE_STRING);
if (pp == NULL)
ret = WS_MEMORY_E;
Comment thread
kareem-wolfssl marked this conversation as resolved.

WMEMCPY(pp, passphrase, ppSz);
pp[ppSz] = 0;
WLOG(WS_LOG_AGENT, "Unlocking with passphrase '%s'", pp);
if (ret == WS_SUCCESS) {
WMEMCPY(pp, passphrase, passphraseSz);
pp[passphraseSz] = 0;
#ifdef SHOW_SECRETS
WLOG(WS_LOG_AGENT, "Unlocking with passphrase '%s'", pp);
#else
WLOG(WS_LOG_AGENT, "Unlocking with passphrase");
#endif
ForceZero(pp, passphraseSz);
WFREE(pp, agent->heap, DYNTYPE_STRING);
}

return WS_SUCCESS;
return ret;
}


Expand Down Expand Up @@ -672,7 +689,8 @@ static WOLFSSH_AGENT_ID* FindKeyId(WOLFSSH_AGENT_ID* id,
if (ret == WS_SUCCESS) {
while (id != NULL &&
WMEMCMP(digest, id->id, WC_SHA256_DIGEST_SIZE) != 0 &&
WMEMCMP(keyBlob, id->keyBlob, keyBlobSz)) {
(id->keyBlobSz != keyBlobSz ||
WMEMCMP(keyBlob, id->keyBlob, id->keyBlobSz) != 0)) {
id = id->next;
}
}
Expand Down Expand Up @@ -1252,7 +1270,8 @@ static int DoAgentLock(WOLFSSH_AGENT_CTX* agent,
ato32(buf + begin, &passphraseSz);
begin += LENGTH_SZ;

if (begin + passphraseSz > len) {
if (passphraseSz > WOLFSSH_MAX_PASSPHRASE_SZ ||
begin + passphraseSz > len) {
ret = WS_PARSE_E;
}
}
Expand Down Expand Up @@ -1296,7 +1315,8 @@ static int DoAgentUnlock(WOLFSSH_AGENT_CTX* agent,
ato32(buf + begin, &passphraseSz);
begin += LENGTH_SZ;

if (begin + passphraseSz > len)
if (passphraseSz > WOLFSSH_MAX_PASSPHRASE_SZ ||
begin + passphraseSz > len)
ret = WS_PARSE_E;
}

Expand Down
12 changes: 7 additions & 5 deletions src/certman.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,16 @@ void wolfSSH_CERTMAN_free(WOLFSSH_CERTMAN* cm)
int wolfSSH_CERTMAN_LoadRootCA_buffer(WOLFSSH_CERTMAN* cm,
const unsigned char* rootCa, word32 rootCaSz)
{
int ret;
int ret = WS_BAD_ARGUMENT;

WLOG_ENTER();

ret = wolfSSL_CertManagerLoadCABuffer(cm->cm, rootCa, rootCaSz,
WOLFSSL_FILETYPE_ASN1);
if (ret == WOLFSSL_SUCCESS) {
ret = WS_SUCCESS;
if (cm != NULL && rootCa != NULL && rootCaSz > 0) {
ret = wolfSSL_CertManagerLoadCABuffer(cm->cm, rootCa, rootCaSz,
WOLFSSL_FILETYPE_ASN1);
if (ret == WOLFSSL_SUCCESS) {
ret = WS_SUCCESS;
}
}

WLOG_LEAVE(ret);
Expand Down
16 changes: 12 additions & 4 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5055,16 +5055,24 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
if (ret == WS_SUCCESS) {
WLOG(WS_LOG_DEBUG, "DKI: Languages - Client to Server");
ret = GetUint32(&skipSz, buf, len, &begin);
if (ret == WS_SUCCESS)
begin += skipSz;
if (ret == WS_SUCCESS) {
if (skipSz <= len - begin)
begin += skipSz;
else
ret = WS_BUFFER_E;
}
}

/* Languages - Server to Client, skip */
if (ret == WS_SUCCESS) {
WLOG(WS_LOG_DEBUG, "DKI: Languages - Server to Client");
ret = GetUint32(&skipSz, buf, len, &begin);
if (ret == WS_SUCCESS)
begin += skipSz;
if (ret == WS_SUCCESS) {
if (skipSz <= len - begin)
begin += skipSz;
else
ret = WS_BUFFER_E;
}
}

/* First KEX Packet Follows */

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 [Medium] ML-DSA-only builds do not advertise publickey auth by default
💡 SUGGEST bug

The PR adds ML-DSA public-key authentication paths and compiles DoUserAuthRequestPublicKey() when !defined(WOLFSSH_NO_MLDSA), but the default allowed-auth list still adds publickey only when RSA or ECDSA is enabled. In an ML-DSA-only build, a server can now process ML-DSA public-key auth, but a normal USERAUTH_FAILURE response will omit publickey, so clients may never attempt the newly supported method unless an application overrides userAuthTypesCb.

Recommendation: Update the compile-time gate to include the public-key algorithms now supported by this file, at least ML-DSA, and add a regression test for an ML-DSA-only default auth list.

Expand Down
13 changes: 12 additions & 1 deletion src/port.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,18 @@ char* wstrnstr(const char* s1, const char* s2, unsigned int n)
* end of s1 including a null terminator. */
char* wstrncat(char* s1, const char* s2, size_t n)
{
size_t freeSpace = n - strlen(s1) - 1;
size_t s1_len = 0;
size_t freeSpace;

while (s1_len < n && s1[s1_len] != '\0') {
s1_len++;
}

if (s1_len >= n) {
return NULL;
}

freeSpace = n - s1_len - 1;

if (freeSpace >= strlen(s2)) {
#ifndef USE_WINDOWS_API
Expand Down
2 changes: 1 addition & 1 deletion src/ssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3835,7 +3835,7 @@ int wolfSSH_RealPath(const char* defaultPath, char* in,
}

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 [Medium] TPM host key API leaves ctx mutated when registration fails
💡 SUGGEST api

The new API stores ctx->tpmDev and ctx->tpmKey before checking whether wolfSSH_SetHostTpmKey() succeeds. If registration fails, for example because privateKeyCount is already at WOLFSSH_MAX_PVT_KEYS, the function returns an error but leaves the context partially configured. A later call with a different TPM key then hits the ctx->tpmKey != NULL && ctx->tpmKey != key guard and fails even though the original setup did not succeed.

Recommendation: Commit TPM device/key pointers only after the host-key slot registration succeeds, or explicitly roll them back on failure.

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 [Medium] New ML-DSA ASN.1 public-key path bypasses caller output buffer
💡 SUGGEST api

The ML-DSA public-key branch always allocates a new output buffer and assigns it to *out. That means a caller-provided buffer is ignored, undersized caller buffers do not get WS_BUFFER_E, and the original *out pointer can be overwritten. This is in newly added ML-DSA handling and should either follow the existing wolfSSH_ReadKey_buffer_ex() output-buffer contract or be explicitly documented and tested as allocate-only.

Recommendation: Compute the required size first, use *out when it is non-NULL and large enough, return WS_BUFFER_E when it is too small, and allocate only when *out == NULL.

/* Everything else is copied */
else {
if (curSz >= outSz - segSz) {
if (segSz > outSz || curSz >= outSz - segSz) {
return WS_INVALID_PATH_E;
}

Expand Down
73 changes: 53 additions & 20 deletions src/wolfscp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
#include <wolfssh/internal.h>
#include <wolfssh/log.h>

#include <errno.h>
#include <stdint.h>


#ifdef NO_INLINE
#include <wolfssh/misc.h>
Expand Down Expand Up @@ -1093,19 +1096,35 @@ static int GetScpFileSize(WOLFSSH* ssh, byte* buf, word32 bufSz,
ret = WS_SCP_BAD_MSG_E;

if (ret == WS_SUCCESS) {
/* replace space with newline for atoi */
buf[spaceIdx] = '\n';
ssh->scpFileSz = atoi((char *)(buf + idx));
/* replace space with newline to terminate the size field, then parse
* with strtoull() which parses in 64-bit width, so a negative field
* such as "-1" wraps above UINT32_MAX and is rejected by the bound
* below instead of becoming a huge word32 size */
char* endptr = NULL;
word64 fileSz;

/* restore space, increment idx to space */
buf[spaceIdx] = '\n';
errno = 0;
fileSz = (word64)strtoull((char*)(buf + idx), &endptr, 10);
buf[spaceIdx] = ' ';
idx = spaceIdx;

/* eat trailing space */
if (bufSz >= (word32)(idx + 1))
idx++;
/* reject any parse error (e.g. ERANGE overflow), a non-numeric field
* (parse must consume every character up to the separator), and
* sizes too large for the word32 scpFileSz */
if (errno != 0 || endptr != (char*)(buf + spaceIdx) ||
fileSz > UINT32_MAX) {
ret = WS_SCP_BAD_MSG_E;
}
else {
ssh->scpFileSz = (word32)fileSz;

*inOutIdx = idx;
/* increment idx to space, then eat trailing space */
idx = spaceIdx;
if (bufSz >= (word32)(idx + 1))
idx++;

*inOutIdx = idx;
}
}

return ret;
Expand Down Expand Up @@ -1225,15 +1244,23 @@ static int GetScpTimestamp(WOLFSSH* ssh, byte* buf, word32 bufSz,

/* read modification time */
if (ret == WS_SUCCESS) {
/* replace space with newline for atoi */
buf[spaceIdx] = '\n';
ssh->scpMTime = atoi((char*)(buf + idx));
char* endptr = NULL;

/* restore space, increment idx past it */
/* replace space with newline to terminate the field */
buf[spaceIdx] = '\n';
errno = 0;
ssh->scpMTime = (word64)strtoull((char*)(buf + idx), &endptr, 10);
buf[spaceIdx] = ' ';
if (spaceIdx + 1 < bufSz) {

/* reject any parse error (e.g. ERANGE overflow) and a non-numeric
* field, then step past the separating space */
if (errno != 0 || endptr != (char*)(buf + spaceIdx)) {
ret = WS_SCP_TIMESTAMP_E;
}
else if (spaceIdx + 1 < bufSz) {
idx = spaceIdx + 1;
} else {
}
else {
ret = WS_SCP_TIMESTAMP_E;
}
}
Expand Down Expand Up @@ -1264,15 +1291,21 @@ static int GetScpTimestamp(WOLFSSH* ssh, byte* buf, word32 bufSz,
}

if (ret == WS_SUCCESS) {
/* replace space with newline for atoi */
char* endptr = NULL;
/* replace space with newline for strtoull */
buf[spaceIdx] = '\n';
ssh->scpATime = atoi((char*)(buf + idx));

errno = 0;
ssh->scpATime = (word64)strtoull((char*)(buf + idx), &endptr, 10);
/* restore space, increment idx past it */
buf[spaceIdx] = ' ';
if (spaceIdx + 1 < bufSz) {

if (errno != 0 || endptr != (char*)(buf + spaceIdx)) {
ret = WS_SCP_TIMESTAMP_E;
}
else if (spaceIdx + 1 < bufSz) {
idx = spaceIdx + 1;
} else {
}
else {
ret = WS_SCP_TIMESTAMP_E;
}
}
Expand Down
Loading