diff --git a/CHANGELOG.md b/CHANGELOG.md index 3241caf3b..143d3fb71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -580,6 +580,16 @@ true until the next version shipped. ### Fixed +- An Iceberg table on object storage can no longer name a file outside its + location by spelling `..` in a form that only the server reads as `..`. The + remote containment guard already refused a literal `..` and a percent-encoded + one. It now also refuses a segment that starts with `..;`, which servers that + read `;` as the start of path parameters (Tomcat, Spring) serve as `..`; a + segment that starts with `..?` or `..#`, which reaches an `http(s)://` + server as the path `..`, because such a key is sent without percent-encoding; + and it treats a backslash as a separator, as some servers do. Such a path is + refused with `escapes the table location` (22023) before any request is + sent. - An Iceberg REST catalog can no longer make a backend hold an unbounded amount of memory by paging a list. `iceberg_rest_namespaces`, `iceberg_rest_tables` and the namespace walk keep every page of a list in diff --git a/src/columnar_iceberg.c b/src/columnar_iceberg.c index 6800a5975..e1c470103 100644 --- a/src/columnar_iceberg.c +++ b/src/columnar_iceberg.c @@ -609,20 +609,40 @@ ice_path_eq(const char *a, const char *b) * key is literal (no symlinks, no realpath), so a recorded ".." cannot be * collapsed the way canonicalize_path does for a local path; reject it, so a * recorded key cannot reference outside the table prefix on a server that - * normalizes "..". */ + * normalizes "..". + * + * The key is judged as the server behind the URL might read it, not only as + * this reader does. A backslash is a separator to some origins, so it splits + * segments here too. A ';' starts path parameters on others (Tomcat, Spring), + * which serve "..;x" as "..". An http(s):// key is sent verbatim, so a '?' + * starts the query and a '#' the fragment: "..?x" reaches the server as the + * path "..". So ';', '?' and '#' each end a ".." segment. */ +static bool +ice_is_seg_sep(char c) +{ + return c == '/' || c == '\\'; +} + +static bool +ice_ends_dotdot(char c) +{ + return c == '\0' || ice_is_seg_sep(c) || c == ';' || c == '?' || c == '#'; +} + static bool ice_has_dotdot(const char *s) { const char *p = s; - while (p != NULL && *p != '\0') + while (*p != '\0') { if (p[0] == '.' && p[1] == '.' && - (p[2] == '\0' || p[2] == '/') && - (p == s || p[-1] == '/')) + ice_ends_dotdot(p[2]) && + (p == s || ice_is_seg_sep(p[-1]))) return true; - p = strchr(p, '/'); - if (p != NULL) + while (*p != '\0' && !ice_is_seg_sep(*p)) + p++; + if (*p != '\0') p++; } return false; @@ -706,7 +726,7 @@ ice_has_encoded_dotdot(const char *s) /* * An encoded NUL truncates every check below this point. The next pass - * measures with strlen and ice_has_dotdot walks with strchr, so both + * measures with strlen and ice_has_dotdot walks only to the first NUL, so both * stop at that byte and never reach the "../" behind it: a key of * "%00%2e%2e/%2e%2e/etc/hostname" decoded to a NUL followed by the * traversal, and this function returned false. The literal-".." guard diff --git a/test/fixtures/iceberg/gen_delete_fixture.py b/test/fixtures/iceberg/gen_delete_fixture.py index acc6ee1e0..a43db83f2 100755 --- a/test/fixtures/iceberg/gen_delete_fixture.py +++ b/test/fixtures/iceberg/gen_delete_fixture.py @@ -542,6 +542,29 @@ def many_region(i): # "%2e%2e" -> ".." before serving would escape the table location. The remote # guard must decode-and-reject it (harmless on a local root: "%2e%2e" is just # a directory name that does not exist, so realpath fails rather than escapes) +emit_eq_variant("eqsemidot", [(2, EQ_SEQ, + f"{LOC}/..;/..;/..;/..;/etc/hostname", + [1], 1)]) +# the same climb-out spelled "..;": an origin that reads ";" as the start of +# path parameters (Tomcat, Spring) serves "..;x" as "..", so the remote guard +# must read a segment that starts with "..;" as "..". +emit_eq_variant("eqbslashdot", [(2, EQ_SEQ, + f"{LOC}/..\\..\\..\\..\\etc/hostname", + [1], 1)]) +# the same climb-out with a backslash between the segments: an origin that +# reads a backslash as a path separator serves it as "/", so the remote guard +# must read a backslash as a separator. +emit_eq_variant("eqqdot", [(2, EQ_SEQ, + f"{LOC}/..?/..?/..?/..?/etc/hostname", + [1], 1)]) +# the same climb-out with "?" after each "..": an http(s):// key is sent +# verbatim, so the server reads the path as ending at "..", with the rest as +# a query. "?" (and "#") end a ".." segment in the remote guard. +emit_eq_variant("eqpctqdot", [(2, EQ_SEQ, + f"{LOC}/..%3F/..%3F/..%3F/..%3F/etc/hostname", + [1], 1)]) +# the same with the "?" percent-encoded: a server or proxy that decodes it +# first sees "..?"; the guard checks every decoded form. # ---- audit arms (multi-agent adversarial audit of 4b) ---------------------- diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqbslashdot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqbslashdot.avro new file mode 100644 index 000000000..f07e47124 Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqbslashdot.avro differ diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqpctqdot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqpctqdot.avro new file mode 100644 index 000000000..efe95d042 Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqpctqdot.avro differ diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqqdot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqqdot.avro new file mode 100644 index 000000000..88d5cf6ca Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqqdot.avro differ diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqsemidot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqsemidot.avro new file mode 100644 index 000000000..fd1b074c6 Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/delete-manifest-eqsemidot.avro differ diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqbslashdot.metadata.json b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqbslashdot.metadata.json new file mode 100644 index 000000000..f3d00b411 --- /dev/null +++ b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqbslashdot.metadata.json @@ -0,0 +1 @@ +{"format-version": 2, "location": "file:///tmp/pgc_ice_del/db/t", "current-schema-id": 0, "schemas": [{"schema-id": 0, "type": "struct", "fields": [{"id": 1, "name": "id", "required": false, "type": "long"}, {"id": 2, "name": "region", "required": false, "type": "string"}, {"id": 3, "name": "amount", "required": false, "type": "int"}, {"id": 8, "name": "created", "required": false, "type": "timestamp"}, {"id": 9, "name": "extra", "required": false, "type": "long"}]}], "partition-specs": [{"spec-id": 0, "fields": []}, {"spec-id": 1, "fields": [{"name": "region", "transform": "identity", "source-id": 2, "field-id": 1000}]}], "default-spec-id": 0, "current-snapshot-id": 7766554433221100, "snapshots": [{"snapshot-id": 7766554433221100, "sequence-number": 6, "timestamp-ms": 0, "manifest-list": "file:///tmp/pgc_ice_del/db/t/metadata/manifest-list-eqbslashdot.avro", "summary": {"operation": "overwrite"}, "schema-id": 0}]} \ No newline at end of file diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqpctqdot.metadata.json b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqpctqdot.metadata.json new file mode 100644 index 000000000..323ee8f3c --- /dev/null +++ b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqpctqdot.metadata.json @@ -0,0 +1 @@ +{"format-version": 2, "location": "file:///tmp/pgc_ice_del/db/t", "current-schema-id": 0, "schemas": [{"schema-id": 0, "type": "struct", "fields": [{"id": 1, "name": "id", "required": false, "type": "long"}, {"id": 2, "name": "region", "required": false, "type": "string"}, {"id": 3, "name": "amount", "required": false, "type": "int"}, {"id": 8, "name": "created", "required": false, "type": "timestamp"}, {"id": 9, "name": "extra", "required": false, "type": "long"}]}], "partition-specs": [{"spec-id": 0, "fields": []}, {"spec-id": 1, "fields": [{"name": "region", "transform": "identity", "source-id": 2, "field-id": 1000}]}], "default-spec-id": 0, "current-snapshot-id": 7766554433221100, "snapshots": [{"snapshot-id": 7766554433221100, "sequence-number": 6, "timestamp-ms": 0, "manifest-list": "file:///tmp/pgc_ice_del/db/t/metadata/manifest-list-eqpctqdot.avro", "summary": {"operation": "overwrite"}, "schema-id": 0}]} \ No newline at end of file diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqqdot.metadata.json b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqqdot.metadata.json new file mode 100644 index 000000000..85f190f6e --- /dev/null +++ b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqqdot.metadata.json @@ -0,0 +1 @@ +{"format-version": 2, "location": "file:///tmp/pgc_ice_del/db/t", "current-schema-id": 0, "schemas": [{"schema-id": 0, "type": "struct", "fields": [{"id": 1, "name": "id", "required": false, "type": "long"}, {"id": 2, "name": "region", "required": false, "type": "string"}, {"id": 3, "name": "amount", "required": false, "type": "int"}, {"id": 8, "name": "created", "required": false, "type": "timestamp"}, {"id": 9, "name": "extra", "required": false, "type": "long"}]}], "partition-specs": [{"spec-id": 0, "fields": []}, {"spec-id": 1, "fields": [{"name": "region", "transform": "identity", "source-id": 2, "field-id": 1000}]}], "default-spec-id": 0, "current-snapshot-id": 7766554433221100, "snapshots": [{"snapshot-id": 7766554433221100, "sequence-number": 6, "timestamp-ms": 0, "manifest-list": "file:///tmp/pgc_ice_del/db/t/metadata/manifest-list-eqqdot.avro", "summary": {"operation": "overwrite"}, "schema-id": 0}]} \ No newline at end of file diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqsemidot.metadata.json b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqsemidot.metadata.json new file mode 100644 index 000000000..e5a4bb329 --- /dev/null +++ b/test/fixtures/iceberg/warehouse_del/db/t/metadata/eqsemidot.metadata.json @@ -0,0 +1 @@ +{"format-version": 2, "location": "file:///tmp/pgc_ice_del/db/t", "current-schema-id": 0, "schemas": [{"schema-id": 0, "type": "struct", "fields": [{"id": 1, "name": "id", "required": false, "type": "long"}, {"id": 2, "name": "region", "required": false, "type": "string"}, {"id": 3, "name": "amount", "required": false, "type": "int"}, {"id": 8, "name": "created", "required": false, "type": "timestamp"}, {"id": 9, "name": "extra", "required": false, "type": "long"}]}], "partition-specs": [{"spec-id": 0, "fields": []}, {"spec-id": 1, "fields": [{"name": "region", "transform": "identity", "source-id": 2, "field-id": 1000}]}], "default-spec-id": 0, "current-snapshot-id": 7766554433221100, "snapshots": [{"snapshot-id": 7766554433221100, "sequence-number": 6, "timestamp-ms": 0, "manifest-list": "file:///tmp/pgc_ice_del/db/t/metadata/manifest-list-eqsemidot.avro", "summary": {"operation": "overwrite"}, "schema-id": 0}]} \ No newline at end of file diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqbslashdot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqbslashdot.avro new file mode 100644 index 000000000..28e925ca4 Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqbslashdot.avro differ diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqpctqdot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqpctqdot.avro new file mode 100644 index 000000000..9eb6fab3f Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqpctqdot.avro differ diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqqdot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqqdot.avro new file mode 100644 index 000000000..f80e62394 Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqqdot.avro differ diff --git a/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqsemidot.avro b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqsemidot.avro new file mode 100644 index 000000000..6a49b9014 Binary files /dev/null and b/test/fixtures/iceberg/warehouse_del/db/t/metadata/manifest-list-eqsemidot.avro differ diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 9a9791f76..b148518a0 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -10904,6 +10904,15 @@ One check per test, each by the shell's name, every reading a value from a modul | `test_a_dotdot_escaping_delete_path_is_refused_over_s3` | a dotdot-escaping delete path is refused over s3 (22023) | | `test_a_percent_encoded_dotdot_delete_path_is_refused_over_s3` | a percent-encoded dotdot delete path is refused over s3 (22023) | | `test_a_percent_encoded_dotdot_behind_an_encoded_nul_is_refused_over_s3` | a percent-encoded dotdot behind an encoded NUL is refused over s3 (22023) | +| `test_a_dotdot_with_path_parameters_delete_path_is_refused_over_s3` | a delete path climbing out with `..;` segments, which a server that reads `;` as path parameters serves as `..`, is refused by the containment guard (22023) before any fetch | +| `test_a_backslash_separated_dotdot_delete_path_is_refused_over_s3` | a delete path climbing out with backslash-separated `..` segments, which a server that reads a backslash as a separator serves as `../`, is refused by the containment guard (22023) before any fetch | +| `test_a_dotdot_ended_by_a_query_mark_delete_path_is_refused_over_s3` | a delete path climbing out with `..?` segments is refused by the containment guard (22023) before any fetch | +| `test_a_dotdot_ended_by_an_encoded_query_mark_delete_path_is_refused_over_s3` | the same with the `?` encoded as `%3F`, which a decoding server or proxy reads as `?`, is refused by the containment guard (22023) | +| `test_an_iceberg_table_reads_over_plain_http` | the premise of the http arms: the table with position deletes reads over an unsigned plain `http://` server on the same files (3 rows) | +| `test_a_dotdot_with_path_parameters_delete_path_is_refused_over_plain_http` | over plain `http://`, where the key is sent verbatim, a `..;` climb is refused by the containment guard (22023) | +| `test_a_dotdot_backslash_separated_delete_path_is_refused_over_plain_http` | over plain `http://`, a backslash-separated `..` climb is refused by the containment guard (22023) | +| `test_a_dotdot_ended_by_a_query_mark_delete_path_is_refused_over_plain_http` | over plain `http://`, a `..?` climb, which the server would read as the path `..` with a query, is refused by the containment guard (22023) | +| `test_a_dotdot_ended_by_an_encoded_query_mark_delete_path_is_refused_over_plain_http` | over plain `http://`, a `..%3F` climb is refused by the containment guard (22023) | | `test_the_backend_is_still_up_after_the_s3_refusal` | backend still up after the s3 refusal | | `test_an_endpoint_not_on_the_allow_list_is_refused` | an endpoint not on objstore_allowed_endpoints is refused | | `test_the_backend_is_still_up_after_the_endpoint_refusal` | backend still up after the endpoint refusal | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 29feef41d..bb7b3926d 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -1891,4 +1891,12 @@ guard_tests 889 # cluster_tests 5350 -> 5352 for test_iceberg_rest_scan.py's REST list paging cap (security audit, # 2026-11-07): test_paging_pages_within_the_response_cap_together_still_list and # test_paging_pages_over_the_response_cap_together_are_refused, by collection. -cluster_tests 5352 +# cluster_tests 5352 -> 5354 for test_iceberg_objstore.py's remote ".." spellings (security audit, +# 2026-11-07): test_a_dotdot_with_path_parameters_delete_path_is_refused_over_s3 and +# test_a_backslash_separated_dotdot_delete_path_is_refused_over_s3, by collection. +# cluster_tests 5354 -> 5361 for the review of #2049 in test_iceberg_objstore.py: "..?" and "..%3F" over +# s3 (test_a_dotdot_ended_by_a_query_mark_delete_path_is_refused_over_s3, +# test_a_dotdot_ended_by_an_encoded_query_mark_delete_path_is_refused_over_s3), the plain http:// leg's +# control (test_an_iceberg_table_reads_over_plain_http) and the four spellings refused over plain http +# (test_a_dotdot_*_delete_path_is_refused_over_plain_http), by collection. +cluster_tests 5361 diff --git a/test/pytest/test_iceberg_objstore.py b/test/pytest/test_iceberg_objstore.py index c45e1666d..f9a254b17 100644 --- a/test/pytest/test_iceberg_objstore.py +++ b/test/pytest/test_iceberg_objstore.py @@ -153,6 +153,18 @@ def iob(pgc_cluster): if out.is_file() and "READY" in out.read_text(errors="replace"): break time.sleep(0.1) + # The same files over plain http://, unsigned. An http(s) key is sent verbatim, + # not percent-encoded as an s3:// key is, so this leg reaches the server with + # the recorded bytes. + http_port = pick_port(28) + hout = work / "http_server.out" + hsrv = subprocess.Popen([sys.executable, str(TEST / "objstore_http_server.py"), "--dir", str(work), + "--port", str(http_port), "--log", str(work / "http.log")], + stdout=open(hout, "w"), stderr=subprocess.STDOUT) + for _ in range(50): + if hout.is_file() and "READY" in hout.read_text(errors="replace"): + break + time.sleep(0.1) datadir = work / "data" datadir.mkdir() @@ -195,6 +207,19 @@ def iob(pgc_cluster): for key, meta in (("escape", "eqescape"), ("dotdot", "eqdotdot"), ("pctdot", "eqpctdot"), ("nulpct", "eqnulpct")): r[key] = _sqlstate(dsn, f"SELECT * FROM {scan.format(MDROOT, meta)}") + # Spellings of ".." that an origin, not this reader, turns into a climb: + # "..;" (path parameters) and a backslash separator. Refused by the + # containment guard, before any fetch, as the forms above are. + for key, meta in (("semidot", "eqsemidot"), ("bslashdot", "eqbslashdot"), + ("qdot", "eqqdot"), ("pctqdot", "eqpctqdot")): + r[key] = _refusal(dsn, f"SELECT * FROM {scan.format(MDROOT, meta)}", + "escapes the table location", " by the containment guard") + http_root = f"http://127.0.0.1:{http_port}/{BUCKET}/db/t/metadata" + r["http apply"] = _value(dsn, f"SELECT count(*) FROM {scan.format(http_root, 'apply')}") + for key, meta in (("semidot", "eqsemidot"), ("bslashdot", "eqbslashdot"), + ("qdot", "eqqdot"), ("pctqdot", "eqpctqdot")): + r[f"http {key}"] = _refusal(dsn, f"SELECT * FROM {scan.format(http_root, meta)}", + "escapes the table location", " by the containment guard") r["after refusal"] = _survived(dsn, log, before) _restart(cluster, datadir, log, dict(aws, AWS_ENDPOINT_URL=f"http://127.0.0.2:{s3_port}")) @@ -206,6 +231,8 @@ def iob(pgc_cluster): _pg_ctl(cluster, ["-D", str(datadir), "-m", "immediate", "-w", "stop"], {}) srv.kill() srv.wait() + hsrv.kill() + hsrv.wait() shutil.rmtree(work, ignore_errors=True) @@ -329,6 +356,74 @@ def test_a_percent_encoded_dotdot_behind_an_encoded_nul_is_refused_over_s3(iob, "a percent-encoded dotdot behind an encoded NUL is refused over s3 (22023)") +def test_a_dotdot_with_path_parameters_delete_path_is_refused_over_s3(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["semidot"], "22023 by the containment guard", + "a delete path climbing out with \"..;\" segments is refused over s3 by the containment guard") + + +def test_a_backslash_separated_dotdot_delete_path_is_refused_over_s3(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["bslashdot"], "22023 by the containment guard", + "a delete path climbing out with backslash-separated \"..\" segments is refused over s3 " + "by the containment guard") + + +def test_a_dotdot_ended_by_a_query_mark_delete_path_is_refused_over_s3(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["qdot"], "22023 by the containment guard", + "a delete path climbing out with \"..?\" segments is refused over s3 by the containment guard") + + +def test_a_dotdot_ended_by_an_encoded_query_mark_delete_path_is_refused_over_s3(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["pctqdot"], "22023 by the containment guard", + "a delete path climbing out with \"..%3F\" segments is refused over s3 by the containment guard") + + +def test_an_iceberg_table_reads_over_plain_http(iob, expect): + """The premise of the http(s) arms below: this leg reaches the reader at all.""" + if _declined(iob, expect): + return + expect.text(iob["http apply"], "3", "an Iceberg table with position deletes reads over plain http (3 rows)") + + +def test_a_dotdot_with_path_parameters_delete_path_is_refused_over_plain_http(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["http semidot"], "22023 by the containment guard", + "a delete path climbing out with \"..;\" segments is refused over plain http, where the key " + "is sent verbatim, by the containment guard") + + +def test_a_dotdot_backslash_separated_delete_path_is_refused_over_plain_http(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["http bslashdot"], "22023 by the containment guard", + "a delete path climbing out with backslash-separated \"..\" segments is refused over plain http, where the key " + "is sent verbatim, by the containment guard") + + +def test_a_dotdot_ended_by_a_query_mark_delete_path_is_refused_over_plain_http(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["http qdot"], "22023 by the containment guard", + "a delete path climbing out with \"..?\" segments is refused over plain http, where the key " + "is sent verbatim, by the containment guard") + + +def test_a_dotdot_ended_by_an_encoded_query_mark_delete_path_is_refused_over_plain_http(iob, expect): + if _declined(iob, expect): + return + expect.text(iob["http pctqdot"], "22023 by the containment guard", + "a delete path climbing out with \"..%3F\" segments is refused over plain http, where the key " + "is sent verbatim, by the containment guard") + + def test_the_backend_is_still_up_after_the_s3_refusal(iob, expect): if _declined(iob, expect): return