From 7dce420ce8fc40e0b3e56cbfdfb8596eb2fe588c Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Mon, 1 Mar 2021 19:16:22 +0000
Subject: [PATCH] Revert "Migrate to labkit error tracking"
This reverts commit 4f0c3682fba9b38631e8a3316996ab42170946af.
---
GITLAB_WORKHORSE_VERSION | 2 +-
.../security-jv-workhorse-router.yml | 5 ++
workhorse/CHANGELOG | 6 ++
workhorse/VERSION | 2 +-
.../internal/staticpages/deploy_page_test.go | 4 +-
.../internal/staticpages/error_pages_test.go | 14 ++--
workhorse/internal/staticpages/servefile.go | 48 +++++++++----
.../internal/staticpages/servefile_test.go | 46 +++++++++++--
workhorse/internal/staticpages/static.go | 1 +
workhorse/internal/staticpages/testdata/file1 | 1 +
.../staticpages/testdata/uploads/file2 | 1 +
workhorse/internal/upstream/routes.go | 20 +++---
workhorse/internal/upstream/upstream.go | 8 ++-
workhorse/internal/upstream/upstream_test.go | 67 +++++++++++++++++++
workhorse/main_test.go | 11 +--
15 files changed, 190 insertions(+), 46 deletions(-)
create mode 100644 changelogs/unreleased/security-jv-workhorse-router.yml
create mode 100644 workhorse/internal/staticpages/testdata/file1
create mode 100644 workhorse/internal/staticpages/testdata/uploads/file2
create mode 100644 workhorse/internal/upstream/upstream_test.go
diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION
index ca1596d1a61c5..bdc95fcda36b4 100644
--- a/GITLAB_WORKHORSE_VERSION
+++ b/GITLAB_WORKHORSE_VERSION
@@ -1 +1 @@
-8.64.0
+8.64.1
diff --git a/changelogs/unreleased/security-jv-workhorse-router.yml b/changelogs/unreleased/security-jv-workhorse-router.yml
new file mode 100644
index 0000000000000..af17a71086d21
--- /dev/null
+++ b/changelogs/unreleased/security-jv-workhorse-router.yml
@@ -0,0 +1,5 @@
+---
+title: 'Workhorse: prevent escaped router path traversal'
+merge_request:
+author:
+type: security
diff --git a/workhorse/CHANGELOG b/workhorse/CHANGELOG
index 21d5bc727c00b..032d82934ee67 100644
--- a/workhorse/CHANGELOG
+++ b/workhorse/CHANGELOG
@@ -1,5 +1,11 @@
# Changelog for gitlab-workhorse
+## v8.64.1
+
+### Security
+- Use URL.EscapePath() in upstream router
+ https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
+
## v8.64.0
### Other
diff --git a/workhorse/VERSION b/workhorse/VERSION
index ca1596d1a61c5..bdc95fcda36b4 100644
--- a/workhorse/VERSION
+++ b/workhorse/VERSION
@@ -1 +1 @@
-8.64.0
+8.64.1
diff --git a/workhorse/internal/staticpages/deploy_page_test.go b/workhorse/internal/staticpages/deploy_page_test.go
index 4b081e73a9744..9bc0a082144dd 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 05ec06cd42931..ab29e187c8eb6 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 c98bc030bc262..fae805862df0d 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,29 @@ 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 {
+ log.WithRequest(r).WithError(err).Error()
+ 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 +65,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 +88,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 e136b87629850..314547b8a5732 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 b42351f15f532..5b804e4d6448f 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 0000000000000..146edcbe0a35a
--- /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 0000000000000..c061beb85924d
--- /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 edcbfa88a6719..fb8a07a803133 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,
@@ -306,12 +314,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 c81a21c0ecd9d..80e7d4056b6dc 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 0000000000000..3afc62a738457
--- /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 b725f36a68a88..4e169b5112f8d 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)
+ })
}
}
--
GitLab