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 7 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
7 commits
Select commit Hold shift + click to select a range
f0ed3ec
Add a segment-size bounds check before computing remaining buffer spa…
kareem-wolfssl Jul 1, 2026
48b55c2
Ensure the key blob size is correct in FindKeyId.
kareem-wolfssl Jul 6, 2026
9756122
Fix multiple issues in PostLock/PostUnlock.
kareem-wolfssl Jul 6, 2026
4c7ef4e
Replace atoi with strtoull in SCP code.
kareem-wolfssl Jul 6, 2026
fb6496e
Add NULL check to wolfSSH_CERTMAN_LoadRootCA_buffer.
kareem-wolfssl Jul 6, 2026
baf2356
Rework free space calculation in wstrncat.
kareem-wolfssl Jul 6, 2026
faba36e
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
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') {

@aidangarske aidangarske Jul 13, 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.

🔴 [High] wstrncat change breaks RealPath callers that pass remaining capacity · Logic

The PR changes wstrncat() to return NULL when s1 is not NUL-terminated within the first n bytes. That matches the helper's documented contract and is safer for callers that pass the full destination size and check the return value, but existing changed-file callers do neither. wolfSSH_RealPath() calls WSTRNCAT(out, ..., outSz - curSz) (src/ssh.c:3843 and src/ssh.c:3846) with remaining capacity instead of the full buffer size while out already holds curSz bytes, and ignores the return. After this PR, for a remote SFTP/SCP path with a long first segment, once curSz >= outSz - curSz the existing terminator is beyond the shortened n, so later appends return NULL even when the final path still fits; wolfSSH_RealPath() still increments curSz and returns WS_SUCCESS with a truncated path (separator/segment omitted). The same new failure mode affects other changed-file SCP path construction such as WSTRNCAT((char*)basePath, "/", sizeof("/")) at src/wolfscp.c:2466. This reaches public wolfSSH_RealPath callers used by SFTP/SCP path handling. Severity note: the review mode rated this High (BLOCK) as a silent path-truncation bug; review-security rated it Medium (CWE-252, unchecked return value); the stricter High rating is kept here.

Fix: Keep the bounded scan, but update affected callers to pass the full destination buffer size and check WSTRNCAT() return values. In wolfSSH_RealPath(), include the optional slash in the bounds check and use explicit length-based appends or call WSTRNCAT(out, ..., outSz) after the existing bounds checks, returning WS_INVALID_PATH_E on NULL:

if (curSz + ((curSz != 1) ? 1 : 0) + segSz >= outSz) {
return WS_INVALID_PATH_E;
}
if (curSz != 1) {
if (WSTRNCAT(out, "/", outSz) == NULL)
return WS_INVALID_PATH_E;
curSz++;
}
if (WSTRNCAT(out, seg, outSz) == NULL)
return WS_INVALID_PATH_E;

Fix the Nucleus SCP basePath append to use the actual basePath capacity. Add a regression test where a valid resolved path grows beyond half of the output buffer but still fits.

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,
}
Comment thread
aidangarske marked this conversation as resolved.
Comment thread
aidangarske marked this conversation as resolved.
/* 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';

@aidangarske aidangarske Jul 13, 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 SCP numeric parsing lacks regression coverage · test

The PR replaces atoi with strtoull and adds overflow and malformed-field handling for SCP file sizes and timestamps, but the current tests only expose wolfSSH_TestScpGetFileMode; there are no targeted tests for these changed parser paths. This is boundary-heavy protocol parsing, so regressions around negative values, overflow, and partial parses would be easy to miss.

Fix: Add WOLFSSH_TEST_INTERNAL wrappers or ReceiveScpMessage-based tests for file sizes -1, UINT32_MAX, UINT32_MAX + 1, empty/non-numeric fields, timestamp overflow, and malformed timestamp fields.

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