diff options
author | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-26 10:47:12 +0300 |
---|---|---|
committer | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-26 10:47:12 +0300 |
commit | aedbb005ce81270238e21699aa66ed46081ee94d (patch) | |
tree | 6a1e14e0c798ea0cd85e0866738f6e5bc9026357 | |
parent | 5cffa83537890540d74664a43e828cd81a164980 (diff) | |
parent | d4586dad212c0048d2c535392ec4a53ebdf0c51c (diff) |
Merge branch 'security-1-1-fix-toctou-race' into '1-1-stable'v1.1.11-1-stable
[1.1] Fix TOCTOU race condition when serving files
See merge request gitlab/gitlab-pages!5
-rw-r--r-- | .gitlab-ci.yml | 19 | ||||
-rw-r--r-- | CHANGELOG | 3 | ||||
-rw-r--r-- | VERSION | 2 | ||||
-rw-r--r-- | internal/domain/domain.go | 10 | ||||
-rw-r--r-- | internal/domain/domain_test.go | 23 |
5 files changed, 49 insertions, 8 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 771831a9..b9bb16dd 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,10 +1,5 @@ -image: golang:1.8 - .test: &test script: - - make setup - - make verify - - make cover - echo "Running all tests without daemonizing..." - make test - echo "Running just the acceptance tests daemonized (tmpdir)...." @@ -14,9 +9,19 @@ image: golang:1.8 artifacts: paths: - bin/gitlab-pages + +verify: + image: golang:1.11 + script: + - make setup + - make verify + - make cover + artifacts: + paths: - coverage.html test:1.8: + image: golang:1.8 <<: *test test:1.9: @@ -26,3 +31,7 @@ test:1.9: test:1.10: image: golang:1.10 <<: *test + +test:1.11: + image: golang:1.11 + <<: *test @@ -1,3 +1,6 @@ +v 1.1.1 +- Fix TOCTOU race condition when serving files + v 1.1.0 - Fix HTTP to HTTPS redirection not working for default domains !106 - Log duplicate domain names !107 @@ -1 +1 @@ -1.1.0 +1.1.1 diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 4f5c870e..1c7abb98 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -14,6 +14,8 @@ import ( "sync" "time" + "golang.org/x/sys/unix" + "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/httputil" ) @@ -131,7 +133,7 @@ func (d *D) IsHTTPSOnly(r *http.Request) bool { func (d *D) serveFile(w http.ResponseWriter, r *http.Request, origPath string) error { fullPath := handleGZip(w, r, origPath) - file, err := os.Open(fullPath) + file, err := openNoFollow(fullPath) if err != nil { return err } @@ -155,7 +157,7 @@ func (d *D) serveCustomFile(w http.ResponseWriter, r *http.Request, code int, or fullPath := handleGZip(w, r, origPath) // Open and serve content of file - file, err := os.Open(fullPath) + file, err := openNoFollow(fullPath) if err != nil { return err } @@ -330,3 +332,7 @@ func (d *D) ServeHTTP(w http.ResponseWriter, r *http.Request) { func endsWithSlash(path string) bool { return strings.HasSuffix(path, "/") } + +func openNoFollow(path string) (*os.File, error) { + return os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW, 0) +} diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 39976bfe..df4a7fee 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -361,6 +361,29 @@ func TestCacheControlHeaders(t *testing.T) { assert.WithinDuration(t, now.UTC().Add(10*time.Minute), expiresTime.UTC(), time.Minute) } +func TestOpenNoFollow(t *testing.T) { + tmpfile, err := ioutil.TempFile("", "link-test") + require.NoError(t, err) + defer tmpfile.Close() + + orig := tmpfile.Name() + softLink := orig + ".link" + defer os.Remove(orig) + + source, err := openNoFollow(orig) + require.NoError(t, err) + require.NotNil(t, source) + defer source.Close() + + err = os.Symlink(orig, softLink) + require.NoError(t, err) + defer os.Remove(softLink) + + link, err := openNoFollow(softLink) + require.Error(t, err) + require.Nil(t, link) +} + var chdirSet = false func setUpTests() { |