From 36c97344ef64e4379db3eaea428a4cf7dc71b32d Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 23 Aug 2026 11:49:52 -0700 Subject: [PATCH] s3: confine a Lance catalog table location to the caller's own bucket (#10901) The Lance namespace gateway took the request-body location field, trimmed a trailing slash, and passed it straight to the marker sink. That location feeds TableDataDirFromMetadataLocation, which joins it under /buckets and collapses any ../ segments, and writeMarker's CreateEntry then auto-creates every missing parent. A caller could point the location at another tenant's bucket, or escape /buckets entirely, and plant a fixed-name marker (recursively creating the parents) or hide a victim's live table with .lance-deregistered. Confine the declared location the way the Iceberg gateway already does: require an s3:// URI whose bucket is the caller's own and whose path carries no traversal segment, on both the declare and register handlers. --- weed/s3api/lance/handlers_table.go | 7 ++++ weed/s3api/lance/handlers_test.go | 58 ++++++++++++++++++++++++++++++ weed/s3api/lance/storage.go | 25 +++++++++++++ 3 files changed, 90 insertions(+) diff --git a/weed/s3api/lance/handlers_table.go b/weed/s3api/lance/handlers_table.go index 909d6f05f..40f23a2d8 100644 --- a/weed/s3api/lance/handlers_table.go +++ b/weed/s3api/lance/handlers_table.go @@ -130,6 +130,9 @@ func (s *Server) handleDeclareTable(w http.ResponseWriter, r *http.Request) { location := strings.TrimSuffix(req.Location, "/") if location == "" { location = tableLocation(bucket, ns, name) + } else if err := confineLocation(bucket, location); err != nil { + writeError(w, r, http.StatusBadRequest, codeInvalidInput, err.Error()) + return } if err := s.createTable(r, bucket, ns, name, location); err != nil { @@ -304,6 +307,10 @@ func (s *Server) handleRegisterTable(w http.ResponseWriter, r *http.Request) { writeError(w, r, http.StatusBadRequest, codeInvalidInput, "location is required") return } + if err := confineLocation(bucket, location); err != nil { + writeError(w, r, http.StatusBadRequest, codeInvalidInput, err.Error()) + return + } mode := normalizeMode(req.Mode, modeCreate) if mode != modeCreate && mode != modeOverwrite { diff --git a/weed/s3api/lance/handlers_test.go b/weed/s3api/lance/handlers_test.go index 8402770ce..d62bdc708 100644 --- a/weed/s3api/lance/handlers_test.go +++ b/weed/s3api/lance/handlers_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/gorilla/mux" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" @@ -348,6 +349,63 @@ func TestRouteAndBodyMustAgree(t *testing.T) { } } +// A client-supplied location must resolve inside its own bucket. Without this, +// a "../"-laden or cross-bucket location escapes when the marker path is joined +// under /buckets, letting the request reach another tenant's namespace, or +// outside /buckets entirely, and auto-create the parent directories on the way. +func TestLocationOutsideBucketIsRejected(t *testing.T) { + cases := []struct { + name string + location string + }{ + {"another bucket", "s3://victim/secret"}, + {"traversal into another bucket", "s3://analytics/../victim/secret"}, + {"traversal above the buckets root", "s3://analytics/../../etc/cron.d"}, + {"not an s3 uri", "../../etc/cron.d"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := s3tables.TableDataDirFromMetadataLocation(tc.location) + body := `{"location":"` + tc.location + `"}` + + // declare writes .lance-reserved at the location: rejection must leave + // nothing behind at the escaped directory. + t.Run("declare", func(t *testing.T) { + h := newTestHarness(t) + h.createBucket(t, "analytics", s3tables.FormatLance) + h.mustDo(t, http.MethodPost, "/v1/namespace/analytics$sales/create", `{}`, http.StatusOK) + + recorder := h.mustDo(t, http.MethodPost, "/v1/table/analytics$sales$orders/declare", body, http.StatusBadRequest) + if got := decode[errorResponse](t, recorder); got.Code != codeInvalidInput { + t.Fatalf("error code = %d, want %d", got.Code, codeInvalidInput) + } + if dir != "" && h.filer.Get(dir, reservedMarker) != nil { + t.Fatalf("declare wrote %s into %s despite rejection", reservedMarker, dir) + } + }) + + // register removes .lance-deregistered at the location: rejection must + // leave a victim's marker in place rather than un-hiding their table. + t.Run("register", func(t *testing.T) { + h := newTestHarness(t) + h.createBucket(t, "analytics", s3tables.FormatLance) + h.mustDo(t, http.MethodPost, "/v1/namespace/analytics$sales/create", `{}`, http.StatusOK) + if dir != "" { + h.filer.PutFile(dir, deregisteredMarker, time.Unix(1, 0)) + } + + recorder := h.mustDo(t, http.MethodPost, "/v1/table/analytics$sales$orders/register", body, http.StatusBadRequest) + if got := decode[errorResponse](t, recorder); got.Code != codeInvalidInput { + t.Fatalf("error code = %d, want %d", got.Code, codeInvalidInput) + } + if dir != "" && h.filer.Get(dir, deregisteredMarker) == nil { + t.Fatalf("register removed %s from %s despite rejection", deregisteredMarker, dir) + } + }) + }) + } +} + // The data plane needs Lance format support that does not exist in Go, so it // answers with the spec's own Unsupported code rather than a bare 404. func TestDataPlaneIsUnsupported(t *testing.T) { diff --git a/weed/s3api/lance/storage.go b/weed/s3api/lance/storage.go index b831913a6..e04a1d83d 100644 --- a/weed/s3api/lance/storage.go +++ b/weed/s3api/lance/storage.go @@ -107,6 +107,31 @@ func tableLocation(bucket string, ns []string, name string) string { return fmt.Sprintf("s3://%s/%s/%s", bucket, strings.Join(ns, "."), name) } +// confineLocation rejects a client-supplied table location that does not resolve +// inside the caller's own bucket, the way the Iceberg gateway confines the same +// field. The location feeds TableDataDirFromMetadataLocation, which joins it +// under /buckets and collapses any "../" segments; an unconfined location lets a +// marker write escape into another tenant's namespace. +func confineLocation(bucket, location string) error { + rest, ok := strings.CutPrefix(location, "s3://") + if !ok { + return fmt.Errorf("location must be an s3:// URI") + } + name, keys, _ := strings.Cut(rest, "/") + if name != bucket { + return fmt.Errorf("location must be within bucket %s", bucket) + } + for _, segment := range strings.Split(keys, "/") { + switch { + case segment == "." || segment == "..": + return fmt.Errorf("location must not contain a path traversal segment") + case strings.ContainsAny(segment, "\\\x00"): + return fmt.Errorf("location contains an invalid character") + } + } + return nil +} + // storageOptions builds the object_store settings a Lance client needs to reach // the dataset. The key names are the aws_-prefixed forms Lance clients pass // through to object_store.