diff options
Diffstat (limited to 'workhorse')
-rw-r--r-- | workhorse/CHANGELOG | 12 | ||||
-rw-r--r-- | workhorse/VERSION | 2 | ||||
-rw-r--r-- | workhorse/internal/staticpages/deploy_page_test.go | 4 | ||||
-rw-r--r-- | workhorse/internal/staticpages/error_pages_test.go | 14 | ||||
-rw-r--r-- | workhorse/internal/staticpages/servefile.go | 47 | ||||
-rw-r--r-- | workhorse/internal/staticpages/servefile_test.go | 46 | ||||
-rw-r--r-- | workhorse/internal/staticpages/static.go | 1 | ||||
-rw-r--r-- | workhorse/internal/staticpages/testdata/file1 | 1 | ||||
-rw-r--r-- | workhorse/internal/staticpages/testdata/uploads/file2 | 1 | ||||
-rw-r--r-- | workhorse/internal/upstream/routes.go | 20 | ||||
-rw-r--r-- | workhorse/internal/upstream/upstream.go | 8 | ||||
-rw-r--r-- | workhorse/internal/upstream/upstream_test.go | 67 | ||||
-rw-r--r-- | workhorse/main_test.go | 11 |
13 files changed, 189 insertions, 45 deletions
diff --git a/workhorse/CHANGELOG b/workhorse/CHANGELOG index 42c13465ba9..13a9afc1482 100644 --- a/workhorse/CHANGELOG +++ b/workhorse/CHANGELOG @@ -1,5 +1,17 @@ # Changelog for gitlab-workhorse +## v8.59.2 + +### Security +- Stop logging when path is excluded + https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/ + +## v8.59.1 + +### Security +- Use URL.EscapePath() in upstream router + https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/ + ## v8.59.0 ### Fixed diff --git a/workhorse/VERSION b/workhorse/VERSION index 8c20f999400..d94761355d4 100644 --- a/workhorse/VERSION +++ b/workhorse/VERSION @@ -1 +1 @@ -8.59.0 +8.59.2 diff --git a/workhorse/internal/staticpages/deploy_page_test.go b/workhorse/internal/staticpages/deploy_page_test.go index 4b081e73a97..9bc0a082144 100644 --- a/workhorse/internal/staticpages/deploy_page_test.go +++ b/workhorse/internal/staticpages/deploy_page_test.go @@ -23,7 +23,7 @@ func TestIfNoDeployPageExist(t *testing.T) { w := httptest.NewRecorder() executed := false - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { executed = true })).ServeHTTP(w, nil) @@ -45,7 +45,7 @@ func TestIfDeployPageExist(t *testing.T) { w := httptest.NewRecorder() executed := false - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { executed = true })).ServeHTTP(w, nil) diff --git a/workhorse/internal/staticpages/error_pages_test.go b/workhorse/internal/staticpages/error_pages_test.go index 05ec06cd429..ab29e187c8e 100644 --- a/workhorse/internal/staticpages/error_pages_test.go +++ b/workhorse/internal/staticpages/error_pages_test.go @@ -32,7 +32,7 @@ func TestIfErrorPageIsPresented(t *testing.T) { require.NoError(t, err) require.Equal(t, len(upstreamBody), n, "bytes written") }) - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) w.Flush() @@ -54,7 +54,7 @@ func TestIfErrorPassedIfNoErrorPageIsFound(t *testing.T) { w.WriteHeader(404) fmt.Fprint(w, errorResponse) }) - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) w.Flush() @@ -78,7 +78,7 @@ func TestIfErrorPageIsIgnoredInDevelopment(t *testing.T) { w.WriteHeader(500) fmt.Fprint(w, serverError) }) - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ErrorPagesUnless(true, ErrorFormatHTML, h).ServeHTTP(w, nil) w.Flush() require.Equal(t, 500, w.Code) @@ -102,7 +102,7 @@ func TestIfErrorPageIsIgnoredIfCustomError(t *testing.T) { w.WriteHeader(500) fmt.Fprint(w, serverError) }) - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) w.Flush() require.Equal(t, 500, w.Code) @@ -137,7 +137,7 @@ func TestErrorPageInterceptedByContentType(t *testing.T) { w.WriteHeader(500) fmt.Fprint(w, serverError) }) - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) w.Flush() require.Equal(t, 500, w.Code) @@ -161,7 +161,7 @@ func TestIfErrorPageIsPresentedJSON(t *testing.T) { require.NoError(t, err) require.Equal(t, len(upstreamBody), n, "bytes written") }) - st := &Static{""} + st := &Static{} st.ErrorPagesUnless(false, ErrorFormatJSON, h).ServeHTTP(w, nil) w.Flush() @@ -181,7 +181,7 @@ func TestIfErrorPageIsPresentedText(t *testing.T) { require.NoError(t, err) require.Equal(t, len(upstreamBody), n, "bytes written") }) - st := &Static{""} + st := &Static{} st.ErrorPagesUnless(false, ErrorFormatText, h).ServeHTTP(w, nil) w.Flush() diff --git a/workhorse/internal/staticpages/servefile.go b/workhorse/internal/staticpages/servefile.go index c98bc030bc2..7e39f919fa6 100644 --- a/workhorse/internal/staticpages/servefile.go +++ b/workhorse/internal/staticpages/servefile.go @@ -1,16 +1,18 @@ package staticpages import ( + "errors" + "fmt" "net/http" "os" "path/filepath" "strings" "time" - "gitlab.com/gitlab-org/labkit/log" "gitlab.com/gitlab-org/labkit/mask" "gitlab.com/gitlab-org/gitlab-workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/log" "gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix" ) @@ -26,21 +28,28 @@ const ( // upstream. func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoundHandler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - file := filepath.Join(s.DocumentRoot, prefix.Strip(r.URL.Path)) + if notFoundHandler == nil { + notFoundHandler = http.HandlerFunc(http.NotFound) + } + + // We intentionally use r.URL.Path instead of r.URL.EscaptedPath() below. + // This is to make it possible to serve static files with e.g. a space + // %20 in their name. + relativePath, err := s.validatePath(prefix.Strip(r.URL.Path)) + if err != nil { + notFoundHandler.ServeHTTP(w, r) + return + } - // The filepath.Join does Clean traversing directories up + file := filepath.Join(s.DocumentRoot, relativePath) if !strings.HasPrefix(file, s.DocumentRoot) { - helper.Fail500(w, r, &os.PathError{ - Op: "open", - Path: file, - Err: os.ErrInvalid, - }) + log.WithRequest(r).WithError(errPathTraversal).Error() + notFoundHandler.ServeHTTP(w, r) return } var content *os.File var fi os.FileInfo - var err error // Serve pre-gzipped assets if acceptEncoding := r.Header.Get("Accept-Encoding"); strings.Contains(acceptEncoding, "gzip") { @@ -55,11 +64,7 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun content, fi, err = helper.OpenFile(file) } if err != nil { - if notFoundHandler != nil { - notFoundHandler.ServeHTTP(w, r) - } else { - http.NotFound(w, r) - } + notFoundHandler.ServeHTTP(w, r) return } defer content.Close() @@ -82,3 +87,17 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun http.ServeContent(w, r, filepath.Base(file), fi.ModTime(), content) }) } + +var errPathTraversal = errors.New("path traversal") + +func (s *Static) validatePath(filename string) (string, error) { + filename = filepath.Clean(filename) + + for _, exc := range s.Exclude { + if strings.HasPrefix(filename, exc) { + return "", fmt.Errorf("file is excluded: %s", exc) + } + } + + return filename, nil +} diff --git a/workhorse/internal/staticpages/servefile_test.go b/workhorse/internal/staticpages/servefile_test.go index e136b876298..314547b8a57 100644 --- a/workhorse/internal/staticpages/servefile_test.go +++ b/workhorse/internal/staticpages/servefile_test.go @@ -20,7 +20,7 @@ func TestServingNonExistingFile(t *testing.T) { httpRequest, _ := http.NewRequest("GET", "/file", nil) w := httptest.NewRecorder() - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) require.Equal(t, 404, w.Code) } @@ -34,7 +34,7 @@ func TestServingDirectory(t *testing.T) { httpRequest, _ := http.NewRequest("GET", "/file", nil) w := httptest.NewRecorder() - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) require.Equal(t, 404, w.Code) } @@ -44,7 +44,7 @@ func TestServingMalformedUri(t *testing.T) { httpRequest, _ := http.NewRequest("GET", "/../../../static/file", nil) w := httptest.NewRecorder() - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) require.Equal(t, 404, w.Code) } @@ -54,7 +54,7 @@ func TestExecutingHandlerWhenNoFileFound(t *testing.T) { httpRequest, _ := http.NewRequest("GET", "/file", nil) executed := false - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ServeExisting("/", CacheDisabled, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { executed = (r == httpRequest) })).ServeHTTP(nil, httpRequest) @@ -76,7 +76,7 @@ func TestServingTheActualFile(t *testing.T) { ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600) w := httptest.NewRecorder() - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) require.Equal(t, 200, w.Code) if w.Body.String() != fileContent { @@ -84,6 +84,40 @@ func TestServingTheActualFile(t *testing.T) { } } +func TestExcludedPaths(t *testing.T) { + testCases := []struct { + desc string + path string + found bool + contents string + }{ + {"allowed file", "/file1", true, "contents1"}, + {"path traversal is allowed", "/uploads/../file1", true, "contents1"}, + {"files in /uploads/ are invisible", "/uploads/file2", false, ""}, + {"cannot use path traversal to get to /uploads/", "/foobar/../uploads/file2", false, ""}, + {"cannot use escaped path traversal to get to /uploads/", "/foobar%2f%2e%2e%2fuploads/file2", false, ""}, + {"cannot use double escaped path traversal to get to /uploads/", "/foobar%252f%252e%252e%252fuploads/file2", false, ""}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + httpRequest, err := http.NewRequest("GET", tc.path, nil) + require.NoError(t, err) + + w := httptest.NewRecorder() + st := &Static{DocumentRoot: "testdata", Exclude: []string{"/uploads/"}} + st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) + + if tc.found { + require.Equal(t, 200, w.Code) + require.Equal(t, tc.contents, w.Body.String()) + } else { + require.Equal(t, 404, w.Code) + } + }) + } +} + func testServingThePregzippedFile(t *testing.T, enableGzip bool) { dir, err := ioutil.TempDir("", "deploy") if err != nil { @@ -108,7 +142,7 @@ func testServingThePregzippedFile(t *testing.T, enableGzip bool) { ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600) w := httptest.NewRecorder() - st := &Static{dir} + st := &Static{DocumentRoot: dir} st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) require.Equal(t, 200, w.Code) if enableGzip { diff --git a/workhorse/internal/staticpages/static.go b/workhorse/internal/staticpages/static.go index b42351f15f5..5b804e4d644 100644 --- a/workhorse/internal/staticpages/static.go +++ b/workhorse/internal/staticpages/static.go @@ -2,4 +2,5 @@ package staticpages type Static struct { DocumentRoot string + Exclude []string } diff --git a/workhorse/internal/staticpages/testdata/file1 b/workhorse/internal/staticpages/testdata/file1 new file mode 100644 index 00000000000..146edcbe0a3 --- /dev/null +++ b/workhorse/internal/staticpages/testdata/file1 @@ -0,0 +1 @@ +contents1
\ No newline at end of file diff --git a/workhorse/internal/staticpages/testdata/uploads/file2 b/workhorse/internal/staticpages/testdata/uploads/file2 new file mode 100644 index 00000000000..c061beb8592 --- /dev/null +++ b/workhorse/internal/staticpages/testdata/uploads/file2 @@ -0,0 +1 @@ +contents2
\ No newline at end of file diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index 5bbd245719b..3ccb7298b96 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -62,6 +62,14 @@ const ( importPattern = `^/import/` ) +var ( + // For legacy reasons, user uploads are stored in public/uploads. To + // prevent anybody who knows/guesses the URL of a user-uploaded file + // from downloading it we configure static.ServeExisting to treat files + // under public/uploads/ as if they do not exist. + staticExclude = []string{"/uploads/"} +) + func compileRegexp(regexpStr string) *regexp.Regexp { if len(regexpStr) == 0 { return nil @@ -181,20 +189,20 @@ func buildProxy(backend *url.URL, version string, rt http.RoundTripper, cfg conf // We match against URI not containing the relativeUrlRoot: // see upstream.ServeHTTP -func (u *upstream) configureRoutes() { +func configureRoutes(u *upstream) { api := apipkg.NewAPI( u.Backend, u.Version, u.RoundTripper, ) - static := &staticpages.Static{DocumentRoot: u.DocumentRoot} + static := &staticpages.Static{DocumentRoot: u.DocumentRoot, Exclude: staticExclude} proxy := buildProxy(u.Backend, u.Version, u.RoundTripper, u.Config) cableProxy := proxypkg.NewProxy(u.CableBackend, u.Version, u.CableRoundTripper) assetsNotFoundHandler := NotFoundUnless(u.DevelopmentMode, proxy) if u.AltDocumentRoot != "" { - altStatic := &staticpages.Static{DocumentRoot: u.AltDocumentRoot} + altStatic := &staticpages.Static{DocumentRoot: u.AltDocumentRoot, Exclude: staticExclude} assetsNotFoundHandler = altStatic.ServeExisting( u.URLPrefix, staticpages.CacheExpireMax, @@ -303,12 +311,6 @@ func (u *upstream) configureRoutes() { u.route("POST", snippetUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)), u.route("POST", userUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)), - // For legacy reasons, user uploads are stored under the document root. - // To prevent anybody who knows/guesses the URL of a user-uploaded file - // from downloading it we make sure requests to /uploads/ do _not_ pass - // through static.ServeExisting. - u.route("", `^/uploads/`, static.ErrorPagesUnless(u.DevelopmentMode, staticpages.ErrorFormatHTML, proxy)), - // health checks don't intercept errors and go straight to rails // TODO: We should probably not return a HTML deploy page? // https://gitlab.com/gitlab-org/gitlab-workhorse/issues/230 diff --git a/workhorse/internal/upstream/upstream.go b/workhorse/internal/upstream/upstream.go index c81a21c0ecd..80e7d4056b6 100644 --- a/workhorse/internal/upstream/upstream.go +++ b/workhorse/internal/upstream/upstream.go @@ -40,6 +40,10 @@ type upstream struct { } func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler { + return newUpstream(cfg, accessLogger, configureRoutes) +} + +func newUpstream(cfg config.Config, accessLogger *logrus.Logger, routesCallback func(*upstream)) http.Handler { up := upstream{ Config: cfg, accessLogger: accessLogger, @@ -56,7 +60,7 @@ func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler { up.RoundTripper = roundtripper.NewBackendRoundTripper(up.Backend, up.Socket, up.ProxyHeadersTimeout, cfg.DevelopmentMode) up.CableRoundTripper = roundtripper.NewBackendRoundTripper(up.CableBackend, up.CableSocket, up.ProxyHeadersTimeout, cfg.DevelopmentMode) up.configureURLPrefix() - up.configureRoutes() + routesCallback(&up) var correlationOpts []correlation.InboundHandlerOption if cfg.PropagateCorrelationID { @@ -95,7 +99,7 @@ func (u *upstream) ServeHTTP(w http.ResponseWriter, r *http.Request) { } // Check URL Root - URIPath := urlprefix.CleanURIPath(r.URL.Path) + URIPath := urlprefix.CleanURIPath(r.URL.EscapedPath()) prefix := u.URLPrefix if !prefix.Match(URIPath) { helper.HTTPError(w, r, fmt.Sprintf("Not found %q", URIPath), http.StatusNotFound) diff --git a/workhorse/internal/upstream/upstream_test.go b/workhorse/internal/upstream/upstream_test.go new file mode 100644 index 00000000000..3afc62a7384 --- /dev/null +++ b/workhorse/internal/upstream/upstream_test.go @@ -0,0 +1,67 @@ +package upstream + +import ( + "io" + "io/ioutil" + "net/http" + "net/http/httptest" + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-workhorse/internal/config" +) + +func TestRouting(t *testing.T) { + handle := func(u *upstream, regex string) routeEntry { + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + io.WriteString(w, regex) + }) + return u.route("", regex, handler) + } + + const ( + foobar = `\A/foobar\z` + quxbaz = `\A/quxbaz\z` + main = "" + ) + + u := newUpstream(config.Config{}, logrus.StandardLogger(), func(u *upstream) { + u.Routes = []routeEntry{ + handle(u, foobar), + handle(u, quxbaz), + handle(u, main), + } + }) + + ts := httptest.NewServer(u) + defer ts.Close() + + testCases := []struct { + desc string + path string + route string + }{ + {"main route works", "/", main}, + {"foobar route works", "/foobar", foobar}, + {"quxbaz route works", "/quxbaz", quxbaz}, + {"path traversal works, ends up in quxbaz", "/foobar/../quxbaz", quxbaz}, + {"escaped path traversal does not match any route", "/foobar%2f%2e%2e%2fquxbaz", main}, + {"double escaped path traversal does not match any route", "/foobar%252f%252e%252e%252fquxbaz", main}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + resp, err := http.Get(ts.URL + tc.path) + require.NoError(t, err) + defer resp.Body.Close() + + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + + require.Equal(t, 200, resp.StatusCode, "response code") + require.Equal(t, tc.route, string(body)) + }) + } +} diff --git a/workhorse/main_test.go b/workhorse/main_test.go index d15af1d3e4c..bcbb2f11369 100644 --- a/workhorse/main_test.go +++ b/workhorse/main_test.go @@ -222,12 +222,15 @@ func TestDeniedPublicUploadsFile(t *testing.T) { for _, resource := range []string{ "/uploads/static.txt", "/uploads%2Fstatic.txt", + "/foobar%2F%2E%2E%2Fuploads/static.txt", } { - resp, body := httpGet(t, ws.URL+resource, nil) + t.Run(resource, func(t *testing.T) { + resp, body := httpGet(t, ws.URL+resource, nil) - require.Equal(t, 404, resp.StatusCode, "GET %q: status code", resource) - require.Equal(t, "", body, "GET %q: response body", resource) - require.True(t, proxied, "GET %q: never made it to backend", resource) + require.Equal(t, 404, resp.StatusCode, "GET %q: status code", resource) + require.Equal(t, "", body, "GET %q: response body", resource) + require.True(t, proxied, "GET %q: never made it to backend", resource) + }) } } |