diff --git a/internal/config/config.go b/internal/config/config.go index cbb4e6e1..14c4c207 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1068,6 +1068,12 @@ func parseAuthURL(value string) (*url.URL, error) { } func authURLMatches(configured, target *url.URL) bool { + // An upstream resolves dot segments, so /scope/../other would leave the + // configured path while still matching it by prefix. Real package URLs + // never contain them, so they get no credentials at all. + if HasDotSegment(target.Path) { + return false + } if !strings.EqualFold(configured.Scheme, target.Scheme) || !strings.EqualFold(configured.Hostname(), target.Hostname()) || authURLPort(configured) != authURLPort(target) { @@ -1085,6 +1091,25 @@ func authURLMatches(configured, target *url.URL) bool { return targetPath == configuredPath || strings.HasPrefix(targetPath, configuredPath+"/") } +// HasDotSegment reports whether the URL path p, or p decoded once more, has a +// '.' or '..' segment. Backslashes count as separators, and ";params" are +// ignored, since some upstream servers (Tomcat, Jetty) treat them that way. +func HasDotSegment(p string) bool { + candidates := []string{p} + if decoded, err := url.PathUnescape(p); err == nil { + candidates = append(candidates, decoded) + } + for _, candidate := range candidates { + for segment := range strings.SplitSeq(strings.ReplaceAll(candidate, `\`, "/"), "/") { + segment, _, _ = strings.Cut(segment, ";") + if segment == "." || segment == ".." { + return true + } + } + } + return false +} + func authURLPort(value *url.URL) string { if port := value.Port(); port != "" { return port diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 224c6517..7a689f85 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1322,6 +1322,13 @@ func TestUpstreamAuthForURLMatchesURLComponents(t *testing.T) { {name: "lookalike host rejected", url: "https://registry.example.com.evil.test/package"}, {name: "different scheme rejected", url: "http://registry.example.com/package"}, {name: "different port rejected", url: "https://registry.example.com:8443/package"}, + {name: "dot dot segment rejected", url: "https://registry.example.com/private/../package"}, + {name: "encoded dot dot segment rejected", url: "https://registry.example.com/private/%2e%2e/package"}, + {name: "double encoded dot dot segment rejected", url: "https://registry.example.com/private/%252e%252e/package"}, + {name: "dot segment rejected", url: "https://registry.example.com/private/./package"}, + {name: "dot dot segment with parameter rejected", url: "https://registry.example.com/private/..;/package"}, + {name: "encoded dot dot segment with parameter rejected", url: "https://registry.example.com/private/%2e%2e;x/package"}, + {name: "dot dot segment with encoded parameter rejected", url: "https://registry.example.com/private/..%3b/package"}, } for _, tt := range tests { diff --git a/internal/server/dot_segments_test.go b/internal/server/dot_segments_test.go new file mode 100644 index 00000000..429ccc09 --- /dev/null +++ b/internal/server/dot_segments_test.go @@ -0,0 +1,97 @@ +package server + +import ( + "context" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/git-pkgs/proxy/internal/config" + "github.com/git-pkgs/proxy/internal/database" + "github.com/git-pkgs/proxy/internal/handler" + "github.com/git-pkgs/proxy/internal/storage" + "github.com/git-pkgs/registries/fetch" + "github.com/go-chi/chi/v5" +) + +// TestProtocolHandlersRejectDotSegments sends paths with encoded '.' and '..' +// segments, and with ';' path parameters, to every mounted ecosystem. Several +// handlers append the decoded path to the upstream URL, where a '..' leaves +// the configured upstream path, and Tomcat and Jetty drop ";params" first. +func TestProtocolHandlersRejectDotSegments(t *testing.T) { + db, err := database.Create(t.TempDir() + "/test.db") + if err != nil { + t.Fatalf("creating database: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + store, err := storage.OpenBucket(context.Background(), "file://"+t.TempDir()) + if err != nil { + t.Fatalf("opening storage: %v", err) + } + + transport := &countingTransport{} + client := &http.Client{Transport: transport} + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(client), fetch.WithMaxRetries(0)) + t.Cleanup(func() { _ = fetcher.Close() }) + proxy := handler.NewProxy(db, store, fetcher, fetch.NewResolver(), slog.New(slog.NewTextHandler(io.Discard, nil))) + proxy.HTTPClient = client + + cfg := config.Default() + cfg.BaseURL = "http://proxy.example" + s := &Server{cfg: cfg} + r := chi.NewRouter() + s.mountProtocolHandlers(r, proxy) + + for _, target := range []string{ + "/maven/%2e%2e/%2e%2e/secret", + "/maven/com/%2e%2e/%2e%2e/x/maven-metadata.xml", + "/maven/com/%2E%2E/x/1.0/x-1.0.jar", + "/maven/com/%2e/x/1.0/x-1.0.jar", + "/maven/com/%252e%252e/x/maven-metadata.xml", + "/conda/c/%2e%2e/x.json", + "/cran/src/contrib/%2e%2e%2fx.tar.gz", + "/cran/src/contrib/..%5cx.tar.gz", + "/go/a/%2e%2e/%2e%2e/x/@v/list", + "/gem/info/%2e%2e%2f%2e%2e%2fx", + "/gem/gems/%2e%2e%2fx-1.0.gem", + "/nuget/v3-flatcontainer/%2e%2e/x/index.json", + "/debian/pool/%2e%2e/x.deb", + "/rpm/%2e%2e/x.rpm", + "/maven/..;/..;/other-repo/com/x/x/1.0/x-1.0.jar", + "/maven/com/%2e%2e;/x/1.0/x-1.0.jar", + "/maven/com/..%3b/x/1.0/x-1.0.jar", + "/maven/com/evil/evil/1.0;x/evil-1.0.jar", + "/generic/github/o/r/releases/download/v1/tool.tar.gz;x", + } { + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + if w.Code != http.StatusBadRequest { + t.Errorf("%s: status = %d, want 400", target, w.Code) + } + } + if n := transport.hits.Load(); n != 0 { + t.Errorf("upstream was contacted %d times", n) + } +} + +// TestDotSegmentGuardKeepsDottedNames keeps names that only contain dots, +// such as versions and file extensions, working. +func TestDotSegmentGuardKeepsDottedNames(t *testing.T) { + for _, target := range []string{ + "/maven/com/example/x/1.0/x-1.0.jar", + "/pypi/simple/zope.interface/", + "/npm/.bin", + "/generic/github/o/r/releases/download/v1/...tar.gz", + "/maven/com/example/x/1..0/x-1..0.jar", + } { + var reached bool + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true }) + w := httptest.NewRecorder() + rejectDotSegments(next).ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + if !reached { + t.Errorf("%s: status = %d, want the request passed on", target, w.Code) + } + } +} diff --git a/internal/server/middleware.go b/internal/server/middleware.go index 0de0dba5..c179744f 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -8,6 +8,7 @@ import ( "time" "github.com/git-pkgs/proxy/internal/accesslog" + "github.com/git-pkgs/proxy/internal/config" "github.com/git-pkgs/proxy/internal/metrics" "github.com/go-chi/chi/v5/middleware" ) @@ -124,15 +125,33 @@ func requestEcosystem(path string) string { } } -// rejectQueryInPath refuses requests whose decoded path contains '?' or '#'. -// Handlers build upstream URLs from the decoded path, where such a character -// would end the path: busybox.apk%3F/foo-9.9-r0.apk fetches busybox but is -// parsed and cached as foo, under the wrong PURL and past the denylist. -// Composer archives are exempt: their upstream URL comes from the package -// metadata, and git branch versions such as dev-fix/#42 may contain '#'. +// rejectQueryInPath refuses requests whose decoded path contains '?', '#' +// or ';'. Handlers build upstream URLs from the decoded path, where '?' or +// '#' would end the path: busybox.apk%3F/foo-9.9-r0.apk fetches busybox but +// is parsed and cached as foo, under the wrong PURL and past the denylist. +// Tomcat and Jetty drop ";params" from a path segment, so 1.0;x/evil-1.0.jar +// would be served as version 1.0 while the proxy checks 1.0;x. Composer +// archives are exempt: their upstream URL comes from the package metadata, +// and git branch versions such as dev-fix/#42 may contain '#' or ';'. func rejectQueryInPath(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if strings.ContainsAny(r.URL.Path, "?#") && !strings.HasPrefix(r.URL.Path, "/composer/files/") { + if strings.ContainsAny(r.URL.Path, "?#;") && !strings.HasPrefix(r.URL.Path, "/composer/files/") { + http.Error(w, "invalid path", http.StatusBadRequest) + return + } + next.ServeHTTP(w, r) + }) +} + +// rejectDotSegments refuses requests whose decoded path has a '.' or '..' +// segment. Routers clean literal dot segments, but encoded ones such as +// %2e%2e reach the handlers, and several of them append the path to the +// upstream URL, where the upstream resolves it outside the configured base +// path. Backslashes count as separators, and the path is decoded once more +// so double-encoded segments are refused too. +func rejectDotSegments(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if config.HasDotSegment(r.URL.Path) { http.Error(w, "invalid path", http.StatusBadRequest) return } diff --git a/internal/server/query_in_path_test.go b/internal/server/query_in_path_test.go index 7c70a13c..44ab58ed 100644 --- a/internal/server/query_in_path_test.go +++ b/internal/server/query_in_path_test.go @@ -93,15 +93,21 @@ func TestProtocolHandlersRejectQueryInPath(t *testing.T) { } // TestComposerFilesAllowHashInVersion keeps Composer archive downloads of git -// branches with '#' in their name working: the version is escaped in the dist -// URL and the archive is resolved from metadata, not from the request path. +// branches with '#' or ';' in their name working: the version is escaped in +// the dist URL and the archive is resolved from metadata, not from the +// request path. func TestComposerFilesAllowHashInVersion(t *testing.T) { - var reached atomic.Bool - next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached.Store(true) }) + for _, target := range []string{ + "/composer/files/acme/lib/dev-fix%2F%2342/abc123.zip", + "/composer/files/acme/lib/dev-fix%3B42/abc123.zip", + } { + var reached atomic.Bool + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached.Store(true) }) - w := httptest.NewRecorder() - rejectQueryInPath(next).ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/composer/files/acme/lib/dev-fix%2F%2342/abc123.zip", nil)) - if w.Code == http.StatusBadRequest || !reached.Load() { - t.Errorf("status = %d, reached = %v; want the request passed on", w.Code, reached.Load()) + w := httptest.NewRecorder() + rejectQueryInPath(next).ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + if w.Code == http.StatusBadRequest || !reached.Load() { + t.Errorf("%s: status = %d, reached = %v; want the request passed on", target, w.Code, reached.Load()) + } } } diff --git a/internal/server/server.go b/internal/server/server.go index 8a0a683a..9a4c5062 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -493,7 +493,7 @@ func (s *Server) mountProtocolHandlers(r chi.Router, proxy *handler.Proxy) { rpmHandler := handler.NewRPMHandlerWithUpstream(proxy, s.cfg.BaseURL, s.cfg.Upstream.RPM) genericHandler := handler.NewGenericHandler(proxy, s.cfg.Upstream.Generic, s.cfg.Upstream.GenericArtifacts) - r = r.With(sandboxUpstreamContent, rejectQueryInPath) + r = r.With(sandboxUpstreamContent, rejectQueryInPath, rejectDotSegments) r.Mount("/npm", http.StripPrefix("/npm", npmHandler.Routes())) r.Mount("/cargo", http.StripPrefix("/cargo", cargoHandler.Routes())) r.Mount("/gem", http.StripPrefix("/gem", gemHandler.Routes()))