diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dec142..b580f25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,36 @@ All notable changes to EigenScript are documented here. ### Security +- **Content-Length was matched by substring, so a header value could frame the + body (#715).** The read loop located the header with a single + `strcasestr(reqbuf, "Content-Length:")` over the whole header block, so the + first hit anywhere won — inside another header's *value* + (`X-Note: Content-Length: 0`), as a suffix of another header's *name* + (`X-Content-Length: 0`), or in the request target + (`POST /x?q=Content-Length:0`). The server framed the body at the decoy's + length, left the read loop early, and dispatched the `code` route with an + empty body; it also let a client hide an oversized real length behind a small + fake one and dodge the `> max_body` 400. The header block is now walked line + by line, matching only at a line start with the colon required immediately + after the name, and two `Content-Length` headers that disagree are rejected + with 400 instead of the first silently winning. Reachable only when the body + arrives in a *separate* write from the headers — sent as one packet the body + is already buffered when the loop breaks, which is why the pre-fix build + passes a single-write version of the same test (HS29–HS31). + +- **`http_post` did not strip CR/LF from outbound header names or values + (#716).** Both halves reached curl's `-H` verbatim, so a `\r\n` in either + injected additional headers into the outbound request — the class the + runtime already recognised one screen away in `http_cors`. Script-controlled, + so not a vulnerability on its own under the SECURITY.md threat model, but a + `code` route that forwards a client-supplied header value into an + `http_post` makes it one. Both now run through a shared `http_strip_crlf`, + which `http_cors` was rewritten onto so there is one implementation (HS32). + SECURITY.md now also states outright that the *destination* of an outbound + request is out of scope — there is no loopback/link-local blocklist, and the + guarded transport (`--proto`/`--proto-redir` pinned to `http,https`) was + inviting the assumption that the destination was guarded too. + - **`chunk_verify` let an untrusted chunk run off the end of its code (#721).** The verifier checked opcodes, operand bounds and jump targets but never that execution *terminates* — "the caller supplies a chunk ending in diff --git a/SECURITY.md b/SECURITY.md index e39deba..200af41 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -50,13 +50,23 @@ Out of scope: - Anything an EigenScript program can already do by virtue of running on the host (reading `~/.ssh/id_rsa`, deleting files, etc.). If the threat is "malicious script", the fix is "do not run malicious scripts". +- **The destination of an outbound request.** `http_get`/`http_post` check only + that the URL is `http://` or `https://` and is not a curl option; there is no + loopback, link-local, or private-range blocklist. A script that builds a URL + out of request data is therefore an SSRF pivot — reaching `127.0.0.1` or + `169.254.169.254` is the script's decision, and the script is trusted. + Stated explicitly because the transport side *is* guarded (`--proto` and + `--proto-redir` are pinned to `http,https`, which closes `file://` via + redirect), and a half-guarded client invites the assumption that the + destination is guarded too. **If you write a route that forwards a + client-supplied URL, you must validate the destination yourself.** ## Supported Versions | Version | Supported | |---------|-----------| -| 0.26.x | Yes | -| < 0.26 | No | +| 0.33.x | Yes | +| < 0.33 | No | ## Contact Routing diff --git a/src/ext_http.c b/src/ext_http.c index 969685e..d3d8c3e 100644 --- a/src/ext_http.c +++ b/src/ext_http.c @@ -352,6 +352,19 @@ Value* builtin_http_session_id(Value *arg) { } +/* Remove CR and LF in place. A CR or LF inside a header name or value ends the + * header early on the wire, so whatever follows is read by the peer as further + * headers (or as the start of the body) — header injection. Callers that build + * a header from script-supplied text must run both halves through this. */ +static void http_strip_crlf(char *s) { + if (!s) return; + char *w = s; + for (const char *r = s; *r; r++) { + if (*r != '\r' && *r != '\n') *w++ = *r; + } + *w = '\0'; +} + static int http_url_is_allowed(const char *url) { if (!url || !url[0] || url[0] == '-') return 0; return strncmp(url, "http://", 7) == 0 || strncmp(url, "https://", 8) == 0; @@ -400,6 +413,13 @@ Value* builtin_http_post(Value *arg) { for (int i = 0; i + 1 < hdr_obj->data.list.count && hdr_count < 32 && argc < 90; i += 2) { char *hk = value_to_string(hdr_obj->data.list.items[i]); char *hv = value_to_string(hdr_obj->data.list.items[i + 1]); + /* Both halves reach curl's -H verbatim, so a CR/LF in either injects + * extra headers into the outbound request. Script-controlled and so + * not a vulnerability on its own under SECURITY.md's threat model, + * but a `code` route that forwards a client-supplied header value + * into an http_post makes it one. */ + http_strip_crlf(hk); + http_strip_crlf(hv); snprintf(header_bufs[hdr_count], sizeof(header_bufs[0]), "%s: %s", hk, hv); free(hk); free(hv); argv[argc++] = "-H"; @@ -929,6 +949,61 @@ static double monotonic_now(void) { return (double)ts.tv_sec + (double)ts.tv_nsec / 1e9; } +/* Find the request's Content-Length by walking the header block LINE BY LINE. + * + * The previous strcasestr() over the whole block took the first match anywhere, + * so a decoy anywhere earlier framed the body: inside another header's VALUE + * (`X-Note: Content-Length: 0`), as a suffix of another header's NAME + * (`X-Content-Length: 0`), or in the request target + * (`POST /x?q=Content-Length:0`). The server then framed the body at the decoy's + * length and broke out of the read loop, dispatching a `code` route with an + * empty or partial body — and let a client hide an oversized real length behind + * a small fake one, dodging the `> max_body` 400. + * + * Matching only at a line start, with the colon required immediately after the + * name (RFC 7230 forbids whitespace there), closes all three. The request line + * is skipped outright rather than relying on it never starting with the name. + * + * Returns 1 and sets *out_len when present, 0 when absent, -1 when malformed — + * unparseable, trailing garbage, or two Content-Length headers that disagree + * (previously the first silently won). + * + * `block` is NUL-terminated at the end of the header block, terminator included. + */ +static int http_find_content_length(const char *block, long *out_len) { + const char *p = strstr(block, "\r\n"); + if (!p) return 0; /* request line only, no headers */ + p += 2; + + int found = 0; + long value = 0; + + while (*p) { + if (p[0] == '\r' && p[1] == '\n') break; /* blank line: block ends */ + const char *eol = strstr(p, "\r\n"); + size_t linelen = eol ? (size_t)(eol - p) : strlen(p); + + if (linelen > 14 && strncasecmp(p, "Content-Length", 14) == 0 && p[14] == ':') { + const char *v = p + 15; + while (*v == ' ' || *v == '\t') v++; + char *end = NULL; + long n = strtol(v, &end, 10); + if (end == v) return -1; /* no digits at all */ + while (*end == ' ' || *end == '\t') end++; + if (end != p + linelen) return -1; /* trailing garbage */ + if (found && n != value) return -1; /* conflicting duplicates */ + found = 1; + value = n; + } + + if (!eol) break; + p = eol + 2; + } + + if (found) *out_len = value; + return found; +} + static void handle_request(int fd) { /* Per-read timeout (backstop for fully idle sockets) */ struct timeval tv = { .tv_sec = HTTP_READ_TIMEOUT_SEC, .tv_usec = 0 }; @@ -1024,16 +1099,16 @@ static void handle_request(int fd) { /* Search only within headers (before \r\n\r\n), not in body */ char saved = reqbuf[header_end]; reqbuf[header_end] = '\0'; - char *cl = strcasestr(reqbuf, "Content-Length:"); + long content_length = 0; + int cl = http_find_content_length(reqbuf, &content_length); reqbuf[header_end] = saved; - if (cl) { - /* atoi silently accepts negative and non-numeric input; a - * negative Content-Length would make body_received trivially - * >= content_length and exit the read loop mid-body. Parse - * with strtol and reject anything outside [0, max_body]. */ - char *clend = NULL; - long content_length = strtol(cl + 15, &clend, 10); - if (clend == cl + 15 || content_length < 0 || content_length > max_body) { + if (cl != 0) { + /* strtol rather than atoi, which silently accepts negative and + * non-numeric input; a negative Content-Length would make + * body_received trivially >= content_length and exit the read + * loop mid-body. Reject anything outside [0, max_body], and any + * header the line parser flagged as malformed. */ + if (cl < 0 || content_length < 0 || content_length > max_body) { /* Malformed or oversized Content-Length — answer 400 so * the client sees a real error instead of hanging until * the per-connection deadline. */ @@ -1463,15 +1538,8 @@ static Value* builtin_http_cors(Value *arg) { if (arg->type != VAL_STR) return make_null(); free(g_server.cors_origin); /* Strip CR/LF to prevent header injection */ - const char *raw = arg->data.str; - size_t len = strlen(raw); - char *clean = xmalloc(len + 1); - size_t j = 0; - for (size_t i = 0; i < len; i++) { - if (raw[i] != '\r' && raw[i] != '\n') - clean[j++] = raw[i]; - } - clean[j] = '\0'; + char *clean = xstrdup(arg->data.str); + http_strip_crlf(clean); g_server.cors_origin = clean; return make_str(clean); } diff --git a/tests/test_http_server.sh b/tests/test_http_server.sh index f54069c..1fa2272 100755 --- a/tests/test_http_server.sh +++ b/tests/test_http_server.sh @@ -70,6 +70,12 @@ r_acleanup is http_route of ["GET", "/acleanup", "code", "shared_delete of \"req # The authed route itself: returns "top secret" iff auth passes. secret is http_route_authed of ["GET", "/secret", "code", "\"top secret\""] +# Echoes the request body back — the framing oracle for HS29/HS30. +r_echo is http_route of ["POST", "/echo", "code", "http_request_body of null"] + +# Echoes the raw request header block — the injection oracle for HS32. +r_hdrs is http_route of ["POST", "/hdrs", "code", "http_request_headers of null"] + # Serve forever. serve is http_serve of $PORT EIGS @@ -501,6 +507,101 @@ fi # that may rerun with a different cap. curl -s --max-time 2 "http://127.0.0.1:$PORT/acleanup" > /dev/null +# ---- Content-Length is matched per header line, not by substring (#715) ---- +# Content-Length used to be found with strcasestr() over the whole header +# block, so the first hit anywhere won: inside another header's VALUE, as a +# suffix of another header's NAME, or in the request target. The server framed +# the body at the decoy's length and dispatched the route with an empty body. +# +# The headers and the body MUST go in separate writes. Sent as one packet the +# body is already in reqbuf when the loop breaks early, so the bug is invisible +# — the pre-fix build passes a single-write version of this test. +ECHO_BODY='HELLO-REAL-BODY-0123456789' # 26 bytes +if command -v timeout >/dev/null 2>&1; then + send_split() { # $1 = request target, $2 = extra header lines before Content-Length + timeout 5 bash -c " + exec 3<>/dev/tcp/127.0.0.1/$PORT + printf 'POST $1 HTTP/1.1\r\nHost: localhost\r\n$2Content-Length: 26\r\n\r\n' >&3 + sleep 0.3 + printf '%s' '$ECHO_BODY' >&3 + cat <&3 + exec 3<&- + " 2>/dev/null || true + } + + # Control first: if the plain split-write case fails, the two decoy cases + # below prove nothing about framing. + GOT=$(send_split "/echo" "" | tr -d '\r' | tail -1) + if [ "$GOT" = "$ECHO_BODY" ]; then + ok "HS29a split-write body arrives intact (control)" + else + fail "HS29a split-write control" "got '$GOT'" + fi + + FRAMED=0 + # Fields are '|'-separated: a ':' delimiter would split the target case, + # whose whole point is that it contains a colon. + for CASE in "value|/echo|X-Note: Content-Length: 0\r\n" \ + "name-suffix|/echo|X-Content-Length: 0\r\n" \ + "target|/echo?q=Content-Length:0|"; do + NAME=${CASE%%|*}; REST=${CASE#*|} + TARGET=${REST%%|*}; EXTRA=${REST#*|} + GOT=$(send_split "$TARGET" "$EXTRA" | tr -d '\r' | tail -1) + if [ "$GOT" != "$ECHO_BODY" ]; then + fail "HS29 decoy Content-Length in $NAME framed the body" "got '$GOT'" + FRAMED=1 + fi + done + [ "$FRAMED" -eq 0 ] && ok "HS29 decoy Content-Length (header value / name suffix / target) does not frame the body" + + # Two Content-Length headers that disagree used to let the first silently + # win. Reject the request instead. + GOT=$(send_split "/echo" "Content-Length: 0\r\n" | head -1) + if echo "$GOT" | grep -q "400"; then + ok "HS30 conflicting duplicate Content-Length rejected with 400" + else + fail "HS30 conflicting duplicate Content-Length" "got '$GOT'" + fi + + # The real header must still be honoured after all that. + RESP=$(curl -s --max-time 2 "http://127.0.0.1:$PORT/ping") + if [ "$RESP" = "pong" ]; then + ok "HS31 server still healthy after framing probes" + else + fail "HS31 server health after framing probes" "got '$RESP'" + fi +else + echo " SKIP: HS29/HS30/HS31 (timeout not available)" +fi + +# ---- http_post strips CR/LF from outbound header names and values (#716) ---- +# hk/hv reached curl's -H verbatim, so a CR/LF in either injected additional +# headers into the outbound request. Loopback: post to /hdrs, which echoes the +# raw header block the server received, and assert the injected name never +# appears at the START of a line. (Grepping for the name alone would false-pass +# — after stripping, the text survives inside X-Test's value, on one line, +# which is exactly the point.) +# Headers go as a flat JSON ARRAY of alternating name/value — http_post only +# reads the VAL_LIST shape, and a JSON object silently delivers no headers +# at all (filed separately). +CLI=$(mktemp /tmp/eigs_http_cli_XXXXXX.eigs) +cat > "$CLI" </dev/null | tr -d '\r') +rm -f "$CLI" +if [ -z "$POSTED" ]; then + fail "HS32 outbound header CRLF injection" "client got no response from /hdrs" +elif echo "$POSTED" | grep -q '^X-Injected:'; then + fail "HS32 outbound header CRLF injection" "injected header reached the wire as its own line" +elif echo "$POSTED" | grep -q '^X-Test: probeX-Injected: yes$'; then + ok "HS32 http_post strips CR/LF from header values (no injected header line)" +else + fail "HS32 outbound header CRLF injection" "X-Test not delivered as expected: $(echo "$POSTED" | grep -i '^X-' | tr '\n' '|')" +fi + # ---- Summary ---- echo "" # ---- Aggregate request-body budget (EIGS_HTTP_MAX_BODY_TOTAL) ----