From 90ef97a37ad088dd1a0265c4676f0ef954ed4ecb Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 7 Nov 2026 16:43:26 +0000 Subject: [PATCH] objstore: refuse a chunk size that wraps the response bound (heap overflow) A chunked HTTP reply could make the object-store reader write past its buffer. os_read_body bounded each chunk with `total + sz > want`, a signed sum. After a 1-byte chunk, the size 7fffffffffffffff wrapped the sum negative, the bound passed, and os_read_exact wrote the rest of the reply past the want-sized buffer. A hostile allow-listed endpoint, or anyone who could change a plain http:// reply, killed the backend with SIGSEGV. os_slurp_body (Iceberg REST client, S3 LIST) had the same sum and raised XX000 from enlargeStringInfo instead of its cap error. Both readers now parse the size with os_chunk_size: the line must start with a hex digit, and strtoull must read a size that fits int64, else "bad chunk size" (08P01). Each bound compares the size with the room left, `sz > want - total` and `sz > cap - s.len`, which cannot overflow. test_objstore_chunked_framing.py drives both readers from its own server on 127.0.0.1: each hostile reply raises its guard's SQLSTATE and the same connection still answers SELECT 1, and a valid chunked reply still reads. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 14 + objstore/columnar_objstore_module.c | 44 +++- test/pytest/TESTS.md | 26 ++ test/pytest/expected_tests.txt | 5 +- test/pytest/test_objstore_chunked_framing.py | 253 +++++++++++++++++++ 5 files changed, 329 insertions(+), 13 deletions(-) create mode 100644 test/pytest/test_objstore_chunked_framing.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 699caf0ef..5719ca814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -534,6 +534,20 @@ true until the next version shipped. ### Fixed +- A remote read no longer writes past its buffer when a server sends a + hostile chunk size. Before this fix, an allow-listed endpoint, or anyone who + could change a plain `http://` reply, could send a 1-byte chunk and then the + chunk size `7fffffffffffffff`. The reader's bound added the two sizes as + signed numbers, the sum wrapped negative, and the reader wrote the rest of + the reply past the end of its buffer. The backend died with a segmentation + fault, and PostgreSQL restarted every session. The Iceberg REST client and S3 + listing had the same sum and raised an internal error instead. The reader now + refuses a chunk size that is not hex or does not fit 64 bits signed, and + compares each size with the room left rather than adding. A ranged read + raises a protocol-violation error (08P01), and a REST reply or listing + raises `exceeded N bytes` (54000). `https://` replies cannot be changed in + transit. `test_objstore_chunked_framing.py`. + - `test/docs_style.sh` no longer prints `command not found` on every run (#2033). Two lines after the block that records the changelog checks repeated the tail of a heredoc, so the shell ran the list of changelog diff --git a/objstore/columnar_objstore_module.c b/objstore/columnar_objstore_module.c index ecf2a5e3a..7b9630068 100644 --- a/objstore/columnar_objstore_module.c +++ b/objstore/columnar_objstore_module.c @@ -1325,6 +1325,32 @@ os_read_line(PgColumnarObjHandle *h, char *buf, size_t cap) } } +/* + * Parse a chunk-size line (RFC 9112 section 7.1): hex digits, then an optional + * chunk extension, which is ignored. The line must start with a hex digit: + * strtoull would otherwise skip blanks and negate a leading minus sign. A size + * that does not fit int64 is refused here; each caller still bounds the size + * by the room it has left, by subtraction, because a sum can wrap. + */ +static int64 +os_chunk_size(PgColumnarObjHandle *h, const char *line) +{ + unsigned long long sz; + char *end; + + errno = 0; + if (line[0] == '\0' || strchr("0123456789abcdefABCDEF", line[0]) == NULL) + ereport(ERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("columnar: bad chunk size from \"%s\"", h->url))); + sz = strtoull(line, &end, 16); + if (end == line || errno == ERANGE || sz > (unsigned long long) PG_INT64_MAX) + ereport(ERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("columnar: bad chunk size from \"%s\"", h->url))); + return (int64) sz; +} + /* * Consume a response body. dst != NULL requires the body to be exactly `want` * bytes; dst == NULL discards a body of any length (error pages, HEAD has @@ -1344,11 +1370,7 @@ os_read_body(PgColumnarObjHandle *h, const OsResponse *resp, int64 sz; os_read_line(h, line, sizeof(line)); - sz = strtoll(line, NULL, 16); - if (sz < 0) - ereport(ERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("columnar: bad chunk size from \"%s\"", h->url))); + sz = os_chunk_size(h, line); if (sz == 0) { /* trailers until the empty line */ @@ -1358,7 +1380,8 @@ os_read_body(PgColumnarObjHandle *h, const OsResponse *resp, } while (line[0] != '\0'); break; } - if (dst != NULL && total + sz > want) + /* total <= want here, so want - total cannot overflow (a sum could) */ + if (dst != NULL && sz > want - total) ereport(ERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), errmsg("columnar: \"%s\" sent more bytes than requested", @@ -3543,11 +3566,7 @@ os_slurp_body(PgColumnarObjHandle *h, const OsResponse *resp, int64 cap, int64 sz; os_read_line(h, line, sizeof(line)); - sz = strtoll(line, NULL, 16); - if (sz < 0) - ereport(ERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("columnar: bad chunk size from \"%s\"", h->url))); + sz = os_chunk_size(h, line); if (sz == 0) { do @@ -3556,7 +3575,8 @@ os_slurp_body(PgColumnarObjHandle *h, const OsResponse *resp, int64 cap, } while (line[0] != '\0'); break; } - if ((int64) s.len + sz > cap) + /* s.len <= cap here, so cap - s.len cannot overflow (a sum could) */ + if (sz > cap - (int64) s.len) ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("columnar: response from \"%s\" exceeded %lld bytes", diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 8943a54f2..4b01267c2 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -401,6 +401,7 @@ behaviour, the source of that number is named. - [353. test_objstore_error_advice.py: an object-store refusal's hint names what to fix, and a refused export keeps its status](#353-test_objstore_error_advicepy-an-object-store-refusals-hint-names-what-to-fix-and-a-refused-export-keeps-its-status) - [354. test_harness_isolation.py: a run is not disturbed by other sessions on a shared machine](#354-test_harness_isolationpy-a-run-is-not-disturbed-by-other-sessions-on-a-shared-machine) - [355. test_native_scale.py: compaction, recluster and reclaim hold at half a million rows](#355-test_native_scalepy-compaction-recluster-and-reclaim-hold-at-half-a-million-rows) +- [356. test_objstore_chunked_framing.py: a chunk size that wraps the response bound is refused, and the backend survives](#356-test_objstore_chunked_framingpy-a-chunk-size-that-wraps-the-response-bound-is-refused-and-the-backend-survives) ## 1. How to read a test in here @@ -14187,3 +14188,28 @@ Each round's `compact_rewrite` must also rewrite at least one group. A group cou | `test_scale_parity_and_reclaim_steady_state` | reclaim round 4: compact_rewrite rewrote at least one group | | `test_scale_parity_and_reclaim_steady_state` | parity after reclaim round 4 | | `test_scale_parity_and_reclaim_steady_state` | reclaim reached steady state (round 4 <= round 2) | + +## 356. test_objstore_chunked_framing.py: a chunk size that wraps the response bound is refused, and the backend survives + +A heap buffer overflow fix, test-first. The object-store reader bounded each chunk of a chunked HTTP reply by adding its size to the bytes read so far. The sum was signed: after a 1-byte chunk, the size `7fffffffffffffff` wrapped it negative, the bound passed, and the reader wrote the rest of the reply past its buffer. On main, the ranged-read check killed the backend (`server closed the connection unexpectedly`), and the REST check got `XX000` instead of the cap's `54000`. + +The file runs its own HTTP server on 127.0.0.1, on a port chosen at run time, and allows it by host and port. A ranged GET through `parquet_schema` reaches `os_read_body`; GET `/v1/config` through `iceberg_rest_table_location` reaches `os_slurp_body`. Each hostile reply must raise its guard's SQLSTATE, and the same connection must then answer `SELECT 1`. The control reads a pyarrow-written file through `read_parquet` and a REST reply, both sent in chunks of 1, 7 and 1000 bytes, and checks every row and the location. + +The second test passed on main too: `strtoll` clamps a size past 64 bits to `7fffffffffffffff`, and with no earlier chunk the sum does not wrap. It pins the new parser's refusal of that size. + +Removing the ranged-read bound fails the first test with the crash. The new unsigned parse with the old signed sums fails the first and third tests. Removing the REST cap fails the third test. + +### Every test + +| test | check | +| --- | --- | +| `test_a_chunk_size_that_wraps_after_a_first_chunk_is_refused_on_a_ranged_read` | a wrapping chunk size on a ranged GET: an ordinary SQL error | +| `test_a_chunk_size_that_wraps_after_a_first_chunk_is_refused_on_a_ranged_read` | a wrapping chunk size on a ranged GET: the same connection answers SELECT 1 after it | +| `test_a_chunk_size_past_64_bits_as_the_first_chunk_is_refused_on_a_ranged_read` | a first chunk size past 64 bits on a ranged GET: an ordinary SQL error | +| `test_a_chunk_size_past_64_bits_as_the_first_chunk_is_refused_on_a_ranged_read` | a first chunk size past 64 bits on a ranged GET: the same connection answers SELECT 1 after it | +| `test_a_chunk_size_that_wraps_the_cap_is_refused_on_a_rest_catalog_reply` | a wrapping chunk size on a REST catalog reply: an ordinary SQL error | +| `test_a_chunk_size_that_wraps_the_cap_is_refused_on_a_rest_catalog_reply` | a wrapping chunk size on a REST catalog reply: the same connection answers SELECT 1 after it | +| `test_a_valid_chunked_reply_of_the_exact_length_still_reads` | a chunked ranged read returns every row | +| `test_a_valid_chunked_reply_of_the_exact_length_still_reads` | a chunked ranged read returns the right values | +| `test_a_valid_chunked_reply_of_the_exact_length_still_reads` | the rows came through ranged GETs, each answered chunked | +| `test_a_valid_chunked_reply_of_the_exact_length_still_reads` | a chunked REST catalog reply is read whole | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index c7da8b177..9d1f0900d 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -1871,4 +1871,7 @@ guard_tests 863 # cluster_tests stays 5347 for #2024's review checks (each reclaim round's compact_rewrite rewrote at least one # group), added to the existing test_scale_parity_and_reclaim_steady_state in test_native_scale.py, by # collection: 6210 tests collected, less guard_tests 863. -cluster_tests 5347 +# cluster_tests 5347 -> 5351 for the four tests in the new test_objstore_chunked_framing.py (a chunk size +# that wraps the response bound is refused on a ranged read and on a REST catalog reply, and the backend +# survives; a valid chunked reply still reads), by collection: 6214 tests collected, less guard_tests 863. +cluster_tests 5351 diff --git a/test/pytest/test_objstore_chunked_framing.py b/test/pytest/test_objstore_chunked_framing.py new file mode 100644 index 000000000..66c5f186e --- /dev/null +++ b/test/pytest/test_objstore_chunked_framing.py @@ -0,0 +1,253 @@ +"""A chunk size that wraps the response bound is refused, and the backend survives. + +An HTTP/1.1 server may send a body in chunks, each led by its size in hex. The +object-store reader adds each size to what it has read so far and refuses a body +that would pass the buffer it is filling. Before this fix the sum was signed +64-bit addition: after a 1-byte chunk, the size 7fffffffffffffff wrapped the sum +negative, the bound passed, and the reader wrote the rest of the body past the +end of the buffer. A hostile server, or anyone who can change a plain http:// +reply, killed the backend with SIGSEGV, which restarts the whole cluster. + +Two readers had the bound: + +- os_read_body, which fills a ranged GET into a fixed buffer + (read_parquet, parquet_schema and every other remote file read); +- os_slurp_body, which reads a whole reply into a growing buffer under a cap + (the Iceberg REST client and S3 LIST). + +Each check below drives one of them from this file's own server on 127.0.0.1, +on a port chosen at run time, allowed by host and port in +pgcolumnar.objstore_allowed_endpoints. The last check is the control: a +legitimate chunked reply of the exact length still reads. +""" +import io +import re +import socket +import threading +import time + +import psycopg +import pytest + +# The hostile chunk size: INT64_MAX. After one byte it wraps a signed sum. +WRAP = b"7fffffffffffffff" +# Bytes sent after the hostile size: what the old code wrote past the buffer. +FLOOD = b"B" * 300_000 + + +def _chunked(parts): + """-> a chunked body carrying `parts` as its chunks, then the last chunk.""" + out = b"".join(b"%x\r\n%s\r\n" % (len(p), p) for p in parts) + return out + b"0\r\n\r\n" + + +class _Server: + """An HTTP server on 127.0.0.1 that serves one object, every reply chunked. + + HEAD answers 200 with the object's length and a strong ETag. A GET with a + Range header answers 206 with the asked-for bytes; a GET without one answers + 200 with the whole object. `frame(data)` turns the bytes to send into the + chunked body, so a check can send a hostile framing for any request. + Every reply carries Connection: close. + """ + + def __init__(self, obj, frame): + self.obj = obj + self.frame = frame + self.requests = [] + self._stop = threading.Event() + self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + self.sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + self.sock.bind(("127.0.0.1", 0)) + self.sock.listen(8) + self.sock.settimeout(0.2) + self.port = self.sock.getsockname()[1] + self.thread = threading.Thread(target=self._run, daemon=True) + self.thread.start() + + def _run(self): + while not self._stop.is_set(): + try: + conn, _ = self.sock.accept() + except socket.timeout: + continue + except OSError: + return + try: + self._serve(conn) + except OSError: + pass # the reader hung up, which a refusal does + finally: + conn.close() + + def _serve(self, conn): + conn.settimeout(5) + head = b"" + while b"\r\n\r\n" not in head: + got = conn.recv(4096) + if not got: + return + head += got + lines = head.split(b"\r\n\r\n", 1)[0].decode("latin-1").split("\r\n") + method, path = lines[0].split()[:2] + headers = {} + for line in lines[1:]: + if ":" in line: + key, value = line.split(":", 1) + headers[key.strip().lower()] = value.strip() + self.requests.append((method, path, headers.get("range"))) + size = len(self.obj) + if method == "HEAD": + conn.sendall(b"HTTP/1.1 200 OK\r\nContent-Length: %d\r\n" + b"ETag: \"e1\"\r\nConnection: close\r\n\r\n" % size) + return + m = re.fullmatch(r"bytes=(\d+)-(\d+)", headers.get("range", "")) + if m: + first, last = int(m.group(1)), min(int(m.group(2)), size - 1) + status = b"206 Partial Content" + extra = b"Content-Range: bytes %d-%d/%d\r\n" % (first, last, size) + data = self.obj[first:last + 1] + else: + status, extra, data = b"200 OK", b"", self.obj + conn.sendall(b"HTTP/1.1 " + status + b"\r\n" + extra + + b"ETag: \"e1\"\r\nTransfer-Encoding: chunked\r\n" + b"Connection: close\r\n\r\n") + conn.sendall(self.frame(data)) + # Let the reader take everything before the socket closes, so a + # close does not reset bytes it has not read yet. + conn.shutdown(socket.SHUT_WR) + try: + while conn.recv(4096): + pass + except OSError: + pass + + def close(self): + self._stop.set() + self.thread.join(timeout=10) + self.sock.close() + + +def _await_up(dsn): + """Wait, at most 60 s, for the server to answer. After a crash the postmaster + restarts every backend, and the next check must not read crash recovery.""" + for _ in range(120): + try: + with psycopg.connect(dsn, autocommit=True, connect_timeout=5) as conn: + conn.execute("SELECT 1") + return + except psycopg.Error: + time.sleep(0.5) + + +def _refused(pgc_cluster, server, sql, sqlstate, expect, what): + """Run `sql` on one connection, assert it raises `sqlstate`, then assert the + same connection still answers SELECT 1: the backend survived.""" + dsn = pgc_cluster.dsn() + _await_up(dsn) + conn = psycopg.connect(dsn, autocommit=True, connect_timeout=10) + try: + conn.execute("SET pgcolumnar.objstore_allowed_endpoints = " + f"'127.0.0.1:{server.port}'") + with pytest.raises(psycopg.Error) as exc: + conn.execute(sql).fetchall() + expect.sqlstate(exc.value, sqlstate, f"{what}: an ordinary SQL error") + try: + alive = conn.execute("SELECT 1").fetchone()[0] + except psycopg.Error: + alive = 0 + expect.num(alive, 1, f"{what}: the same connection answers SELECT 1 after it") + finally: + conn.close() + _await_up(dsn) + + +def _ranged(server): + return (f"SELECT * FROM pgcolumnar.parquet_schema(" + f"'http://127.0.0.1:{server.port}/a.parquet')") + + +def test_a_chunk_size_that_wraps_after_a_first_chunk_is_refused_on_a_ranged_read( + pgc_cluster, expect): + # One byte, then INT64_MAX: total + size wraps a signed sum. + server = _Server(b"\0" * 4096, lambda _data: b"1\r\nA\r\n" + WRAP + b"\r\n" + FLOOD) + try: + _refused(pgc_cluster, server, _ranged(server), "08P01", expect, + "a wrapping chunk size on a ranged GET") + finally: + server.close() + + +def test_a_chunk_size_past_64_bits_as_the_first_chunk_is_refused_on_a_ranged_read( + pgc_cluster, expect): + # No prior chunk, and a size that does not fit 64 bits at all. + server = _Server(b"\0" * 4096, + lambda _data: b"1ffffffffffffffff\r\n" + FLOOD) + try: + _refused(pgc_cluster, server, _ranged(server), "08P01", expect, + "a first chunk size past 64 bits on a ranged GET") + finally: + server.close() + + +def test_a_chunk_size_that_wraps_the_cap_is_refused_on_a_rest_catalog_reply( + pgc_cluster, expect): + # The REST client reads GET /v1/config through os_slurp_body. + server = _Server(b"{}", lambda _data: b"1\r\n{\r\n" + WRAP + b"\r\n" + FLOOD) + sql = (f"SELECT pgcolumnar.iceberg_rest_table_location(" + f"'http://127.0.0.1:{server.port}/cat', 'db', 'events')") + try: + _refused(pgc_cluster, server, sql, "54000", expect, + "a wrapping chunk size on a REST catalog reply") + finally: + server.close() + + +def _pieces(data): + """Split `data` into chunks of 1, 7, 1000, 1000, ... bytes, the last one short.""" + out, at = [], 0 + for size in (1, 7): + out.append(data[at:at + size]) + at += size + while at < len(data): + out.append(data[at:at + 1000]) + at += 1000 + return [p for p in out if p] + + +def test_a_valid_chunked_reply_of_the_exact_length_still_reads(pgc_cluster, expect): + pa = expect.require_module("pyarrow", name="pyarrow writes the control file") + if pa is None: + return + pq = expect.require_module("pyarrow.parquet", name="pyarrow writes the control file") + if pq is None: + return + n = 5000 + buf = io.BytesIO() + pq.write_table(pa.table({"v": pa.array(range(n), pa.int64())}), buf, + row_group_size=1000, compression="NONE", use_dictionary=False) + + dsn = pgc_cluster.dsn() + _await_up(dsn) + server = _Server(buf.getvalue(), lambda data: _chunked(_pieces(data))) + rest = _Server(b'{"metadata-location": "s3://b/t/metadata/1.json"}', + lambda data: _chunked(_pieces(data))) + try: + with psycopg.connect(dsn, autocommit=True, connect_timeout=10) as conn: + conn.execute("SET pgcolumnar.objstore_allowed_endpoints = " + f"'127.0.0.1:{server.port},127.0.0.1:{rest.port}'") + got = conn.execute( + f"SELECT count(*), sum(v) FROM pgcolumnar.read_parquet(" + f"'http://127.0.0.1:{server.port}/a.parquet') AS t(v int8)").fetchone() + loc = conn.execute( + f"SELECT pgcolumnar.iceberg_rest_table_location(" + f"'http://127.0.0.1:{rest.port}/cat', 'db', 'events')").fetchone()[0] + finally: + server.close() + rest.close() + expect.num(got[0], n, "a chunked ranged read returns every row") + expect.num(int(got[1]), n * (n - 1) // 2, "a chunked ranged read returns the right values") + expect.at_least(sum(1 for r in server.requests if r[0] == "GET" and r[2]), 2, + "the rows came through ranged GETs, each answered chunked") + expect.num(1 if loc == "s3://b/t/metadata/1.json" else 0, 1, + "a chunked REST catalog reply is read whole")