From 70f4dcc2a4543b49170c1e560d5570118a9a00e8 Mon Sep 17 00:00:00 2001 From: Kayos Date: Tue, 28 Apr 2026 23:24:59 -0700 Subject: [PATCH] =?UTF-8?q?clients/c:=20apply=20audit=20findings=20?= =?UTF-8?q?=E2=80=94=20security=20+=20CVE=20bump=20(a69e924=20=E2=86=92=20?= =?UTF-8?q?new)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- clients/c/README.md | 10 +- clients/c/include/clawdforge.h | 7 + clients/c/src/cjson/cJSON.c | 55 +++++-- clients/c/src/cjson/cJSON.h | 9 +- clients/c/src/client.c | 56 ++++++- clients/c/src/http.c | 60 +++++++- clients/c/tests/test_client.c | 258 ++++++++++++++++++++++++++++++++- 7 files changed, 420 insertions(+), 35 deletions(-) diff --git a/clients/c/README.md b/clients/c/README.md index 13e4981..8fdd39b 100644 --- a/clients/c/README.md +++ b/clients/c/README.md @@ -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 (``) + 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. diff --git a/clients/c/include/clawdforge.h b/clients/c/include/clawdforge.h index 6578480..78df726 100644 --- a/clients/c/include/clawdforge.h +++ b/clients/c/include/clawdforge.h @@ -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); diff --git a/clients/c/src/cjson/cJSON.c b/clients/c/src/cjson/cJSON.c index 3063f74..61483d9 100644 --- a/clients/c/src/cjson/cJSON.c +++ b/clients/c/src/cjson/cJSON.c @@ -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; } diff --git a/clients/c/src/cjson/cJSON.h b/clients/c/src/cjson/cJSON.h index 92907a2..88cf0bc 100644 --- a/clients/c/src/cjson/cJSON.h +++ b/clients/c/src/cjson/cJSON.h @@ -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 @@ -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) diff --git a/clients/c/src/client.c b/clients/c/src/client.c index d088b40..ac6fbe3 100644 --- a/clients/c/src/client.c +++ b/clients/c/src/client.c @@ -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 +#include -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/" — 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/". 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) { diff --git a/clients/c/src/http.c b/clients/c/src/http.c index f46398c..0b0bafa 100644 --- a/clients/c/src/http.c +++ b/clients/c/src/http.c @@ -7,6 +7,7 @@ #include "internal.h" +#include #include #include #include @@ -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: diff --git a/clients/c/tests/test_client.c b/clients/c/tests/test_client.c index 756a4d5..d9aa430 100644 --- a/clients/c/tests/test_client.c +++ b/clients/c/tests/test_client.c @@ -20,10 +20,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -32,6 +34,17 @@ #include #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);