diff --git a/weed/server/filer_server_handlers_proxy.go b/weed/server/filer_server_handlers_proxy.go index e06c5f5a9..09e52132d 100644 --- a/weed/server/filer_server_handlers_proxy.go +++ b/weed/server/filer_server_handlers_proxy.go @@ -6,6 +6,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/security" + "github.com/seaweedfs/seaweedfs/weed/storage/needle" util_http "github.com/seaweedfs/seaweedfs/weed/util/http" "github.com/seaweedfs/seaweedfs/weed/util/mem" "github.com/seaweedfs/seaweedfs/weed/util/request_id" @@ -13,6 +14,7 @@ import ( "io" "math/rand/v2" "net/http" + "strings" ) // proxyReadConcurrencyPerVolumeServer limits how many concurrent proxy read @@ -48,8 +50,50 @@ func releaseProxySemaphore(host string) { } } +// baseFileId strips the trailing _N delta suffix that batch assigns append to a +// fid, and only that: the suffix must be a non-empty run of digits, otherwise +// the fid is returned whole for the caller to reject. +// +// The volume server strips at the last "_" unconditionally, which is safe there +// because its fid already came out of a path the mux parsed and so cannot hold +// a "/". Here the value is raw query input, and an unguarded strip would reduce +// "3,01637037d6_1/../../status" to a valid fid and wave the traversal through. +func baseFileId(fileId string) string { + sepIndex := strings.LastIndex(fileId, "_") + if sepIndex <= 0 { + return fileId + } + delta := fileId[sepIndex+1:] + if delta == "" { + return fileId + } + for _, c := range delta { + if c < '0' || c > '9' { + return fileId + } + } + return fileId[:sepIndex] +} + +// validateProxyChunkId rejects a proxyChunkId that is not a well-formed fid. +// LookupFileId only requires a single comma, and the value is pasted into the +// volume server URL path, so "3,x/../../status" resolves to a volume the caller +// never named -- the volume server's mux cleans the dot segments and redirects +// to /status, which the filer follows and relays. +func validateProxyChunkId(fileId string) error { + _, err := needle.ParseFileIdFromString(baseFileId(fileId)) + return err +} + func (fs *FilerServer) proxyToVolumeServer(w http.ResponseWriter, r *http.Request, fileId string) { ctx := r.Context() + + if err := validateProxyChunkId(fileId); err != nil { + glog.V(1).InfofCtx(ctx, "reject proxyChunkId %q: %v", fileId, err) + w.WriteHeader(http.StatusBadRequest) + return + } + urlStrings, err := fs.filer.MasterClient.GetLookupFileIdFunction()(ctx, fileId) if err != nil { glog.ErrorfCtx(ctx, "locate %s: %v", fileId, err) diff --git a/weed/server/filer_server_handlers_proxy_test.go b/weed/server/filer_server_handlers_proxy_test.go index 0bb4bfeb2..8fe654b5a 100644 --- a/weed/server/filer_server_handlers_proxy_test.go +++ b/weed/server/filer_server_handlers_proxy_test.go @@ -2,12 +2,80 @@ package weed_server import ( "context" + "net/http" + "net/http/httptest" "sync" "sync/atomic" "testing" "time" ) +func TestValidateProxyChunkId(t *testing.T) { + for _, tc := range []struct { + fileId string + ok bool + }{ + {"3,01637037d6", true}, + {"1,0c2b3f2f0f", true}, + {"12,04f0e6ba1d", true}, + {"3,01637037d6_1", true}, // batch-assign delta form + {"3,01637037d6_12", true}, // multi-digit delta + {"3,x/../../status", false}, + {"3,01637037d6/../../status", false}, + {"3,01637037d6/../../stats/counter", false}, + {"3,../../status", false}, + {"3,01637037d6/../../status_1", false}, // traversal wearing a delta suffix + // The suffix must be digits only, or stripping it would reduce a + // traversal payload to a valid fid and let it through. + {"3,01637037d6_1/../../status", false}, + {"3,01637037d6_../../status", false}, + {"3,01637037d6_1/../../stats/counter", false}, + {"3,01637037d6_", false}, + {"3,01637037d6_abc", false}, + {"3,01637037d6_1a", false}, + {"3,01637037d6?readDeleted=true", false}, + {"3,01637037d6#frag", false}, + {"3,", false}, + {"3,abc", false}, + {"3", false}, + {"", false}, + } { + err := validateProxyChunkId(tc.fileId) + if tc.ok && err != nil { + t.Errorf("validateProxyChunkId(%q) rejected a valid fid: %v", tc.fileId, err) + } + if !tc.ok && err == nil { + t.Errorf("validateProxyChunkId(%q) accepted a malformed fid", tc.fileId) + } + } +} + +// A fid carrying dot segments must be rejected before the lookup, so it can +// never be pasted into a volume server URL. Asserting on 400 (not merely "no +// traversal") also proves the request never left the filer. +func TestProxyRejectsTraversalBeforeLookup(t *testing.T) { + fs := &FilerServer{} + + for _, fileId := range []string{ + "3,x/../../status", + "3,01637037d6/../../status", + "3,01637037d6/../../stats/counter", + "3,01637037d6_1/../../status", + "3,01637037d6_../../status", + } { + r := httptest.NewRequest(http.MethodGet, "http://filer:8888/?proxyChunkId="+fileId, nil) + w := httptest.NewRecorder() + + // fs.filer is nil: reaching the lookup would panic, so surviving this + // call is itself proof the fid was rejected first. + fs.proxyToVolumeServer(w, r, fileId) + + if w.Code != http.StatusBadRequest { + t.Errorf("proxyChunkId=%q returned %d, want 400", fileId, w.Code) + } + } +} + func TestProxySemaphore_LimitsConcurrency(t *testing.T) { host := "test-volume:8080" defer proxySemaphores.Delete(host)