use os.Root to mitigate fs security issues

Author: evict

cmd/src/servegit.go

@@ -58,11 +58,18 @@ Documentation at https://sourcegraph.com/docs/admin/code_hosts/src_serve_git
 			dbug = log.New(os.Stderr, "DBUG serve-git: ", log.LstdFlags)
 		}
 
+		root, err := os.OpenRoot(repoDir)
+		if err != nil {
+			return err
+		}
+		defer root.Close()
+
 		s := &servegit.Serve{
-			Addr:  *addrFlag,
-			Root:  repoDir,
-			Info:  log.New(os.Stderr, "serve-git: ", log.LstdFlags),
-			Debug: dbug,
+			Addr:   *addrFlag,
+			Root:   repoDir,
+			RootFS: root,
+			Info:   log.New(os.Stderr, "serve-git: ", log.LstdFlags),
+			Debug:  dbug,
 		}
 
 		if *listFlag {

internal/servegit/gitservice.go

@@ -5,6 +5,7 @@ import (
 	"bytes"
 	"compress/gzip"
 	"context"
+	"io"
 	"net/http"
 	"os"
 	"os/exec"
@@ -65,6 +66,10 @@ type Handler struct {
 	// call the returned function when done executing. If the executation
 	// failed, it will pass in a non-nil error.
 	Trace func(ctx context.Context, svc, repo, protocol string) func(error)
+
+	// RootFS is a traversal safe API that ensures files outside of the
+	// root cannot be opened.
+	RootFS *os.Root
 }
 
 func (s *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
@@ -93,7 +98,10 @@ func (s *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if _, err = os.Stat(dir); os.IsNotExist(err) {
+	// os.Root only accepts relative paths from it's root. So we trim the
+	// prefix.
+	relDir := strings.TrimPrefix(dir, s.RootFS.Name()+string(os.PathSeparator))
+	if _, err = s.RootFS.Stat(relDir); os.IsNotExist(err) {
 		http.Error(w, "repository not found", http.StatusNotFound)
 		return
 	} else if err != nil {
@@ -104,6 +112,7 @@ func (s *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	body := r.Body
 	defer body.Close()
 
+	// TODO(@evict) max filereader
 	if r.Header.Get("Content-Encoding") == "gzip" {
 		gzipReader, err := gzip.NewReader(body)
 		if err != nil {

internal/servegit/gitservice_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 	"net/http/httptest"
+	"os"
 	"os/exec"
 	"path/filepath"
 	"strings"
@@ -31,10 +32,17 @@ func TestHandler(t *testing.T) {
 		runCmd(t, repo, "git", "tag", fmt.Sprintf("v%d", i+1))
 	}
 
+	rootFS, err := os.OpenRoot(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { rootFS.Close() })
+
 	ts := httptest.NewServer(&Handler{
 		Dir: func(_ context.Context, s string) (string, error) {
 			return filepath.Join(root, s, ".git"), nil
 		},
+		RootFS: rootFS,
 	})
 	defer ts.Close()
 

internal/servegit/serve.go

@@ -9,6 +9,7 @@ import (
 	"log"
 	"net"
 	"net/http"
+	"os"
 	"os/exec"
 	pathpkg "path"
 	"path/filepath"
@@ -19,10 +20,11 @@ import (
 )
 
 type Serve struct {
-	Addr  string
-	Root  string
-	Info  *log.Logger
-	Debug *log.Logger
+	Addr   string
+	Root   string
+	RootFS *os.Root
+	Info   *log.Logger
+	Debug  *log.Logger
 }
 
 func (s *Serve) Start() error {
@@ -69,7 +71,7 @@ func (s *Serve) handler() http.Handler {
 
 	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "text/html; charset=utf-8")
-		err := indexHTML.Execute(w, map[string]interface{}{
+		err := indexHTML.Execute(w, map[string]any{
 			"Explain": explainAddr(s.Addr),
 			"Links": []string{
 				"/v1/list-repos",
@@ -100,7 +102,8 @@ func (s *Serve) handler() http.Handler {
 		_ = enc.Encode(&resp)
 	})
 
-	fs := http.FileServer(http.Dir(s.Root))
+	safeFS := http.FS(s.RootFS.FS())
+	fs := http.FileServer(safeFS)
 	svc := &Handler{
 		Dir: func(_ context.Context, name string) (string, error) {
 			return filepath.Join(s.Root, filepath.FromSlash(name)), nil
@@ -117,6 +120,7 @@ func (s *Serve) handler() http.Handler {
 				}
 			}
 		},
+		RootFS: s.RootFS,
 	}
 	mux.Handle("/repos/", http.StripPrefix("/repos/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		// Use git service if git is trying to clone. Otherwise show http.FileServer for convenience

internal/servegit/serve_test.go

@@ -36,13 +36,21 @@ func TestReposHandler(t *testing.T) {
 	}}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
+
 			root := gitInitRepos(t, tc.repos...)
 
+			rootFS, err := os.OpenRoot(root)
+			if err != nil {
+				t.Fatal(err)
+			}
+			t.Cleanup(func() { rootFS.Close() })
+
 			h := (&Serve{
-				Info:  testLogger(t),
-				Debug: discardLogger,
-				Addr:  testAddress,
-				Root:  root,
+				Info:   testLogger(t),
+				Debug:  discardLogger,
+				Addr:   testAddress,
+				Root:   root,
+				RootFS: rootFS,
 			}).handler()
 
 			var want []Repo
@@ -132,6 +140,13 @@ func gitInit(t *testing.T, path string) {
 
 func gitInitRepos(t *testing.T, names ...string) string {
 	root := t.TempDir()
+
+	// We cannot set root on a non-existent dir so we return tmpdir
+	// creating repos-root first causes issues with test clean-up on Windows.
+	if len(names) == 0 {
+		return root
+	}
+
 	root = filepath.Join(root, "repos-root")
 
 	for _, name := range names {
@@ -161,10 +176,17 @@ func TestIgnoreGitSubmodules(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	rootFS, err := os.OpenRoot(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { rootFS.Close() })
+
 	repos, err := (&Serve{
-		Info:  testLogger(t),
-		Debug: discardLogger,
-		Root:  root,
+		Info:   testLogger(t),
+		Debug:  discardLogger,
+		Root:   root,
+		RootFS: rootFS,
 	}).Repos()
 	if err != nil {
 		t.Fatal(err)
@@ -201,6 +223,6 @@ type testWriter struct {
 }
 
 func (tw testWriter) Write(p []byte) (n int, err error) {
-	tw.T.Log(string(p))
+	tw.Log(string(p))
 	return len(p), nil
 }