clients/c: apply audit findings — security + CVE bump (a69e924 → new)
HIGH: - H1: enlarge test base_with_slash buffer 64 → 80; cmake --build now clean under -Werror=format-truncation. - H2: CURLOPT_FOLLOWLOCATION = 0 (no cross-host bearer leak; SDK talks to a known endpoint, redirects unexpected). MAXREDIRS dropped. - H3: cf_admin_revoke_token validates name [A-Za-z0-9_-]+ client-side before URL build; rejects "a/../healthz" with CF_ERR_USAGE before the request leaves the process. MEDIUM: - M1: cf_buf_append overflow guards — n + len + 1 wrap-check up front; newcap *= 2 doubling-loop bounded by SIZE_MAX/2. - M2: 64 MiB CF_MAX_RESPONSE_BYTES cap exposed on the public header; write_cb aborts the transfer once exceeded → CF_ERR_TRANSPORT. - M3: CURLOPT_CONNECTTIMEOUT_MS = 10000 (was implicit 300s default). - M4: g_curl_init_count is now _Atomic int (C11 stdatomic) using atomic_fetch_add/sub; concurrent cf_client_new/cf_client_free across threads no longer races the libcurl global init/cleanup transition. LOW: - L1: push_auth propagates CF_ERR_OOM via an out-param instead of silently dropping the Authorization header (which previously surfaced as a misleading 401 from the server). - L2: write_cb size*nmemb overflow defensive guard. CVE: - Bump vendored cJSON 1.7.15 → 1.7.18 (fixes CVE-2024-31755: cJSON_SetValuestring NULL-deref). cJSON.c/cJSON.h replaced from upstream tag v1.7.18; LICENSE file unchanged. README updated. Tests added (15 → 21): - test_revoke_token_validates_name: path-traversal name rejected, valid name proceeds through to transport. - test_buf_append_overflow_guards: synthetic SIZE_MAX-edge inputs trigger error-return rather than wrap. - test_response_body_size_cap: mock streams 65 MiB; client aborts with CF_ERR_TRANSPORT. - test_connect_timeout: dial 10.255.255.1, assert <18s wallclock (vs. libcurl's 300s default). - test_concurrent_client_init: 4 pthreads × 50 iters, no crash, no leak under valgrind. - test_cjson_bump: cJSON_SetValuestring(node, NULL) returns NULL safely; malformed cJSON_Parse returns NULL. Verification: - cmake --build build (Release): clean - ctest --test-dir build: 21/21 pass (incl. 10s connect-timeout test) - ctest --test-dir build-asan (ASan + UBSan): clean - valgrind --leak-check=full: 10,313 allocs == 10,313 frees, 0 errors, 0 leaks README updated: cJSON 1.7.18 note, C11 + stdatomic requirement. Audit: memory/clawdforge-audits/c-a69e924.md
This commit is contained in:
parent
a507ed2a00
commit
70f4dcc2a4
7 changed files with 420 additions and 35 deletions
|
|
@ -5,7 +5,9 @@ clawdforge wraps `claude -p` subprocess calls behind a bearer-token-gated REST
|
|||
API; this SDK is what you reach for from C, or what you FFI into from any
|
||||
language with a C ABI.
|
||||
|
||||
- **C99 minimum, C11 preferred.** No GNU extensions in the public surface.
|
||||
- **C11 required.** The implementation uses C11 atomics (`<stdatomic.h>`)
|
||||
for the shared libcurl-init refcount. The public header itself stays
|
||||
C99-compatible. No GNU extensions in the public surface.
|
||||
- **Dependencies:** [libcurl](https://curl.se/libcurl/) (system) + [cJSON](https://github.com/DaveGamble/cJSON) (vendored under `src/cjson/`).
|
||||
- **Errors are out-params**, not globals. Library functions never write to `stderr`.
|
||||
- **All allocations are caller-aware** — every output struct ships with a `_free()`.
|
||||
|
|
@ -251,5 +253,7 @@ shared connection pool is a future enhancement that won't change the API.
|
|||
|
||||
## Vendored cJSON
|
||||
|
||||
`src/cjson/cJSON.{c,h}` are vendored from cJSON v1.7.15 (MIT). The license
|
||||
file lives next to them.
|
||||
`src/cjson/cJSON.{c,h}` are vendored from cJSON v1.7.18 (MIT, from upstream).
|
||||
The license file lives next to them. Bumping to 1.7.18 picks up the fix for
|
||||
[CVE-2024-31755](https://nvd.nist.gov/vuln/detail/CVE-2024-31755) — a NULL
|
||||
pointer dereference reachable through `cJSON_Parse` on crafted input.
|
||||
|
|
|
|||
|
|
@ -61,6 +61,13 @@ extern "C" {
|
|||
#define CLAWDFORGE_VERSION_PATCH 0
|
||||
#define CLAWDFORGE_VERSION_STRING "0.1.0"
|
||||
|
||||
/* Maximum HTTP response body size accepted by the SDK. A response
|
||||
* larger than this aborts mid-transfer with CF_ERR_TRANSPORT, which
|
||||
* caps the heap a malicious or runaway server can force the client
|
||||
* to allocate. 64 MiB is comfortably larger than any expected
|
||||
* /run, /healthz, or /admin response. */
|
||||
#define CF_MAX_RESPONSE_BYTES ((size_t)64 * 1024 * 1024)
|
||||
|
||||
/* Returns the runtime library version string ("X.Y.Z"). */
|
||||
CF_API const char *cf_version(void);
|
||||
|
||||
|
|
|
|||
|
|
@ -96,9 +96,9 @@ CJSON_PUBLIC(const char *) cJSON_GetErrorPtr(void)
|
|||
return (const char*) (global_error.json + global_error.position);
|
||||
}
|
||||
|
||||
CJSON_PUBLIC(char *) cJSON_GetStringValue(const cJSON * const item)
|
||||
CJSON_PUBLIC(char *) cJSON_GetStringValue(const cJSON * const item)
|
||||
{
|
||||
if (!cJSON_IsString(item))
|
||||
if (!cJSON_IsString(item))
|
||||
{
|
||||
return NULL;
|
||||
}
|
||||
|
|
@ -106,9 +106,9 @@ CJSON_PUBLIC(char *) cJSON_GetStringValue(const cJSON * const item)
|
|||
return item->valuestring;
|
||||
}
|
||||
|
||||
CJSON_PUBLIC(double) cJSON_GetNumberValue(const cJSON * const item)
|
||||
CJSON_PUBLIC(double) cJSON_GetNumberValue(const cJSON * const item)
|
||||
{
|
||||
if (!cJSON_IsNumber(item))
|
||||
if (!cJSON_IsNumber(item))
|
||||
{
|
||||
return (double) NAN;
|
||||
}
|
||||
|
|
@ -117,7 +117,7 @@ CJSON_PUBLIC(double) cJSON_GetNumberValue(const cJSON * const item)
|
|||
}
|
||||
|
||||
/* This is a safeguard to prevent copy-pasters from using incompatible C and header files */
|
||||
#if (CJSON_VERSION_MAJOR != 1) || (CJSON_VERSION_MINOR != 7) || (CJSON_VERSION_PATCH != 15)
|
||||
#if (CJSON_VERSION_MAJOR != 1) || (CJSON_VERSION_MINOR != 7) || (CJSON_VERSION_PATCH != 18)
|
||||
#error cJSON.h and cJSON.c have different versions. Make sure that both have the same.
|
||||
#endif
|
||||
|
||||
|
|
@ -263,10 +263,12 @@ CJSON_PUBLIC(void) cJSON_Delete(cJSON *item)
|
|||
if (!(item->type & cJSON_IsReference) && (item->valuestring != NULL))
|
||||
{
|
||||
global_hooks.deallocate(item->valuestring);
|
||||
item->valuestring = NULL;
|
||||
}
|
||||
if (!(item->type & cJSON_StringIsConst) && (item->string != NULL))
|
||||
{
|
||||
global_hooks.deallocate(item->string);
|
||||
item->string = NULL;
|
||||
}
|
||||
global_hooks.deallocate(item);
|
||||
item = next;
|
||||
|
|
@ -397,11 +399,17 @@ CJSON_PUBLIC(double) cJSON_SetNumberHelper(cJSON *object, double number)
|
|||
return object->valuedouble = number;
|
||||
}
|
||||
|
||||
/* Note: when passing a NULL valuestring, cJSON_SetValuestring treats this as an error and return NULL */
|
||||
CJSON_PUBLIC(char*) cJSON_SetValuestring(cJSON *object, const char *valuestring)
|
||||
{
|
||||
char *copy = NULL;
|
||||
/* if object's type is not cJSON_String or is cJSON_IsReference, it should not set valuestring */
|
||||
if (!(object->type & cJSON_String) || (object->type & cJSON_IsReference))
|
||||
if ((object == NULL) || !(object->type & cJSON_String) || (object->type & cJSON_IsReference))
|
||||
{
|
||||
return NULL;
|
||||
}
|
||||
/* return NULL if the object is corrupted or valuestring is NULL */
|
||||
if (object->valuestring == NULL || valuestring == NULL)
|
||||
{
|
||||
return NULL;
|
||||
}
|
||||
|
|
@ -511,7 +519,7 @@ static unsigned char* ensure(printbuffer * const p, size_t needed)
|
|||
|
||||
return NULL;
|
||||
}
|
||||
|
||||
|
||||
memcpy(newbuffer, p->buffer, p->offset + 1);
|
||||
p->hooks.deallocate(p->buffer);
|
||||
}
|
||||
|
|
@ -562,6 +570,10 @@ static cJSON_bool print_number(const cJSON * const item, printbuffer * const out
|
|||
{
|
||||
length = sprintf((char*)number_buffer, "null");
|
||||
}
|
||||
else if(d == (double)item->valueint)
|
||||
{
|
||||
length = sprintf((char*)number_buffer, "%d", item->valueint);
|
||||
}
|
||||
else
|
||||
{
|
||||
/* Try 15 decimal places of precision to avoid nonsignificant nonzero digits */
|
||||
|
|
@ -884,6 +896,7 @@ fail:
|
|||
if (output != NULL)
|
||||
{
|
||||
input_buffer->hooks.deallocate(output);
|
||||
output = NULL;
|
||||
}
|
||||
|
||||
if (input_pointer != NULL)
|
||||
|
|
@ -1103,7 +1116,7 @@ CJSON_PUBLIC(cJSON *) cJSON_ParseWithLengthOpts(const char *value, size_t buffer
|
|||
}
|
||||
|
||||
buffer.content = (const unsigned char*)value;
|
||||
buffer.length = buffer_length;
|
||||
buffer.length = buffer_length;
|
||||
buffer.offset = 0;
|
||||
buffer.hooks = global_hooks;
|
||||
|
||||
|
|
@ -1226,6 +1239,7 @@ static unsigned char *print(const cJSON * const item, cJSON_bool format, const i
|
|||
|
||||
/* free the buffer */
|
||||
hooks->deallocate(buffer->buffer);
|
||||
buffer->buffer = NULL;
|
||||
}
|
||||
|
||||
return printed;
|
||||
|
|
@ -1234,11 +1248,13 @@ fail:
|
|||
if (buffer->buffer != NULL)
|
||||
{
|
||||
hooks->deallocate(buffer->buffer);
|
||||
buffer->buffer = NULL;
|
||||
}
|
||||
|
||||
if (printed != NULL)
|
||||
{
|
||||
hooks->deallocate(printed);
|
||||
printed = NULL;
|
||||
}
|
||||
|
||||
return NULL;
|
||||
|
|
@ -1279,6 +1295,7 @@ CJSON_PUBLIC(char *) cJSON_PrintBuffered(const cJSON *item, int prebuffer, cJSON
|
|||
if (!print_value(item, &p))
|
||||
{
|
||||
global_hooks.deallocate(p.buffer);
|
||||
p.buffer = NULL;
|
||||
return NULL;
|
||||
}
|
||||
|
||||
|
|
@ -1650,6 +1667,11 @@ static cJSON_bool parse_object(cJSON * const item, parse_buffer * const input_bu
|
|||
current_item = new_item;
|
||||
}
|
||||
|
||||
if (cannot_access_at_index(input_buffer, 1))
|
||||
{
|
||||
goto fail; /* nothing comes after the comma */
|
||||
}
|
||||
|
||||
/* parse the name of the child */
|
||||
input_buffer->offset++;
|
||||
buffer_skip_whitespace(input_buffer);
|
||||
|
|
@ -2260,7 +2282,7 @@ CJSON_PUBLIC(cJSON_bool) cJSON_InsertItemInArray(cJSON *array, int which, cJSON
|
|||
{
|
||||
cJSON *after_inserted = NULL;
|
||||
|
||||
if (which < 0)
|
||||
if (which < 0 || newitem == NULL)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
|
@ -2271,6 +2293,11 @@ CJSON_PUBLIC(cJSON_bool) cJSON_InsertItemInArray(cJSON *array, int which, cJSON
|
|||
return add_item_to_array(array, newitem);
|
||||
}
|
||||
|
||||
if (after_inserted != array->child && after_inserted->prev == NULL) {
|
||||
/* return false if after_inserted is a corrupted array item */
|
||||
return false;
|
||||
}
|
||||
|
||||
newitem->next = after_inserted;
|
||||
newitem->prev = after_inserted->prev;
|
||||
after_inserted->prev = newitem;
|
||||
|
|
@ -2287,7 +2314,7 @@ CJSON_PUBLIC(cJSON_bool) cJSON_InsertItemInArray(cJSON *array, int which, cJSON
|
|||
|
||||
CJSON_PUBLIC(cJSON_bool) cJSON_ReplaceItemViaPointer(cJSON * const parent, cJSON * const item, cJSON * replacement)
|
||||
{
|
||||
if ((parent == NULL) || (replacement == NULL) || (item == NULL))
|
||||
if ((parent == NULL) || (parent->child == NULL) || (replacement == NULL) || (item == NULL))
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
|
@ -2357,6 +2384,11 @@ static cJSON_bool replace_item_in_object(cJSON *object, const char *string, cJSO
|
|||
cJSON_free(replacement->string);
|
||||
}
|
||||
replacement->string = (char*)cJSON_strdup((const unsigned char*)string, &global_hooks);
|
||||
if (replacement->string == NULL)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
replacement->type &= ~cJSON_StringIsConst;
|
||||
|
||||
return cJSON_ReplaceItemViaPointer(object, get_object_item(object, string, case_sensitive), replacement);
|
||||
|
|
@ -2689,7 +2721,7 @@ CJSON_PUBLIC(cJSON *) cJSON_CreateStringArray(const char *const *strings, int co
|
|||
if (a && a->child) {
|
||||
a->child->prev = n;
|
||||
}
|
||||
|
||||
|
||||
return a;
|
||||
}
|
||||
|
||||
|
|
@ -3107,4 +3139,5 @@ CJSON_PUBLIC(void *) cJSON_malloc(size_t size)
|
|||
CJSON_PUBLIC(void) cJSON_free(void *object)
|
||||
{
|
||||
global_hooks.deallocate(object);
|
||||
object = NULL;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -81,7 +81,7 @@ then using the CJSON_API_VISIBILITY flag to "export" the same symbols the way CJ
|
|||
/* project version */
|
||||
#define CJSON_VERSION_MAJOR 1
|
||||
#define CJSON_VERSION_MINOR 7
|
||||
#define CJSON_VERSION_PATCH 15
|
||||
#define CJSON_VERSION_PATCH 18
|
||||
|
||||
#include <stddef.h>
|
||||
|
||||
|
|
@ -279,6 +279,13 @@ CJSON_PUBLIC(double) cJSON_SetNumberHelper(cJSON *object, double number);
|
|||
/* Change the valuestring of a cJSON_String object, only takes effect when type of object is cJSON_String */
|
||||
CJSON_PUBLIC(char*) cJSON_SetValuestring(cJSON *object, const char *valuestring);
|
||||
|
||||
/* If the object is not a boolean type this does nothing and returns cJSON_Invalid else it returns the new type*/
|
||||
#define cJSON_SetBoolValue(object, boolValue) ( \
|
||||
(object != NULL && ((object)->type & (cJSON_False|cJSON_True))) ? \
|
||||
(object)->type=((object)->type &(~(cJSON_False|cJSON_True)))|((boolValue)?cJSON_True:cJSON_False) : \
|
||||
cJSON_Invalid\
|
||||
)
|
||||
|
||||
/* Macro for iterating over an array or object */
|
||||
#define cJSON_ArrayForEach(element, array) for(element = (array != NULL) ? (array)->child : NULL; element != NULL; element = element->next)
|
||||
|
||||
|
|
|
|||
|
|
@ -49,26 +49,56 @@ static cf_status_t check_http_status(const cf_http_response_t *r,
|
|||
|
||||
/* curl_global_init/cleanup are reference-counted by libcurl. We pin
|
||||
* the global with the first cf_client_new and release on the matching
|
||||
* cf_client_free. Not thread-safe to mix init/cleanup across threads
|
||||
* as documented; users should call cf_client_new from a single thread
|
||||
* before fanning out, which is the usual pattern anyway. */
|
||||
* cf_client_free.
|
||||
*
|
||||
* M4: g_curl_init_count is a C11 _Atomic int so concurrent
|
||||
* cf_client_new / cf_client_free across threads observe the
|
||||
* transition through 0 atomically. Each cf_client_t is still
|
||||
* documented as not thread-safe to share between threads, but the
|
||||
* global init guard is shared by definition and must be race-free
|
||||
* against all callers. */
|
||||
|
||||
#include <curl/curl.h>
|
||||
#include <stdatomic.h>
|
||||
|
||||
static int g_curl_init_count = 0;
|
||||
static _Atomic int g_curl_init_count = 0;
|
||||
|
||||
static void curl_pin(void) {
|
||||
if (g_curl_init_count++ == 0) {
|
||||
/* fetch_add returns the previous value; if it was zero this is the
|
||||
* thread that needs to call curl_global_init. */
|
||||
if (atomic_fetch_add(&g_curl_init_count, 1) == 0) {
|
||||
curl_global_init(CURL_GLOBAL_DEFAULT);
|
||||
}
|
||||
}
|
||||
|
||||
static void curl_unpin(void) {
|
||||
if (--g_curl_init_count == 0) {
|
||||
/* fetch_sub returns the previous value; if it was 1 we just
|
||||
* dropped the count to 0 and own the cleanup. */
|
||||
if (atomic_fetch_sub(&g_curl_init_count, 1) == 1) {
|
||||
curl_global_cleanup();
|
||||
}
|
||||
}
|
||||
|
||||
/* H3 helper: validate an admin token name matches the server's
|
||||
* allow-list ([A-Za-z0-9_-]+). The path component is interpolated
|
||||
* directly into the URL, so anything outside this set could be used
|
||||
* to traverse to another endpoint via reverse-proxy path collapsing
|
||||
* ("a/../healthz") or to bypass server-side parsing. We reject
|
||||
* client-side rather than URL-encoding so the call fails earlier and
|
||||
* mirrors the server contract. */
|
||||
static int admin_token_name_is_safe(const char *name) {
|
||||
if (!name || !*name) return 0;
|
||||
for (const unsigned char *p = (const unsigned char *)name; *p; ++p) {
|
||||
unsigned char ch = *p;
|
||||
if ((ch >= 'a' && ch <= 'z') ||
|
||||
(ch >= 'A' && ch <= 'Z') ||
|
||||
(ch >= '0' && ch <= '9') ||
|
||||
ch == '_' || ch == '-') continue;
|
||||
return 0;
|
||||
}
|
||||
return 1;
|
||||
}
|
||||
|
||||
/* ---------- cf_client ------------------------------------------- */
|
||||
|
||||
cf_client_t *cf_client_new(const char *base_url, const char *token) {
|
||||
|
|
@ -584,8 +614,18 @@ cf_status_t cf_admin_revoke_token(cf_client_t *c,
|
|||
cf_status_t s = admin_required(c, err);
|
||||
if (s != CF_OK) return s;
|
||||
|
||||
/* Build path "/admin/tokens/<name>" — the server requires a-z0-9_-,
|
||||
* so no URL-encoding needed for valid names; we still bound the size. */
|
||||
/* H3: validate name client-side against the server's allow-list
|
||||
* before interpolating into the URL. Without this, "a/../healthz"
|
||||
* collapses through a reverse proxy to /admin/healthz before the
|
||||
* server's auth + parsing has a chance to reject it. */
|
||||
if (!admin_token_name_is_safe(name)) {
|
||||
return cf_err_set(err, CF_ERR_USAGE, 0,
|
||||
"invalid token name: must match [A-Za-z0-9_-]+ (got %.64s)",
|
||||
name);
|
||||
}
|
||||
|
||||
/* Build path "/admin/tokens/<name>". Validated above; no URL-encoding
|
||||
* needed and the alphabet is ASCII so byte == char. */
|
||||
char path[256];
|
||||
int n = snprintf(path, sizeof path, "/admin/tokens/%s", name);
|
||||
if (n <= 0 || (size_t)n >= sizeof path) {
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@
|
|||
|
||||
#include "internal.h"
|
||||
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
|
|
@ -20,10 +21,17 @@ void cf_buf_init(cf_buf_t *b) {
|
|||
}
|
||||
|
||||
int cf_buf_append(cf_buf_t *b, const void *src, size_t n) {
|
||||
/* Guard the additive size computation. need = b->len + n + 1.
|
||||
* If n > SIZE_MAX - b->len - 1 the addition wraps. */
|
||||
if (n > SIZE_MAX - b->len - 1) return -1;
|
||||
if (b->len + n + 1 > b->cap) {
|
||||
size_t need = b->len + n + 1;
|
||||
size_t newcap = b->cap ? b->cap : 256;
|
||||
while (newcap < need) newcap *= 2;
|
||||
/* Cap the doubling so newcap *= 2 cannot wrap. */
|
||||
while (newcap < need) {
|
||||
if (newcap > SIZE_MAX / 2) return -1;
|
||||
newcap *= 2;
|
||||
}
|
||||
char *p = (char *)realloc(b->data, newcap);
|
||||
if (!p) return -1;
|
||||
b->data = p;
|
||||
|
|
@ -72,25 +80,48 @@ static char *make_url(const char *base, const char *path) {
|
|||
return url;
|
||||
}
|
||||
|
||||
/* push_auth returns NULL on OOM (caller must treat as error). On success
|
||||
* returns the new list head. If there is no token to inject (override==NULL
|
||||
* and client has no token), returns the original list unchanged.
|
||||
*
|
||||
* L1 fix: previously this silently fell back to the original list on OOM,
|
||||
* which meant the request went unauthenticated and the server returned
|
||||
* 401 — the caller saw a misleading "auth" error. Now OOM is propagated. */
|
||||
static struct curl_slist *push_auth(struct curl_slist *list,
|
||||
const struct cf_client *c,
|
||||
const char *override) {
|
||||
const char *override,
|
||||
int *out_oom) {
|
||||
*out_oom = 0;
|
||||
const char *tok = override ? override : c->token;
|
||||
if (!tok || !*tok) return list;
|
||||
/* "Authorization: Bearer " + tok */
|
||||
size_t n = strlen(tok) + 32;
|
||||
char *line = (char *)malloc(n);
|
||||
if (!line) return list;
|
||||
if (!line) { *out_oom = 1; return list; }
|
||||
snprintf(line, n, "Authorization: Bearer %s", tok);
|
||||
struct curl_slist *next = curl_slist_append(list, line);
|
||||
free(line);
|
||||
return next ? next : list;
|
||||
if (!next) { *out_oom = 1; return list; }
|
||||
return next;
|
||||
}
|
||||
|
||||
/* libcurl write callback. */
|
||||
/* libcurl write callback.
|
||||
*
|
||||
* M2: enforce CF_MAX_RESPONSE_BYTES so a malicious server cannot stream
|
||||
* gigabytes into the client heap before perform() returns. Aborting from
|
||||
* the write callback (returning < n) causes libcurl to surface
|
||||
* CURLE_WRITE_ERROR which we map to CF_ERR_TRANSPORT.
|
||||
*
|
||||
* L2: defensive size*nmemb overflow guard. libcurl historically guarantees
|
||||
* size==1, but the prototype permits arbitrary multiplication so we guard. */
|
||||
static size_t write_cb(char *ptr, size_t size, size_t nmemb, void *user) {
|
||||
cf_buf_t *buf = (cf_buf_t *)user;
|
||||
if (size && nmemb > SIZE_MAX / size) return 0;
|
||||
size_t n = size * nmemb;
|
||||
if (buf->len > CF_MAX_RESPONSE_BYTES || n > CF_MAX_RESPONSE_BYTES - buf->len) {
|
||||
/* Abort: response body cap exceeded. */
|
||||
return 0;
|
||||
}
|
||||
if (cf_buf_append(buf, ptr, n) != 0) return 0;
|
||||
return n;
|
||||
}
|
||||
|
|
@ -131,7 +162,14 @@ static cf_status_t do_request(const struct cf_client *c,
|
|||
}
|
||||
|
||||
struct curl_slist *headers = NULL;
|
||||
headers = push_auth(headers, c, bearer_override);
|
||||
int auth_oom = 0;
|
||||
headers = push_auth(headers, c, bearer_override, &auth_oom);
|
||||
if (auth_oom) {
|
||||
curl_slist_free_all(headers);
|
||||
curl_easy_cleanup(h);
|
||||
free(url);
|
||||
return cf_err_set(err, CF_ERR_OOM, 0, "out of memory building Authorization header");
|
||||
}
|
||||
|
||||
curl_mime *mime = NULL;
|
||||
char errbuf[CURL_ERROR_SIZE] = {0};
|
||||
|
|
@ -140,11 +178,17 @@ static cf_status_t do_request(const struct cf_client *c,
|
|||
curl_easy_setopt(h, CURLOPT_WRITEFUNCTION, write_cb);
|
||||
curl_easy_setopt(h, CURLOPT_WRITEDATA, &out->body);
|
||||
curl_easy_setopt(h, CURLOPT_TIMEOUT, c->timeout_secs);
|
||||
curl_easy_setopt(h, CURLOPT_CONNECTTIMEOUT_MS, 10000L);
|
||||
curl_easy_setopt(h, CURLOPT_NOSIGNAL, 1L);
|
||||
curl_easy_setopt(h, CURLOPT_ERRORBUFFER, errbuf);
|
||||
curl_easy_setopt(h, CURLOPT_USERAGENT, "clawdforge-c/" CLAWDFORGE_VERSION_STRING);
|
||||
curl_easy_setopt(h, CURLOPT_FOLLOWLOCATION, 1L);
|
||||
curl_easy_setopt(h, CURLOPT_MAXREDIRS, 4L);
|
||||
/* H2: do NOT follow redirects. The SDK speaks to a known endpoint;
|
||||
* any 3xx is unexpected and following one would replay the
|
||||
* Authorization: Bearer header to the redirect target — a cross-host
|
||||
* bearer leak. CURLOPT_UNRESTRICTED_AUTH only governs USERPWD, not
|
||||
* custom Authorization headers, so disabling FOLLOWLOCATION is the
|
||||
* correct fix. */
|
||||
curl_easy_setopt(h, CURLOPT_FOLLOWLOCATION, 0L);
|
||||
|
||||
switch (opts->method) {
|
||||
case CF_GET:
|
||||
|
|
|
|||
|
|
@ -20,10 +20,12 @@
|
|||
#include <netinet/in.h>
|
||||
#include <pthread.h>
|
||||
#include <signal.h>
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <sys/socket.h>
|
||||
#include <sys/time.h>
|
||||
#include <sys/types.h>
|
||||
#include <unistd.h>
|
||||
#include <fcntl.h>
|
||||
|
|
@ -32,6 +34,17 @@
|
|||
#include <clawdforge.h>
|
||||
#include "cJSON.h"
|
||||
|
||||
/* Internal symbols used by the buffer-overflow guard test. The static
|
||||
* library exports them; these are not part of the public ABI. */
|
||||
typedef struct cf_buf {
|
||||
char *data;
|
||||
size_t len;
|
||||
size_t cap;
|
||||
} cf_buf_t_test;
|
||||
extern void cf_buf_init(cf_buf_t_test *b);
|
||||
extern int cf_buf_append(cf_buf_t_test *b, const void *src, size_t n);
|
||||
extern void cf_buf_free(cf_buf_t_test *b);
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* tiny test harness */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
|
@ -89,6 +102,11 @@ typedef struct mock_response {
|
|||
int status;
|
||||
char *content_type;
|
||||
char *body;
|
||||
/* When stream_bytes > 0, the response body is `stream_bytes` ASCII
|
||||
* 'x' characters generated on the fly from a small fixed buffer
|
||||
* (no big malloc). Used to exercise the response-body cap without
|
||||
* allocating tens of megabytes server-side. */
|
||||
long stream_bytes;
|
||||
} mock_response_t;
|
||||
|
||||
#define MAX_RESPONSES 16
|
||||
|
|
@ -120,6 +138,18 @@ static void enqueue_response(int status, const char *content_type, const char *b
|
|||
r->status = status;
|
||||
r->content_type = content_type ? strdup(content_type) : strdup("application/json");
|
||||
r->body = body ? strdup(body) : strdup("");
|
||||
r->stream_bytes = 0;
|
||||
q_resp_tail = (q_resp_tail + 1) % MAX_RESPONSES;
|
||||
pthread_mutex_unlock(&q_mu);
|
||||
}
|
||||
|
||||
static void enqueue_stream_response(int status, long bytes) {
|
||||
pthread_mutex_lock(&q_mu);
|
||||
mock_response_t *r = &q_resp[q_resp_tail];
|
||||
r->status = status;
|
||||
r->content_type = strdup("application/octet-stream");
|
||||
r->body = NULL;
|
||||
r->stream_bytes = bytes;
|
||||
q_resp_tail = (q_resp_tail + 1) % MAX_RESPONSES;
|
||||
pthread_mutex_unlock(&q_mu);
|
||||
}
|
||||
|
|
@ -235,6 +265,9 @@ static int read_request(int fd, mock_request_t *req) {
|
|||
}
|
||||
|
||||
static void write_response(int fd, const mock_response_t *resp) {
|
||||
size_t body_len = resp->stream_bytes > 0
|
||||
? (size_t)resp->stream_bytes
|
||||
: (resp->body ? strlen(resp->body) : 0);
|
||||
char header[512];
|
||||
int n = snprintf(header, sizeof header,
|
||||
"HTTP/1.1 %d %s\r\n"
|
||||
|
|
@ -248,11 +281,24 @@ static void write_response(int fd, const mock_response_t *resp) {
|
|||
resp->status == 404 ? "Not Found" :
|
||||
resp->status == 502 ? "Bad Gateway" : "Other",
|
||||
resp->content_type ? resp->content_type : "application/json",
|
||||
resp->body ? strlen(resp->body) : 0);
|
||||
body_len);
|
||||
if (n > 0) {
|
||||
ssize_t wrote = write(fd, header, (size_t)n);
|
||||
(void)wrote;
|
||||
if (resp->body && *resp->body) {
|
||||
if (resp->stream_bytes > 0) {
|
||||
/* Stream `stream_bytes` ASCII 'x' from a small chunk buffer.
|
||||
* Stops early on EPIPE — clients that abort write_cb hang up
|
||||
* mid-transfer, which is exactly what we want to test. */
|
||||
char chunk[8192];
|
||||
memset(chunk, 'x', sizeof chunk);
|
||||
size_t remaining = (size_t)resp->stream_bytes;
|
||||
while (remaining > 0) {
|
||||
size_t step = remaining < sizeof chunk ? remaining : sizeof chunk;
|
||||
ssize_t w = write(fd, chunk, step);
|
||||
if (w <= 0) break;
|
||||
remaining -= (size_t)w;
|
||||
}
|
||||
} else if (resp->body && *resp->body) {
|
||||
wrote = write(fd, resp->body, strlen(resp->body));
|
||||
(void)wrote;
|
||||
}
|
||||
|
|
@ -280,7 +326,7 @@ static void *server_loop(void *arg) {
|
|||
free(resp.body);
|
||||
} else {
|
||||
/* No script left — return 500. */
|
||||
mock_response_t fb = {500, strdup("text/plain"), strdup("no scripted response")};
|
||||
mock_response_t fb = {500, strdup("text/plain"), strdup("no scripted response"), 0};
|
||||
write_response(cfd, &fb);
|
||||
free(fb.content_type);
|
||||
free(fb.body);
|
||||
|
|
@ -749,7 +795,7 @@ static void t_url_normalisation(void) {
|
|||
"{\"ok\":true,\"claude_present\":false}");
|
||||
|
||||
/* Trailing slash should be stripped. */
|
||||
char base_with_slash[64];
|
||||
char base_with_slash[80];
|
||||
snprintf(base_with_slash, sizeof base_with_slash, "%s/", make_base_url());
|
||||
cf_client_t *c = cf_client_new(base_with_slash, "tok");
|
||||
cf_healthz_t h = {0};
|
||||
|
|
@ -761,6 +807,202 @@ static void t_url_normalisation(void) {
|
|||
cf_client_free(c);
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* New tests added by audit-fix pass */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
/* H3 follow-up: cf_admin_revoke_token must reject names that are
|
||||
* not [A-Za-z0-9_-]+ before it interpolates into the URL. */
|
||||
static void t_revoke_token_validates_name(void) {
|
||||
TEST("revoke_token_validates_name");
|
||||
cf_client_t *c = cf_client_new("http://127.0.0.1:1", NULL);
|
||||
cf_client_set_admin_token(c, "admin");
|
||||
cf_error_t err = {0};
|
||||
|
||||
/* Path-traversal attempt: must be rejected client-side. */
|
||||
cf_status_t s = cf_admin_revoke_token(c, "a/../healthz", &err);
|
||||
CHECK(s == CF_ERR_USAGE);
|
||||
CHECK(err.message != NULL);
|
||||
if (err.message) CHECK(strstr(err.message, "invalid token name") != NULL);
|
||||
cf_error_free(&err);
|
||||
|
||||
/* Empty name */
|
||||
s = cf_admin_revoke_token(c, "", &err);
|
||||
CHECK(s == CF_ERR_USAGE);
|
||||
cf_error_free(&err);
|
||||
|
||||
/* URL-encoded byte still rejected — raw byte not in allow-list. */
|
||||
s = cf_admin_revoke_token(c, "a%2F..%2Fhealthz", &err);
|
||||
CHECK(s == CF_ERR_USAGE);
|
||||
cf_error_free(&err);
|
||||
|
||||
/* A valid name still proceeds to a network call (which fails because
|
||||
* no server is listening at :1, surfacing CF_ERR_TRANSPORT — not
|
||||
* CF_ERR_USAGE — proving the validator let it through). */
|
||||
s = cf_admin_revoke_token(c, "valid-name_123", &err);
|
||||
CHECK(s == CF_ERR_TRANSPORT);
|
||||
cf_error_free(&err);
|
||||
|
||||
cf_client_free(c);
|
||||
}
|
||||
|
||||
/* M1: cf_buf_append guards both the additive overflow (n + len + 1) and
|
||||
* the doubling overflow (newcap *= 2). The guards must fire on the size
|
||||
* computation alone — neither realloc nor memcpy is reached on either
|
||||
* synthetic input, so data may safely stay NULL. */
|
||||
static void t_buf_append_overflow_guards(void) {
|
||||
TEST("buf_append_overflow_guards");
|
||||
cf_buf_t_test b;
|
||||
cf_buf_init(&b);
|
||||
|
||||
/* (a) additive overflow: n + len + 1 wraps. */
|
||||
b.data = NULL;
|
||||
b.cap = 0;
|
||||
b.len = SIZE_MAX - 4;
|
||||
int rc = cf_buf_append(&b, "abcd", 5); /* 5 > SIZE_MAX - (SIZE_MAX-4) - 1 = 3 */
|
||||
CHECK(rc == -1);
|
||||
|
||||
/* (b) doubling overflow: cap is already past SIZE_MAX/2, and n
|
||||
* forces need > cap so the loop runs. The first iteration must
|
||||
* trip the `newcap > SIZE_MAX/2` guard. */
|
||||
b.data = NULL;
|
||||
b.len = 0;
|
||||
b.cap = (SIZE_MAX / 2) + 100;
|
||||
rc = cf_buf_append(&b, "x", SIZE_MAX - 200);
|
||||
CHECK(rc == -1);
|
||||
|
||||
/* Reset to a clean state for the sanity check / free. */
|
||||
b.data = NULL;
|
||||
b.len = 0;
|
||||
b.cap = 0;
|
||||
|
||||
/* (c) sanity: a normal small append still works. */
|
||||
rc = cf_buf_append(&b, "hello", 5);
|
||||
CHECK(rc == 0);
|
||||
CHECK(b.len == 5);
|
||||
CHECK(b.data != NULL);
|
||||
if (b.data) CHECK(strcmp(b.data, "hello") == 0);
|
||||
cf_buf_free(&b);
|
||||
}
|
||||
|
||||
/* M2: a server response larger than CF_MAX_RESPONSE_BYTES must abort
|
||||
* mid-transfer, not silently consume the heap. */
|
||||
static void t_response_body_size_cap(void) {
|
||||
TEST("response_body_size_cap");
|
||||
reset_captures();
|
||||
/* CF_MAX_RESPONSE_BYTES is 64 MiB. Stream 65 MiB so we cross the cap. */
|
||||
long over_cap = (long)CF_MAX_RESPONSE_BYTES + (1L << 20);
|
||||
enqueue_stream_response(200, over_cap);
|
||||
|
||||
cf_client_t *c = cf_client_new(make_base_url(), "tok");
|
||||
cf_client_set_timeout_secs(c, 30);
|
||||
cf_healthz_t h = {0};
|
||||
cf_error_t err = {0};
|
||||
cf_status_t s = cf_healthz(c, &h, &err);
|
||||
CHECK(s == CF_ERR_TRANSPORT);
|
||||
CHECK(err.message != NULL);
|
||||
|
||||
cf_healthz_free(&h);
|
||||
cf_error_free(&err);
|
||||
cf_client_free(c);
|
||||
}
|
||||
|
||||
/* M3: with no CONNECTTIMEOUT set, libcurl waits ~300s. We set 10s,
|
||||
* so a connect to 10.255.255.1 (RFC1918 unroutable) must fail well
|
||||
* before the test-suite timeout (60s). */
|
||||
static void t_connect_timeout(void) {
|
||||
TEST("connect_timeout");
|
||||
cf_client_t *c = cf_client_new("http://10.255.255.1:80", "tok");
|
||||
/* Total timeout high so we know the connect-timeout is what trips. */
|
||||
cf_client_set_timeout_secs(c, 30);
|
||||
cf_healthz_t h = {0};
|
||||
cf_error_t err = {0};
|
||||
|
||||
struct timeval t0, t1;
|
||||
gettimeofday(&t0, NULL);
|
||||
cf_status_t s = cf_healthz(c, &h, &err);
|
||||
gettimeofday(&t1, NULL);
|
||||
double elapsed = (t1.tv_sec - t0.tv_sec) +
|
||||
(t1.tv_usec - t0.tv_usec) / 1e6;
|
||||
|
||||
CHECK(s == CF_ERR_TRANSPORT);
|
||||
/* Allow 18s slack for slow CI; the libcurl default of 300s would
|
||||
* blow the test-suite timeout long before this. */
|
||||
CHECK(elapsed < 18.0);
|
||||
|
||||
cf_healthz_free(&h);
|
||||
cf_error_free(&err);
|
||||
cf_client_free(c);
|
||||
}
|
||||
|
||||
/* M4: g_curl_init_count must survive concurrent cf_client_new /
|
||||
* cf_client_free across threads without leaking, double-cleaning, or
|
||||
* tearing down libcurl while another thread is mid-request. */
|
||||
typedef struct {
|
||||
int iters;
|
||||
} init_thread_arg_t;
|
||||
|
||||
static void *init_worker(void *arg) {
|
||||
init_thread_arg_t *a = (init_thread_arg_t *)arg;
|
||||
for (int i = 0; i < a->iters; ++i) {
|
||||
cf_client_t *c = cf_client_new("http://127.0.0.1:1", "tok");
|
||||
if (c) {
|
||||
cf_client_set_timeout_secs(c, 1);
|
||||
cf_client_free(c);
|
||||
}
|
||||
}
|
||||
return NULL;
|
||||
}
|
||||
|
||||
static void t_concurrent_client_init(void) {
|
||||
TEST("concurrent_client_init");
|
||||
enum { N = 4 };
|
||||
pthread_t th[N];
|
||||
init_thread_arg_t args[N];
|
||||
for (int i = 0; i < N; ++i) {
|
||||
args[i].iters = 50;
|
||||
int rc = pthread_create(&th[i], NULL, init_worker, &args[i]);
|
||||
CHECK(rc == 0);
|
||||
}
|
||||
for (int i = 0; i < N; ++i) {
|
||||
pthread_join(th[i], NULL);
|
||||
}
|
||||
/* If we're still running with no crash and no SIGSEGV, the atomic
|
||||
* refcount held. valgrind/asan will catch any heap corruption. */
|
||||
CHECK(1);
|
||||
}
|
||||
|
||||
/* CVE-2024-31755: prior to cJSON 1.7.18, calling cJSON_SetValuestring
|
||||
* with a NULL `valuestring` argument on a string node would deref NULL
|
||||
* inside strlen. The fix is a NULL check that returns NULL safely. */
|
||||
static void t_cjson_bump(void) {
|
||||
TEST("cjson_bump");
|
||||
cJSON *node = cJSON_CreateString("seed");
|
||||
CHECK(node != NULL);
|
||||
if (node) {
|
||||
/* Pre-fix: this call crashed. Post-fix: returns NULL. */
|
||||
char *r = cJSON_SetValuestring(node, NULL);
|
||||
CHECK(r == NULL);
|
||||
/* The original valuestring must be untouched. */
|
||||
CHECK(cJSON_IsString(node));
|
||||
if (cJSON_IsString(node)) {
|
||||
CHECK_STR_EQ(node->valuestring, "seed");
|
||||
}
|
||||
cJSON_Delete(node);
|
||||
}
|
||||
|
||||
/* Belt-and-braces: parsing crafted JSON also returns sanely (NULL
|
||||
* or a node), never crashes. */
|
||||
const char *crafted = "{\"a\":[1,2,{\"b\":\"c\"}],\"d\":null}";
|
||||
cJSON *parsed = cJSON_Parse(crafted);
|
||||
CHECK(parsed != NULL);
|
||||
if (parsed) cJSON_Delete(parsed);
|
||||
|
||||
/* And a malformed input: must return NULL, not crash. */
|
||||
cJSON *bad = cJSON_Parse("{\"a\":");
|
||||
CHECK(bad == NULL);
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* main */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
|
@ -790,6 +1032,14 @@ int main(void) {
|
|||
t_admin_requires_token();
|
||||
t_url_normalisation();
|
||||
|
||||
/* Audit-fix tests */
|
||||
t_revoke_token_validates_name();
|
||||
t_buf_append_overflow_guards();
|
||||
t_response_body_size_cap();
|
||||
t_connect_timeout();
|
||||
t_concurrent_client_init();
|
||||
t_cjson_bump();
|
||||
|
||||
stop_server();
|
||||
|
||||
fprintf(stderr, "\n%d tests, %d failures\n", g_tests, g_failures);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue