common: bound untrusted inputs and address parsing (v26.06.7 security, 4/7)

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;
 }
 

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;

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();
 }

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;
 }

common/test/run-wireaddr.c

@@ -1,5 +1,6 @@
 #include "config.h"
 #include <bitcoin/chainparams.h>
+#include <ccan/array_size/array_size.h>
 #include <common/amount.h>
 #include <common/pseudorand.h>
 #include <common/setup.h>
@@ -263,6 +264,47 @@ 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);
+	}
+
+	/* ...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();
 }

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;
 }
 
@@ -563,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;

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,14 +161,29 @@ 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;
 		connect->buffer[3] = SOCKS_DOMAIN;
 		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,