Fix pcl-a1j: buffer overflow in ptools_create_backend_event() via unchecked sprintf and strcat (#355)
* Fix pcl-a1j.1: replace sprintf with snprintf and add strcat length check in ptools_create_backend_event() Add paramname length check (> 254 bytes → skip with warning) before snprintf into charBuff[i][258], and validate the snprintf return value. Add explicit length check before strcat into keyParams to prevent overflow when paramname exceeds remaining buffer space. Eliminates buffer overflow from long paramname. Ref GH #195. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix pcl-a1j.1: clamp pCnt to PTOOLS_MAX_PARAMS to prevent charBuff stack overflow charBuff[30][258] is a fixed-size stack array but pCnt was unbounded, allowing any caller with params->paramCnt > 30 to overflow the stack via charBuff[i] access. Add PTOOLS_MAX_PARAMS (30) define, use it to size charBuff, and clamp pCnt to PTOOLS_MAX_PARAMS with a warning log before the loop. Ref GH #195. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add ptools_create_backend_event() validation tests (pcl-a1j) 11 test cases covering pCnt clamping, paramname length guards, snprintf boundary, keyParams overflow check, and comma-prefix for subsequent params. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Levi Neely <lkn@darkstar.example.net> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Levi Neely
GitHub
parent
d5a1c74c43
commit
60e43edec4
|
|
@ -44,12 +44,15 @@
|
|||
#include <sys/types.h>
|
||||
|
||||
#include "papi.h"
|
||||
#include "pdbg.h"
|
||||
#include "plibs.h"
|
||||
#include "pnetlibs.h"
|
||||
#include "psettings.h"
|
||||
#include "psql.h"
|
||||
#include "ptools.h"
|
||||
|
||||
#define PTOOLS_MAX_PARAMS 30
|
||||
|
||||
char *ptools_get_mac_addr() {
|
||||
char buffer[128];
|
||||
|
||||
|
|
@ -93,10 +96,16 @@ int ptools_create_backend_event(const char *binapi, const char *category,
|
|||
binparam *paramsLocal;
|
||||
int i;
|
||||
int pCnt = params->paramCnt; // Number of optional parameters
|
||||
if (pCnt > PTOOLS_MAX_PARAMS) {
|
||||
pdbg_logf(D_WARNING,
|
||||
"ptools_create_backend_event: paramCnt %d exceeds max %d, clamping",
|
||||
pCnt, PTOOLS_MAX_PARAMS);
|
||||
pCnt = PTOOLS_MAX_PARAMS;
|
||||
}
|
||||
int mpCnt = 6; // Number of mandatory params
|
||||
int tpCnt; // Total number of parameters
|
||||
char *keyParams = NULL;
|
||||
char charBuff[30][258];
|
||||
char charBuff[PTOOLS_MAX_PARAMS][258];
|
||||
|
||||
sock = papi_connect(binapi, psync_setting_get_bool(0));
|
||||
|
||||
|
|
@ -130,8 +139,31 @@ int ptools_create_backend_event(const char *binapi, const char *category,
|
|||
keyParams[0] = 0;
|
||||
|
||||
for (i = 0; i < pCnt; i++) {
|
||||
size_t namelen;
|
||||
size_t used;
|
||||
size_t needed;
|
||||
int n;
|
||||
charBuff[i][0] = 0;
|
||||
|
||||
/* Reject paramnames > 254 bytes: "key" (3) + paramname + NUL must fit in 258 */
|
||||
namelen = strlen(params->Params[i].paramname);
|
||||
if (namelen > 254) {
|
||||
pdbg_logf(D_WARNING,
|
||||
"ptools_create_backend_event: paramname[%d] too long (%zu bytes), skipping",
|
||||
i, namelen);
|
||||
continue;
|
||||
}
|
||||
|
||||
/* Length check before strcat: verify paramname fits in remaining keyParams space */
|
||||
used = strlen(keyParams);
|
||||
needed = namelen + (i > 0 ? 2 : 1); /* comma (if i>0) + name + NUL */
|
||||
if (used + needed > (size_t)(258 * pCnt)) {
|
||||
pdbg_logf(D_WARNING,
|
||||
"ptools_create_backend_event: keyParams buffer full, skipping param %d",
|
||||
i);
|
||||
continue;
|
||||
}
|
||||
|
||||
if (i > 0) {
|
||||
strcat(keyParams, ",");
|
||||
strcat(keyParams, params->Params[i].paramname);
|
||||
|
|
@ -139,7 +171,15 @@ int ptools_create_backend_event(const char *binapi, const char *category,
|
|||
strcat(keyParams, params->Params[i].paramname);
|
||||
}
|
||||
|
||||
snprintf(charBuff[i], sizeof(charBuff[i]), "key%s", params->Params[i].paramname);
|
||||
n = snprintf(charBuff[i], sizeof(charBuff[i]), "key%s", params->Params[i].paramname);
|
||||
if (n < 0 || n >= (int)sizeof(charBuff[i])) {
|
||||
/* Should not happen after the namelen > 254 check above, but guard anyway */
|
||||
pdbg_logf(D_WARNING,
|
||||
"ptools_create_backend_event: snprintf truncated charBuff[%d], skipping",
|
||||
i);
|
||||
charBuff[i][0] = 0;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (params->Params[i].paramtype == 0) {
|
||||
paramsLocal[mpCnt + i] =
|
||||
|
|
|
|||
|
|
@ -0,0 +1,225 @@
|
|||
/*
|
||||
* Test: ptools_create_backend_event() parameter validation (pcl-a1j)
|
||||
*
|
||||
* Exercises the guards added in commits bb7fc67 + cda963c:
|
||||
* 1. pCnt > PTOOLS_MAX_PARAMS → clamped to 30
|
||||
* 2. paramname > 254 bytes → skipped (continue)
|
||||
* 3. snprintf fits in charBuff[i] (258 bytes) after namelen guard
|
||||
* 4. keyParams strcat length check prevents buffer overrun
|
||||
* 5. Normal short paramname → accepted, charBuff populated correctly
|
||||
*
|
||||
* The function under test requires a live network connection, so we
|
||||
* replicate its validation logic inline (identical to ptools.c) and
|
||||
* drive it with crafted inputs.
|
||||
*/
|
||||
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
|
||||
#define PTOOLS_MAX_PARAMS 30
|
||||
|
||||
static int passes = 0;
|
||||
static int failures = 0;
|
||||
|
||||
#define PASS(name) do { printf("PASS: %s\n", name); passes++; } while(0)
|
||||
#define FAIL(name, ...) do { printf("FAIL: " name " — " __VA_ARGS__); printf("\n"); failures++; } while(0)
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* Replica: pCnt clamping */
|
||||
/* ------------------------------------------------------------------ */
|
||||
static int clamp_pCnt(int raw) {
|
||||
int pCnt = raw;
|
||||
if (pCnt > PTOOLS_MAX_PARAMS)
|
||||
pCnt = PTOOLS_MAX_PARAMS;
|
||||
return pCnt;
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* Replica: per-param validation loop body */
|
||||
/* Returns 1 if param is accepted, 0 if skipped */
|
||||
/* ------------------------------------------------------------------ */
|
||||
typedef struct {
|
||||
int skipped_namelen; /* namelen > 254 */
|
||||
int skipped_keybuf; /* keyParams full */
|
||||
int skipped_snprintf; /* snprintf truncation (should never fire
|
||||
after namelen guard, but guarded) */
|
||||
char charBuff[258];
|
||||
char keyParams_contrib[258];
|
||||
} param_result_t;
|
||||
|
||||
static param_result_t validate_param(const char *paramname,
|
||||
const char *keyParams_so_far,
|
||||
int i,
|
||||
int pCnt) {
|
||||
param_result_t r;
|
||||
memset(&r, 0, sizeof(r));
|
||||
|
||||
size_t namelen = strlen(paramname);
|
||||
if (namelen > 254) {
|
||||
r.skipped_namelen = 1;
|
||||
return r;
|
||||
}
|
||||
|
||||
size_t used = strlen(keyParams_so_far);
|
||||
size_t needed = namelen + (i > 0 ? 2 : 1);
|
||||
if (used + needed > (size_t)(258 * pCnt)) {
|
||||
r.skipped_keybuf = 1;
|
||||
return r;
|
||||
}
|
||||
|
||||
int n = snprintf(r.charBuff, sizeof(r.charBuff), "key%s", paramname);
|
||||
if (n < 0 || n >= (int)sizeof(r.charBuff)) {
|
||||
r.skipped_snprintf = 1;
|
||||
r.charBuff[0] = 0;
|
||||
return r;
|
||||
}
|
||||
|
||||
/* Build keyParams contribution */
|
||||
if (i > 0) {
|
||||
strcat(r.keyParams_contrib, ",");
|
||||
}
|
||||
strcat(r.keyParams_contrib, paramname);
|
||||
|
||||
return r;
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* Tests */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
static void test_clamp_within_limit(void) {
|
||||
if (clamp_pCnt(10) == 10)
|
||||
PASS("pCnt=10 stays 10");
|
||||
else
|
||||
FAIL("pCnt=10 stays 10", "got %d", clamp_pCnt(10));
|
||||
}
|
||||
|
||||
static void test_clamp_at_limit(void) {
|
||||
if (clamp_pCnt(30) == 30)
|
||||
PASS("pCnt=30 stays 30");
|
||||
else
|
||||
FAIL("pCnt=30 stays 30", "got %d", clamp_pCnt(30));
|
||||
}
|
||||
|
||||
static void test_clamp_exceeds_limit(void) {
|
||||
int clamped = clamp_pCnt(31);
|
||||
if (clamped == PTOOLS_MAX_PARAMS)
|
||||
PASS("pCnt=31 clamped to PTOOLS_MAX_PARAMS (30)");
|
||||
else
|
||||
FAIL("pCnt=31 clamped to 30", "got %d", clamped);
|
||||
}
|
||||
|
||||
static void test_clamp_large(void) {
|
||||
int clamped = clamp_pCnt(9999);
|
||||
if (clamped == PTOOLS_MAX_PARAMS)
|
||||
PASS("pCnt=9999 clamped to PTOOLS_MAX_PARAMS (30)");
|
||||
else
|
||||
FAIL("pCnt=9999 clamped to 30", "got %d", clamped);
|
||||
}
|
||||
|
||||
static void test_short_paramname_accepted(void) {
|
||||
param_result_t r = validate_param("mykey", "", 0, 1);
|
||||
if (!r.skipped_namelen && !r.skipped_keybuf && !r.skipped_snprintf
|
||||
&& strcmp(r.charBuff, "keymykey") == 0
|
||||
&& strcmp(r.keyParams_contrib, "mykey") == 0)
|
||||
PASS("short paramname accepted, charBuff = 'keymykey'");
|
||||
else
|
||||
FAIL("short paramname accepted",
|
||||
"skips=(%d,%d,%d) charBuff='%s'",
|
||||
r.skipped_namelen, r.skipped_keybuf, r.skipped_snprintf,
|
||||
r.charBuff);
|
||||
}
|
||||
|
||||
static void test_paramname_exactly_254_accepted(void) {
|
||||
char name[255];
|
||||
memset(name, 'a', 254);
|
||||
name[254] = '\0';
|
||||
param_result_t r = validate_param(name, "", 0, PTOOLS_MAX_PARAMS);
|
||||
if (!r.skipped_namelen && !r.skipped_keybuf && !r.skipped_snprintf)
|
||||
PASS("paramname len=254 accepted (boundary)");
|
||||
else
|
||||
FAIL("paramname len=254 accepted",
|
||||
"skips=(%d,%d,%d)", r.skipped_namelen, r.skipped_keybuf, r.skipped_snprintf);
|
||||
}
|
||||
|
||||
static void test_paramname_255_rejected(void) {
|
||||
char name[256];
|
||||
memset(name, 'a', 255);
|
||||
name[255] = '\0';
|
||||
param_result_t r = validate_param(name, "", 0, PTOOLS_MAX_PARAMS);
|
||||
if (r.skipped_namelen)
|
||||
PASS("paramname len=255 rejected (> 254)");
|
||||
else
|
||||
FAIL("paramname len=255 rejected", "was accepted (charBuff='%.20s...')", r.charBuff);
|
||||
}
|
||||
|
||||
static void test_paramname_overflow_rejected(void) {
|
||||
/* 1024-byte name — well above 254 */
|
||||
char name[1025];
|
||||
memset(name, 'B', 1024);
|
||||
name[1024] = '\0';
|
||||
param_result_t r = validate_param(name, "", 0, PTOOLS_MAX_PARAMS);
|
||||
if (r.skipped_namelen)
|
||||
PASS("paramname len=1024 rejected (overflow guard)");
|
||||
else
|
||||
FAIL("paramname len=1024 rejected", "was accepted");
|
||||
}
|
||||
|
||||
static void test_charBuff_snprintf_fits(void) {
|
||||
/* "key" (3) + 254-char name = 257 chars + NUL = 258 → exactly fits in charBuff[258] */
|
||||
char name[255];
|
||||
memset(name, 'c', 254);
|
||||
name[254] = '\0';
|
||||
param_result_t r = validate_param(name, "", 0, PTOOLS_MAX_PARAMS);
|
||||
if (!r.skipped_snprintf && strlen(r.charBuff) == 257)
|
||||
PASS("charBuff snprintf fits: 'key' + 254-char name = 257 chars");
|
||||
else
|
||||
FAIL("charBuff snprintf fits", "skipped=%d len=%zu", r.skipped_snprintf, strlen(r.charBuff));
|
||||
}
|
||||
|
||||
static void test_keyparams_strcat_length_check(void) {
|
||||
/*
|
||||
* With pCnt=1, keyParams buffer is 258*1=258 bytes.
|
||||
* If keyParams is already 257 bytes full and we try to add a 2-byte name,
|
||||
* needed = 2+1 = 3, used=257, used+needed=260 > 258 → must be skipped.
|
||||
*/
|
||||
char big_existing[258];
|
||||
memset(big_existing, 'x', 257);
|
||||
big_existing[257] = '\0';
|
||||
|
||||
param_result_t r = validate_param("ab", big_existing, 1, 1);
|
||||
if (r.skipped_keybuf)
|
||||
PASS("keyParams full: strcat length check rejects overflow");
|
||||
else
|
||||
FAIL("keyParams full strcat check", "param was accepted");
|
||||
}
|
||||
|
||||
static void test_second_param_keyparams_comma(void) {
|
||||
/* Second param (i=1) should contribute ",name" to keyParams */
|
||||
param_result_t r = validate_param("foo", "bar", 1, 2);
|
||||
if (!r.skipped_namelen && !r.skipped_keybuf
|
||||
&& strcmp(r.keyParams_contrib, ",foo") == 0
|
||||
&& strcmp(r.charBuff, "keyfoo") == 0)
|
||||
PASS("second param gets comma prefix in keyParams");
|
||||
else
|
||||
FAIL("second param comma prefix",
|
||||
"contrib='%s' charBuff='%s'", r.keyParams_contrib, r.charBuff);
|
||||
}
|
||||
|
||||
int main(void) {
|
||||
test_clamp_within_limit();
|
||||
test_clamp_at_limit();
|
||||
test_clamp_exceeds_limit();
|
||||
test_clamp_large();
|
||||
test_short_paramname_accepted();
|
||||
test_paramname_exactly_254_accepted();
|
||||
test_paramname_255_rejected();
|
||||
test_paramname_overflow_rejected();
|
||||
test_charBuff_snprintf_fits();
|
||||
test_keyparams_strcat_length_check();
|
||||
test_second_param_keyparams_comma();
|
||||
|
||||
printf("\n%d passed, %d failed\n", passes, failures);
|
||||
return failures ? 1 : 0;
|
||||
}
|
||||
Loading…
Reference in New Issue