From 98b95b2fc016ae5c82dd717772eb1888a309e603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Tue, 11 Aug 2026 18:41:28 -0300 Subject: [PATCH 1/8] pytest: check the SOCKS5 request for a maximum-length hostname connectd assembles the SOCKS5 CONNECT request in a fixed buffer, but never checks that the hostname fits it. Add a test that a maximum-length proxied hostname produces exactly the request it should, with connectd still alive afterwards. Changelog-None (cherry picked from commit 6ecead49248408172489e3b77c34b44994b25414) --- tests/test_connection.py | 84 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/tests/test_connection.py b/tests/test_connection.py index 4b07bda708c9..2e7b44e8a347 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -25,8 +25,10 @@ import unittest import websocket import signal +import socket import ssl import sys +import threading def test_connect_basic(node_factory): @@ -5136,3 +5138,85 @@ def test_open_channel_funding_above_max_supply(node_factory, bitcoind): funding_sat, push_msat) assert l1.rpc.getinfo()['id'] == l1.info['id'] + + +@pytest.mark.xfail(strict=True) +def test_connect_proxy_maxlen_hostname(node_factory): + """A maximum-length hostname must produce a well-formed SOCKS5 request. + + The request is assembled in a fixed buffer, and the hostname was copied + into it without checking that it fit, which overran the buffer and + corrupted the adjacent length field. That length was then used for the + write, so the proxy got a wildly oversized read of connectd's memory + instead of the request (and connectd died on the way). + + A gossiped DNS address reaches the same builder, so this covers that + path too. + """ + # A minimal SOCKS5 server: accept the "no authentication" greeting, + # then collect whatever request connectd sends us. + received = [] + listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + listener.bind(('127.0.0.1', 0)) + listener.listen(1) + proxyport = listener.getsockname()[1] + + def serve(): + data = b'' + conn, _ = listener.accept() + with conn: + try: + conn.settimeout(TIMEOUT) + conn.recv(len(b'\x05\x01\x00')) + conn.sendall(b'\x05\x00') + # Deliberately ask for far more than the largest legal + # request (262 bytes), so an oversized write is visible + # here rather than silently truncated by us. We stop on the + # timeout, once connectd has finished writing and is waiting + # for a reply we're never going to send. + conn.settimeout(2) + while len(data) < 65536: + more = conn.recv(65536) + if not more: + break + data += more + except OSError: + pass + received.append(data) + + server = threading.Thread(target=serve, daemon=True) + server.start() + + l1 = node_factory.get_node(options={'proxy': '127.0.0.1:{}'.format(proxyport), + 'always-use-proxy': 'true'}, + # Without the fix connectd dies here, and we + # want that to be a test failure rather than + # a teardown error. + may_fail=True, broken_log='.*') + + # unresolved.name[256] is what limits us: 255 is the longest we can ask for. + hostname = 'a' * (255 - len('.example.com')) + '.example.com' + assert len(hostname) == 255 + + # There's nothing on the far side of the proxy, so this fails: it just + # must not take connectd down with it. + # Any valid pubkey will do: we never get far enough to talk to it. + nodeid = '0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798' + with pytest.raises(RpcError, match="All addresses failed"): + l1.rpc.connect(nodeid, hostname, 1234) + + server.join(TIMEOUT) + listener.close() + + # connectd is still there, and so is the node. + assert not l1.daemon.is_in_log('FATAL SIGNAL') + l1.rpc.getinfo() + + # And the proxy got exactly the request it should have: version, CONNECT, + # reserved, "domain name", length, the name itself, then the port. + request = only_one(received) + assert request == (b'\x05\x01\x00\x03' + + bytes([len(hostname)]) + + hostname.encode('ascii') + + (1234).to_bytes(2, 'big')) From 19339dec3dc205d26940f59925d8d85e4b01246b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Tue, 11 Aug 2026 18:41:49 -0300 Subject: [PATCH 2/8] connectd: bound the SOCKS5 request hostname to the request buffer The SOCKS5 CONNECT request is built into a fixed 255-byte buffer, but a domain-name request needs 7 + strlen(host) bytes and the host length was never checked. A host longer than 248 bytes overran the buffer while the request was assembled, corrupting the adjacent length field, which was then used as the io_write() length and produced a large out-of-bounds read: the proxy socket received connectd's heap instead of the request, and connectd died. The host reaches this both from a proxied connect and from a gossiped DNS address. Size the buffer to hold the largest legal request (header plus a maximum-length domain name), and refuse a hostname which still does not fit rather than building a request we cannot represent. Truncating the host is not an option: that is a connection to the wrong destination. Changelog-Fixed: connectd: a proxied connection to a very long hostname (including a gossiped DNS address) could crash the node. (cherry picked from commit 04ca809936794de47943951b9d116d9fd1ea1844) --- connectd/tor.c | 26 ++++++++++++++++++++++++-- tests/test_connection.py | 1 - 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/connectd/tor.c b/connectd/tor.c index 95ba253abc42..83c25df7c6fe 100644 --- a/connectd/tor.c +++ b/connectd/tor.c @@ -17,7 +17,6 @@ #define SOCKS_TYP_IPV6 4 #define SOCKS_V5 5 -#define MAX_SIZE_OF_SOCKS5_REQ_OR_RESP 255 #define SIZE_OF_RESPONSE 4 #define SIZE_OF_REQUEST 3 #define SIZE_OF_IPV4_RESPONSE 6 @@ -26,6 +25,14 @@ #define SOCK_REQ_V5_LEN 5 #define SOCK_REQ_V5_HEADER_LEN 7 +/* The domain name in a SOCKS5 request is preceded by a single length + * byte, so it can never be longer than this. */ +#define MAX_SIZE_OF_SOCKS5_DOMAIN 255 +/* The largest thing we ever put in the buffer is a domain-name CONNECT + * request: the header plus a maximum-length domain name. */ +#define MAX_SIZE_OF_SOCKS5_REQ_OR_RESP (SOCK_REQ_V5_HEADER_LEN \ + + MAX_SIZE_OF_SOCKS5_DOMAIN) + /* some crufts can not forward ipv6 */ #undef BIND_FIRST_TO_IPV6 @@ -154,6 +161,21 @@ static struct io_plan *io_tor_connect_after_resp_to_connect(struct io_conn if (connect->buffer[1] == '\0') { /* make the V5 request */ connect->hlen = strlen(connect->host); + + /* The length is carried in a single byte, and the whole + * request has to fit in our buffer: refuse rather than + * build a request we can't represent. */ + if (connect->hlen > MAX_SIZE_OF_SOCKS5_DOMAIN) { + const char *msg = tal_fmt(tmpctx, + "Connected out for %s error: hostname too long for socks5 request", + connect->host); + status_debug("%s", msg); + add_errors_to_error_list(connect->connect, msg); + + errno = ECONNREFUSED; + return io_close(conn); + } + connect->buffer[0] = SOCKS_V5; connect->buffer[1] = SOCKS_CONNECT; connect->buffer[2] = 0; @@ -161,7 +183,7 @@ static struct io_plan *io_tor_connect_after_resp_to_connect(struct io_conn connect->buffer[4] = connect->hlen; memcpy(connect->buffer + SOCK_REQ_V5_LEN, connect->host, connect->hlen); - memcpy(connect->buffer + SOCK_REQ_V5_LEN + strlen(connect->host), + memcpy(connect->buffer + SOCK_REQ_V5_LEN + connect->hlen, &(connect->port), sizeof connect->port); status_io(LOG_IO_OUT, NULL, "proxy", connect->buffer, diff --git a/tests/test_connection.py b/tests/test_connection.py index 2e7b44e8a347..0e672477c94d 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -5140,7 +5140,6 @@ def test_open_channel_funding_above_max_supply(node_factory, bitcoind): assert l1.rpc.getinfo()['id'] == l1.info['id'] -@pytest.mark.xfail(strict=True) def test_connect_proxy_maxlen_hostname(node_factory): """A maximum-length hostname must produce a well-formed SOCKS5 request. From ed77b9e9af07e7396ba1e59e91c218a67df47a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Tue, 11 Aug 2026 18:42:30 -0300 Subject: [PATCH 3/8] common: ignore DNS address descriptors which are not hostnames fromwire_wireaddr() accepted any bytes at all as an ADDR_TYPE_DNS address, so a node_announcement could put arbitrary content into log messages, listnodes output and outgoing connect attempts. is_dnsaddr() already encodes the rules; apply it, and ignore the descriptor when it fails, the same way we already ignore a descriptor with a zero port. Changelog-Changed: gossipd: DNS addresses in a node_announcement which are not valid hostnames are now ignored. (cherry picked from commit 9574eaa55d46f791e9bdf1108219f760d412dc9e) --- common/test/run-wireaddr.c | 26 ++++++++++++++++++++++++++ common/wireaddr.c | 16 ++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/common/test/run-wireaddr.c b/common/test/run-wireaddr.c index 5c10d191b25d..7c41457f42f0 100644 --- a/common/test/run-wireaddr.c +++ b/common/test/run-wireaddr.c @@ -1,5 +1,6 @@ #include "config.h" #include +#include #include #include #include @@ -263,6 +264,31 @@ int main(int argc, char *argv[]) assert(fromwire_wireaddr((const u8 **) &encoded_wa, &encoded_wa_len, &decoded_wa) == FROMWIREADDR_OK); assert(wireaddr_eq(&wa, &decoded_wa)); + /* A DNS descriptor which isn't actually a hostname is ignored, rather + * than handed on to the rest of the daemon. */ + const char *baddnsaddrs[] = { "", + "not a hostname", + "invalid..example.com", + "-.invalid.com" }; + + for (size_t i = 0; i < ARRAY_SIZE(baddnsaddrs); i++) { + struct wireaddr bad_wa = { + .type = ADDR_TYPE_DNS, + .addrlen = strlen(baddnsaddrs[i]), + .port = DEFAULT_PORT + }; + memcpy(bad_wa.addr, baddnsaddrs[i], bad_wa.addrlen); + + u8 *encoded_bad = tal_arr(tmpctx, u8, 0); + towire_wireaddr(&encoded_bad, &bad_wa); + size_t encoded_bad_len = tal_bytelen(encoded_bad); + + struct wireaddr decoded_bad; + assert(fromwire_wireaddr((const u8 **) &encoded_bad, + &encoded_bad_len, + &decoded_bad) == FROMWIREADDR_IGNORE); + } + tal_free(expect); common_shutdown(); } diff --git a/common/wireaddr.c b/common/wireaddr.c index 80932566c6d5..7f218d399b45 100644 --- a/common/wireaddr.c +++ b/common/wireaddr.c @@ -65,6 +65,22 @@ enum fromwireaddr_ret fromwire_wireaddr(const u8 **cursor, size_t *max, struct w /* FIXME: This seems universal? */ if (addr->port == 0) return FROMWIREADDR_IGNORE; + + /* BOLT #7: + * * `5`: DNS hostname; data = `[1:hostname_len][hostname_len:hostname][2:port]` (length up to 258) + * * `hostname` bytes MUST be ASCII characters. + */ + /* Don't let a malformed name loose in the rest of the daemon: it + * ends up in log messages, in connect requests, and in whatever a + * plugin does with it. */ + if (addr->type == ADDR_TYPE_DNS) { + char hostname[DNS_ADDRLEN + 1]; + + memcpy(hostname, addr->addr, addr->addrlen); + hostname[addr->addrlen] = '\0'; + if (!is_dnsaddr(hostname)) + return FROMWIREADDR_IGNORE; + } return FROMWIREADDR_OK; } From e25575d06e28134ca443a4e575f23f1df077e654 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Fri, 21 Aug 2026 14:45:20 -0300 Subject: [PATCH 4/8] common: reject dns: addresses which are not hostnames fromwire_wireaddr() now ignores a DNS descriptor which isn't a hostname, but parse_wireaddr() still accepts one: it only checks the length. So an announce-addr which fails is_dnsaddr() is accepted at startup, gossiped, and then dropped by every peer which receives it, with nothing in our own logs to say the address is useless. Apply the same rule where the operator can see the answer. A name which passes is_dnsaddr() is unaffected; a name which fails it was already being ignored by everyone. This also restores the towire/fromwire round trip which tests/fuzz/fuzz-wireaddr asserts, and which the fromwire-side check alone broke: Assertion failed: (fromwire_wireaddr((const u8 **)&output_buffer, &len, &decoded_wa) == FROMWIREADDR_OK), function run, file fuzz-wireaddr.c, line 37. Changelog-Changed: config: `dns:` addresses which are not valid hostnames are now rejected at startup instead of being silently ignored by peers. (cherry picked from commit 0613903e013c8d1f2e634b63af332cbd24804f55) --- common/test/run-wireaddr.c | 16 ++++++++++++++++ common/wireaddr.c | 6 ++++++ 2 files changed, 22 insertions(+) diff --git a/common/test/run-wireaddr.c b/common/test/run-wireaddr.c index 7c41457f42f0..c94d451c4e49 100644 --- a/common/test/run-wireaddr.c +++ b/common/test/run-wireaddr.c @@ -289,6 +289,22 @@ int main(int argc, char *argv[]) &decoded_bad) == FROMWIREADDR_IGNORE); } + /* ...and we don't produce one either: a dns: address which isn't a + * hostname is rejected at parse time, rather than accepted here and + * silently dropped by every peer we announce it to. */ + for (size_t i = 0; i < ARRAY_SIZE(baddnsaddrs); i++) { + struct wireaddr parsed; + const char *arg = tal_fmt(tmpctx, "dns:%s", baddnsaddrs[i]); + assert(parse_wireaddr(tmpctx, arg, DEFAULT_PORT, NULL, + &parsed) != NULL); + } + + /* A real hostname still works. */ + struct wireaddr good_dns; + assert(parse_wireaddr(tmpctx, "dns:example.com", DEFAULT_PORT, NULL, + &good_dns) == NULL); + assert(good_dns.type == ADDR_TYPE_DNS); + tal_free(expect); common_shutdown(); } diff --git a/common/wireaddr.c b/common/wireaddr.c index 7f218d399b45..1364e4931383 100644 --- a/common/wireaddr.c +++ b/common/wireaddr.c @@ -579,6 +579,12 @@ const char *parse_wireaddr(const tal_t *ctx, if (strlen(ip) > DNS_ADDRLEN) return "DNS address too long"; + /* fromwire_wireaddr() ignores a DNS descriptor which isn't a + * hostname, so anything we announce that fails this is dropped + * by every peer. Say so here, where the operator can see it. */ + if (!is_dnsaddr(ip)) + return tal_fmt(ctx, "dns: '%s' is not a hostname", ip); + addr->addrlen = strlen(ip); memcpy(addr->addr, ip, addr->addrlen); addr->port = port; From 9a744dd95794cd91d2c53febd4b913c3262dc7a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Thu, 20 Aug 2026 18:47:53 -0300 Subject: [PATCH 5/8] common: bound JSON nesting depth when parsing json_next() and the JSMN result validator each recurse once per level of nesting, so a sufficiently deeply nested (but otherwise valid) JSON document could exhaust the C stack. Bound the nesting depth in json_parse_input(), before either recursive walk runs, and reject anything deeper. The depth check is itself iterative, so it cannot overflow. Changelog-Fixed: JSON-RPC: reject excessively-nested JSON rather than risk a stack overflow. (cherry picked from commit b940aa32b7b385b4139bbc9251b1cca044c496cc) --- common/json_parse_simple.c | 48 ++++++++++++++++++++++++++++++++++++-- common/test/run-json.c | 44 ++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/common/json_parse_simple.c b/common/json_parse_simple.c index 7e0e299c9dfa..356942fe4c42 100644 --- a/common/json_parse_simple.c +++ b/common/json_parse_simple.c @@ -179,6 +179,47 @@ const jsmntok_t *json_next(const jsmntok_t *tok) return t; } +/* We refuse JSON nested deeper than this. jsmn tokenizes iteratively, but + * json_next() and the validators below recurse once per nesting level, so an + * unbounded depth overflows the C stack. Real JSON-RPC and BOLT payloads nest + * only a handful of levels; this sits far above them and far below the stack + * limit. Enforced in json_parse_input(), so every token array handed to + * json_next() and friends elsewhere has already been bounded. */ +#define JSON_MAX_NESTING 256 + +/* Iteratively count the tokens in the first datum of toks[], rejecting + * anything nested deeper than JSON_MAX_NESTING. On success sets *len to the + * token count (as json_next(toks) - toks would) and returns true; returns + * false without recursing on over-nested, attacker-controlled input. */ +static bool bounded_datum_len(const jsmntok_t *toks, size_t *len) +{ + /* remaining[d] = child datums still to visit at nesting level d; + * level 0 holds the single root datum. */ + size_t remaining[JSON_MAX_NESTING + 1]; + size_t depth = 0, i = 0; + + remaining[0] = 1; + for (;;) { + /* Ascend out of every level we have finished. */ + while (remaining[depth] == 0) { + if (depth == 0) { + *len = i; + return true; + } + depth--; + } + remaining[depth]--; + + /* Descend into this token's children, if it has any. */ + if (toks[i].size != 0) { + if (depth == JSON_MAX_NESTING) + return false; + remaining[++depth] = toks[i].size; + } + i++; + } +} + const jsmntok_t *json_get_membern(const char *buffer, const jsmntok_t tok[], const char *label, size_t len) @@ -492,8 +533,11 @@ bool json_parse_input(jsmn_parser *parser, /* If we read a partial element at the end of the stream we'll get a * errro, but due to the previous check we know we read at * least one full element, so count tokens that are part of this root - * element. */ - ret = json_next(*toks) - *toks; + * element. Bound the nesting depth here, before any recursive walk. */ + size_t datumlen; + if (!bounded_datum_len(*toks, &datumlen)) + return false; + ret = datumlen; if (!validate_jsmn_parse_output(input, *toks, *toks + ret)) return false; diff --git a/common/test/run-json.c b/common/test/run-json.c index 1fc42a2223ae..c0fcaeaaa92a 100644 --- a/common/test/run-json.c +++ b/common/test/run-json.c @@ -226,6 +226,49 @@ static void test_json_bad_utf8(void) assert(json_parse_simple(tmpctx, buf, strlen(buf))); } +static void test_json_deep_nesting(void) +{ + char *buf; + size_t d, i; + + /* Arrays nested exactly at the limit still parse. */ + d = JSON_MAX_NESTING; + buf = tal_arr(tmpctx, char, 2 * d + 2); + memset(buf, '[', d); + buf[d] = '0'; + memset(buf + d + 1, ']', d); + buf[2 * d + 1] = '\0'; + assert(json_parse_simple(tmpctx, buf, 2 * d + 1)); + + /* One level deeper is rejected, not crashed. */ + d = JSON_MAX_NESTING + 1; + buf = tal_arr(tmpctx, char, 2 * d + 2); + memset(buf, '[', d); + buf[d] = '0'; + memset(buf + d + 1, ']', d); + buf[2 * d + 1] = '\0'; + assert(!json_parse_simple(tmpctx, buf, 2 * d + 1)); + + /* A pathologically deep array is rejected iteratively, without + * overflowing the stack. */ + d = 100000; + buf = tal_arr(tmpctx, char, 2 * d + 2); + memset(buf, '[', d); + buf[d] = '0'; + memset(buf + d + 1, ']', d); + buf[2 * d + 1] = '\0'; + assert(!json_parse_simple(tmpctx, buf, 2 * d + 1)); + + /* Same for deeply nested objects. */ + buf = tal_strdup(tmpctx, ""); + for (i = 0; i < 100000; i++) + tal_append_fmt(&buf, "{\"a\":"); + tal_append_fmt(&buf, "1"); + for (i = 0; i < 100000; i++) + tal_append_fmt(&buf, "}"); + assert(!json_parse_simple(tmpctx, buf, strlen(buf))); +} + int main(int argc, char *argv[]) { common_setup(argv[0]); @@ -234,6 +277,7 @@ int main(int argc, char *argv[]) test_json_tok_bitcoin_amount(); test_json_tok_millionths(); test_json_bad_utf8(); + test_json_deep_nesting(); common_shutdown(); } From 1630f6219df4ca39f8ad96376e637c11ab2bf72e Mon Sep 17 00:00:00 2001 From: daywalker90 Date: Wed, 12 Aug 2026 14:05:26 +0200 Subject: [PATCH 6/8] clnrest: restrict maximum size of request bodies to 2MiB Changelog-Fixed: clnrest: an unauthenticated user could crash the node with a large request body; bodies are now capped at 2MiB. (cherry picked from commit d5c2f6dbf1b7a54679b97d2a71015ad45443baf8) --- Cargo.lock | 1 + plugins/rest-plugin/Cargo.toml | 1 + plugins/rest-plugin/src/handlers.rs | 32 +++++++++++++++++++++++++++-- plugins/rest-plugin/src/structs.rs | 3 +++ 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8a2a792c6fd0..0820e33bd3ab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -577,6 +577,7 @@ dependencies = [ "cln-plugin", "cln-rpc", "futures-util", + "http-body-util", "hyper 1.11.0", "log", "log-panics", diff --git a/plugins/rest-plugin/Cargo.toml b/plugins/rest-plugin/Cargo.toml index 5db65b12e9ce..572e84e311ef 100644 --- a/plugins/rest-plugin/Cargo.toml +++ b/plugins/rest-plugin/Cargo.toml @@ -41,6 +41,7 @@ futures-util = { version = "0.3", default-features = false, features = [ ] } rcgen = "0.14" hyper = "1" +http-body-util = "0.1" tower = "0.5" tower-http = { version = "0.7", features = ["cors", "set-header"] } utoipa = { version = "5", features = ['axum_extras'] } diff --git a/plugins/rest-plugin/src/handlers.rs b/plugins/rest-plugin/src/handlers.rs index 77bb6375fbee..9a59853ce4d2 100644 --- a/plugins/rest-plugin/src/handlers.rs +++ b/plugins/rest-plugin/src/handlers.rs @@ -23,6 +23,10 @@ use crate::{ structs::{AppError, CheckRuneParams, ClnrestMap, PluginState}, }; +/// Maximum size (in bytes) of an accepted request body. +/// Oversized bodies are rejected with 413. +pub const MAX_BODY_SIZE: usize = 2 * 1024 * 1024; // 2 MiB + /* Handler for list-methods */ #[utoipa::path( get, @@ -115,11 +119,20 @@ pub async fn call_rpc_method( .and_then(|v| v.to_str().ok()) .map(String::from); - let request_bytes = match to_bytes(body.into_body(), usize::MAX).await { + let request_bytes = match to_bytes(body.into_body(), MAX_BODY_SIZE).await { Ok(o) => o, Err(e) => { + if is_body_too_large(&e) { + return Err(AppError::PayloadTooLarge(RpcError { + code: Some(-32600), + data: None, + message: format!( + "Request body exceeds the maximum allowed size of {MAX_BODY_SIZE} bytes" + ), + })); + } return Err(AppError::InternalServerError(RpcError { - code: None, + code: Some(-32700), data: None, message: format!("Could not read request body: {}", e), })); @@ -160,6 +173,21 @@ pub async fn call_rpc_method( convert_json_to_response(headers, &rest_map.rpc_method, cln_result) } +fn is_body_too_large(e: &axum::Error) -> bool { + fn inner(e: &(dyn std::error::Error + 'static)) -> bool { + if e.downcast_ref::() + .is_some() + { + return true; + } + match e.source() { + Some(src) => inner(src), + None => false, + } + } + inner(e) +} + fn fill_rune_restrictions( rest_map: &mut ClnrestMap, rpc_params: &serde_json::Map, diff --git a/plugins/rest-plugin/src/structs.rs b/plugins/rest-plugin/src/structs.rs index 0cc0ce103e69..b77666f60154 100644 --- a/plugins/rest-plugin/src/structs.rs +++ b/plugins/rest-plugin/src/structs.rs @@ -31,6 +31,7 @@ pub enum AppError { MethodNotAllowed(RpcError), InternalServerError(RpcError), NotAcceptable(RpcError), + PayloadTooLarge(RpcError), } impl IntoResponse for AppError { @@ -42,6 +43,7 @@ impl IntoResponse for AppError { AppError::MethodNotAllowed(err) => (StatusCode::METHOD_NOT_ALLOWED, err), AppError::InternalServerError(err) => (StatusCode::INTERNAL_SERVER_ERROR, err), AppError::NotAcceptable(err) => (StatusCode::NOT_ACCEPTABLE, err), + AppError::PayloadTooLarge(err) => (StatusCode::PAYLOAD_TOO_LARGE, err), }; let body = Json(json!(error_message)); @@ -58,6 +60,7 @@ impl std::fmt::Display for AppError { AppError::MethodNotAllowed(err) => write!(f, "Method not allowed: {err}"), AppError::InternalServerError(err) => write!(f, "Internal Server Error: {err}"), AppError::NotAcceptable(err) => write!(f, "Not Acceptable: {err}"), + AppError::PayloadTooLarge(err) => write!(f, "Payload Too Large: {err}"), } } } From 966d828e71e0fb0416411bbbf937568b459fce4b Mon Sep 17 00:00:00 2001 From: daywalker90 Date: Wed, 12 Aug 2026 14:22:55 +0200 Subject: [PATCH 7/8] tests: add clnrest test for large request bodies Changelog-None (cherry picked from commit 23e98db66b6f8fd8538d78363d0be39429a6b2b8) --- tests/test_clnrest.py | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/test_clnrest.py b/tests/test_clnrest.py index 479ca25a7938..bcc9a668e122 100644 --- a/tests/test_clnrest.py +++ b/tests/test_clnrest.py @@ -27,6 +27,7 @@ def http_session_with_retry(): retry = Retry(connect=10, backoff_factor=0.5) adapter = HTTPAdapter(max_retries=retry) http_session.mount('https://', adapter) + http_session.mount('http://', adapter) return http_session @@ -131,7 +132,7 @@ def test_generate_certificate(node_factory): assert [c[0] != c[1] for c in zip(contents, contents_2)] == [True] * len(files) -def start_node_with_clnrest(node_factory, plugin=None): +def start_node_with_clnrest(node_factory, plugin=None, protocol='https'): """Start a node with the clnrest plugin, whose options are the default options. Return: - the node, @@ -139,11 +140,12 @@ def start_node_with_clnrest(node_factory, plugin=None): - the certificate authority path used for the self-signed certificates.""" rest_port = str(node_factory.get_unused_port()) rest_certs = node_factory.directory + '/clnrest-certs' - options = {'clnrest-port': rest_port, 'clnrest-certs': rest_certs} + options = {'clnrest-port': rest_port, 'clnrest-certs': rest_certs, + 'clnrest-protocol': protocol} if plugin is not None: options['plugin'] = plugin l1 = node_factory.get_node(options=options) - base_url = 'https://127.0.0.1:' + rest_port + base_url = f'{protocol}://127.0.0.1:' + rest_port # This might happen really early! l1.daemon.logsearch_start = 0 l1.daemon.wait_for_log(r'plugin-clnrest: REST server running at ' + base_url) @@ -882,3 +884,22 @@ def test_dynamic_path_rune(node_factory): dynamic_res.raise_for_status() dynamic_json = dynamic_res.json() assert dynamic_json["test-dynamic-clnrest"] == "success" + + +def test_large_request_body(node_factory): + """Test large request bodies getting rejected without a crash. + + Run over plain HTTP: with TLS, a client that sends the whole body before + reading the response can only see a connection reset instead of the 413, + depending on the OpenSSL version. + """ + l1, base_url, _ = start_node_with_clnrest(node_factory, protocol='http') + http_session = http_session_with_retry() + + body = b'{"pad":"' + b"B" * (32 * 1024 * 1024) + b'"}' + response = http_session.post(base_url + "/v1/getinfo", data=body) + assert response.status_code == 413 + assert response.json()["code"] == -32600 + assert "Request body exceeds the maximum allowed size" in response.json()["message"] + + l1.rpc.getinfo() From 4e3dce8688c39aee56be9c88852789272448949c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Mon, 24 Aug 2026 15:41:17 -0300 Subject: [PATCH 8/8] common: fix tlv_span returning huge length for out-of-range stream tlv_span tracked its range with two pointers and returned end - start. end was assigned only after the type > maxfield test, so a stream whose first in-range field also exceeds maxfield left end NULL while start was set, and the guard that copies start over only fired when start was NULL. The returned end - start was then a pointer difference against NULL, close to the full width of size_t, which callers hand to sha256_update as a length: an out-of-bounds read walking mapped memory until it faults. Track the span as offsets rather than pointers, and return an empty span (at the range's position) whenever no field falls inside the range or the end precedes the start, which is the correct measure for such a stream. Changelog-Fixed: A crafted BOLT12 message could cause an out-of-bounds read while computing its identifier. Co-authored-by: Claude (cherry picked from commit a0b451761edf2d0148b6787f0d2f07aea44a2fff) --- common/bolt12.c | 36 ++++++++++++++++++++++++------- common/test/run-tlv_span.c | 43 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/common/bolt12.c b/common/bolt12.c index 3b80f0578376..ace3ff853595 100644 --- a/common/bolt12.c +++ b/common/bolt12.c @@ -666,31 +666,51 @@ bool bolt12_has_prefix(const char *str) bolt12_has_request_prefix(str); } -/* Inclusive span of tlv range >= minfield and <= maxfield */ +/* Byte length of the run of TLV records whose type is in [minfield, maxfield], + * storing its offset into tlvstream in *startp. TLV records are ordered by + * type, so the wanted records are contiguous: we track two byte offsets into + * the stream, `start` (the first record at or above minfield) and `end` (just + * past the last record at or below maxfield), and return end - start. + * + * Offsets, not pointers: a pointer past the end of a range that was never + * entered would underflow to a near-SIZE_MAX length, which callers hash. */ size_t tlv_span(const u8 *tlvstream, u64 minfield, u64 maxfield, size_t *startp) { const u8 *cursor = tlvstream; size_t tlvlen = tal_bytelen(tlvstream); - const u8 *start, *end; + size_t start, end; + /* Distinguishes "start is offset 0" from "no in-range field seen". */ + bool have_start = false; - start = end = NULL; + start = end = 0; while (tlvlen) { - const u8 *before = cursor; + size_t before = cursor - tlvstream; bigsize_t type = fromwire_bigsize(&cursor, &tlvlen); bigsize_t len = fromwire_bigsize(&cursor, &tlvlen); - if (type >= minfield && start == NULL) + /* Truncated header: stop, keeping the span so far. */ + if (!cursor) + break; + if (type >= minfield && !have_start) { start = before; + have_start = true; + } + /* Past the range: later records are all higher, so we're done. */ if (type > maxfield) break; fromwire_pad(&cursor, &tlvlen, len); - end = cursor; + /* Truncated value: this record does not count. */ + if (!cursor) + break; + end = cursor - tlvstream; } - if (!start) + /* No in-range field, or the range opened only after the last record we + * accepted: collapse to an empty span at end rather than a bogus length. */ + if (!have_start || end < start) start = end; if (startp) - *startp = start - tlvstream; + *startp = start; return end - start; } diff --git a/common/test/run-tlv_span.c b/common/test/run-tlv_span.c index 8d5e22d8314f..67b9d57f11b8 100644 --- a/common/test/run-tlv_span.c +++ b/common/test/run-tlv_span.c @@ -115,10 +115,17 @@ void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNE { fprintf(stderr, "towire_u8_array called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ +/* bolt12 id-hash field ranges: invoice_request hashes 0..159, offer 1..79. */ +#define INVREQ_MIN 0 +#define INVREQ_MAX 159 +#define OFFER_MIN 1 +#define OFFER_MAX 79 + int main(int argc, char *argv[]) { u8 *wire; size_t len, start; + const char *hex; common_setup(argv[0]); @@ -135,6 +142,42 @@ int main(int argc, char *argv[]) len = tlv_span(wire, 0, 1, &start); assert(start == 0); assert(len == strlen("0010b8538094dbd70d8a0f0439d8e64f766f") / 2); + + /* Stream whose first field is already above maxfield: empty span. */ + hex = "f0020102"; + wire = tal_hexdata(tmpctx, hex, strlen(hex)); + len = tlv_span(wire, INVREQ_MIN, INVREQ_MAX, &start); + assert(start <= tal_bytelen(wire)); + assert(len == 0); + + len = tlv_span(wire, OFFER_MIN, OFFER_MAX, &start); + assert(start <= tal_bytelen(wire)); + assert(len == 0); + + /* Same, but with a field below the range in front of it. */ + hex = "0002010af0020102"; + wire = tal_hexdata(tmpctx, hex, strlen(hex)); + len = tlv_span(wire, OFFER_MIN, OFFER_MAX, &start); + assert(start <= tal_bytelen(wire)); + assert(len == 0); + + len = tlv_span(wire, INVREQ_MIN, INVREQ_MAX, &start); + assert(start == 0); + assert(len == strlen("0002010a") / 2); + + /* Truncated stream. */ + hex = "f0"; + wire = tal_hexdata(tmpctx, hex, strlen(hex)); + len = tlv_span(wire, INVREQ_MIN, INVREQ_MAX, &start); + assert(start <= tal_bytelen(wire)); + assert(len == 0); + + hex = "0002"; + wire = tal_hexdata(tmpctx, hex, strlen(hex)); + len = tlv_span(wire, INVREQ_MIN, INVREQ_MAX, &start); + assert(start <= tal_bytelen(wire)); + assert(len == 0); + common_shutdown(); return 0; }