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

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
diff --git a/CHANGELOG b/CHANGELOG
index 8d210afd..77f01494 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -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
diff --git a/VERSION b/VERSION
index 9084fa2f..524cb552 100644
--- a/VERSION
+++ b/VERSION
@@ -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() {