Improve Git handler security (#769)

internal/git/commands.go

@@ -109,7 +109,7 @@ func GetFilesOfRepository(user string, gist string, revision string) ([]string,
 		"git",
 		"ls-tree",
 		"--name-only",
-		"--",
+		"--end-of-options",
 		revision,
 	)
 	cmd.Dir = repositoryPath
@@ -136,7 +136,7 @@ func CatFileBatch(user string, gist string, revision string, truncate bool) ([]*
 	repositoryPath := RepositoryPath(user, gist)
 	maxFiles := 50
 
-	lsTreeCmd := exec.Command("git", "ls-tree", "-l", revision)
+	lsTreeCmd := exec.Command("git", "ls-tree", "-l", "--end-of-options", revision)
 	lsTreeCmd.Dir = repositoryPath
 
 	var lsTreeStderr bytes.Buffer
@@ -291,6 +291,7 @@ func GetFileContent(user string, gist string, revision string, filename string,
 		"git",
 		"--no-pager",
 		"show",
+		"--end-of-options",
 		revision+":"+convertURLToOctal(filename),
 	)
 	cmd.Dir = repositoryPath
@@ -315,6 +316,7 @@ func GetFileSize(user string, gist string, revision string, filename string) (ui
 		"git",
 		"cat-file",
 		"-s",
+		"--end-of-options",
 		revision+":"+convertURLToOctal(filename),
 	)
 	cmd.Dir = repositoryPath
@@ -346,6 +348,7 @@ func GetLog(user string, gist string, revision string, skip int, limit int) ([]*
 		strconv.Itoa(skip),
 		"--format=format:c %H%na %aN%nm %ae%nt %at",
 		"--shortstat",
+		"--end-of-options",
 		revision,
 	)
 	cmd.Dir = repositoryPath

internal/web/handlers/gist/revision_test.go

@@ -0,0 +1,55 @@
+package gist_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	webtest "github.com/thomiceli/opengist/internal/web/test"
+)
+
+func TestRevisionArgInjection(t *testing.T) {
+	s := webtest.Setup(t)
+	defer webtest.Teardown(t)
+
+	repoPath, _, user, gistId := s.CreateGist(t, "0")
+
+	markers := []string{
+		filepath.Join(repoPath, "OGPOC_INJECTED:x"),
+		filepath.Join(repoPath, "OGPOC_INJECTED"),
+	}
+	for _, m := range markers {
+		_, err := os.Stat(m)
+		require.Truef(t, os.IsNotExist(err), "marker %s must not exist before the attack", m)
+	}
+
+	base := "/" + user + "/" + gistId
+
+	attacks := []string{
+		base + "/raw/--output=OGPOC_INJECTED/x",      // git show --output= sink
+		base + "/download/--output=OGPOC_INJECTED/x", // git show --output= sink
+		base + "/archive/--output=OGPOC_INJECTED",    // git ls-tree sink
+		base + "/archive/-p",                         // bare short option
+		base + "/rev/--output=OGPOC_INJECTED",        // GistIndex, git ls-tree sink
+		base + "/rev/--all",                          // GistIndex, git ls-tree sink
+	}
+
+	for _, path := range attacks {
+		t.Run(path, func(t *testing.T) {
+			code := s.Request(t, "GET", path, nil, 0).StatusCode
+			require.NotEqualf(t, 200, code, "injection %s must not succeed", path)
+		})
+	}
+
+	for _, m := range markers {
+		_, err := os.Stat(m)
+		require.Truef(t, os.IsNotExist(err),
+			"argument injection must not create a server-side file, but %s exists", m)
+	}
+
+	t.Run("HEAD-still-works", func(t *testing.T) {
+		s.Request(t, "GET", base+"/raw/HEAD/file.txt", nil, 200)
+		s.Request(t, "GET", base+"/archive/HEAD", nil, 200)
+	})
+}

internal/web/handlers/git/dumb_private_test.go

@@ -0,0 +1,139 @@
+package git_test
+
+import (
+	"bytes"
+	"compress/zlib"
+	"encoding/base64"
+	"encoding/hex"
+	"fmt"
+	"io"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	webtest "github.com/thomiceli/opengist/internal/web/test"
+)
+
+func dumbGet(t *testing.T, baseUrl, urlPath, creds string) (int, []byte) {
+	req, err := http.NewRequest("GET", baseUrl+urlPath, nil)
+	require.NoError(t, err)
+	req.Header.Set("User-Agent", "git/2.43.0")
+	if creds != "" {
+		req.Header.Set("Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte(creds)))
+	}
+	resp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	body, _ := io.ReadAll(resp.Body)
+	return resp.StatusCode, body
+}
+
+func masterCommitSha(t *testing.T, baseUrl, user, gistId, ownerCreds string) string {
+	code, body := dumbGet(t, baseUrl, fmt.Sprintf("/%s/%s.git/info/refs", user, gistId), ownerCreds)
+	require.Equalf(t, 200, code, "owner must be able to read dumb info/refs (got %d)", code)
+	for _, line := range strings.Split(string(body), "\n") {
+		if strings.Contains(line, "refs/heads/") {
+			return strings.Fields(line)[0]
+		}
+	}
+	t.Fatalf("no ref found in info/refs body %q", string(body))
+	return ""
+}
+
+func looseObjectPath(user, gistId, sha string) string {
+	return fmt.Sprintf("/%s/%s.git/objects/%s/%s", user, gistId, sha[:2], sha[2:])
+}
+
+func inflateLooseObject(t *testing.T, baseUrl, user, gistId, sha, creds string) (objType string, payload []byte) {
+	code, raw := dumbGet(t, baseUrl, looseObjectPath(user, gistId, sha), creds)
+	require.Equalf(t, 200, code, "loose object %s should be served to the owner (got %d)", sha, code)
+	zr, err := zlib.NewReader(bytes.NewReader(raw))
+	require.NoError(t, err)
+	dec, err := io.ReadAll(zr)
+	require.NoError(t, err)
+	nul := bytes.IndexByte(dec, 0)
+	require.Greater(t, nul, 0)
+	objType = strings.SplitN(string(dec[:nul]), " ", 2)[0]
+	payload = dec[nul+1:]
+	return objType, payload
+}
+
+func TestDumbHttpPrivateGistNotDisclosed(t *testing.T) {
+	s := webtest.Setup(t)
+	defer webtest.Teardown(t)
+
+	baseUrl := s.StartHttpServer(t)
+
+	s.Register(t, "alice")
+
+	_, _, user, privateId := s.CreateGist(t, "2")
+
+	const ownerCreds = "thomas:thomas"
+
+	commitSha := masterCommitSha(t, baseUrl, user, privateId, ownerCreds)
+	require.Regexp(t, "^[0-9a-f]{40}$", commitSha)
+
+	_, commit := inflateLooseObject(t, baseUrl, user, privateId, commitSha, ownerCreds)
+	var treeSha string
+	for _, line := range strings.Split(string(commit), "\n") {
+		if strings.HasPrefix(line, "tree ") {
+			treeSha = strings.TrimPrefix(line, "tree ")
+			break
+		}
+	}
+	require.Regexp(t, "^[0-9a-f]{40}$", treeSha)
+
+	_, tree := inflateLooseObject(t, baseUrl, user, privateId, treeSha, ownerCreds)
+	var blobSha string
+	for len(tree) > 0 {
+		nul := bytes.IndexByte(tree, 0)
+		entry := string(tree[:nul]) // "<mode> <name>"
+		sha := hex.EncodeToString(tree[nul+1 : nul+21])
+		tree = tree[nul+21:]
+		if strings.HasSuffix(entry, " file.txt") {
+			blobSha = sha
+			break
+		}
+	}
+	require.Regexp(t, "^[0-9a-f]{40}$", blobSha)
+
+	_, blob := inflateLooseObject(t, baseUrl, user, privateId, blobSha, ownerCreds)
+	require.Equal(t, "hello world", string(blob), "owner must be able to read the private gist over dumb-http")
+
+	dumbPaths := []string{
+		fmt.Sprintf("/%s/%s.git/info/refs", user, privateId),
+		fmt.Sprintf("/%s/%s.git/HEAD", user, privateId),
+		fmt.Sprintf("/%s/%s.git/objects/info/packs", user, privateId),
+		looseObjectPath(user, privateId, commitSha),
+		looseObjectPath(user, privateId, treeSha),
+		looseObjectPath(user, privateId, blobSha),
+	}
+
+	for _, creds := range []string{"alice:alice", "bogus:not-a-real-account", "thomas:wrongpassword"} {
+		t.Run("denied/"+creds, func(t *testing.T) {
+			for _, p := range dumbPaths {
+				code, body := dumbGet(t, baseUrl, p, creds)
+				require.NotEqualf(t, 200, code,
+					"dumb path %s must NOT be served to non-owner %q (leaked body: %q)", p, creds, string(body))
+				require.Equalf(t, 404, code,
+					"dumb path %s should be denied with 404 for non-owner %q", p, creds)
+			}
+		})
+	}
+
+	// Anonymous (no Authorization header at all) is challenged with 401.
+	t.Run("denied/anonymous", func(t *testing.T) {
+		for _, p := range dumbPaths {
+			code, _ := dumbGet(t, baseUrl, p, "")
+			require.Equalf(t, 401, code, "dumb path %s should challenge an anonymous caller with 401", p)
+		}
+	})
+
+	// The smart pull of the private gist by a non-owner must also stay denied.
+	t.Run("smart-pull-non-owner-denied", func(t *testing.T) {
+		code, _ := dumbGet(t, baseUrl,
+			fmt.Sprintf("/%s/%s.git/info/refs?service=git-upload-pack", user, privateId), "alice:alice")
+		require.Equal(t, 404, code, "smart pull of a private gist by a non-owner must be denied")
+	})
+}

internal/web/handlers/git/http.go

@@ -53,6 +53,13 @@ func GitHttp(ctx *context.Context) error {
 	isPush := ctx.QueryParam("service") == "git-receive-pack" ||
 		strings.HasSuffix(ctx.Request().URL.Path, "git-receive-pack") && !isInfoRefs
 
+	// Anything that is neither an init request, a smart pull, nor a push is a
+	// dumb-protocol read: GET .../info/refs (no service), .../HEAD,
+	// .../objects/*. These stream repository files straight off disk, so they
+	// are read operations equivalent to a pull and must be authorized as one —
+	// never served merely because an Authorization header is present.
+	isDumb := initKind == initNone && !isPull && !isPush
+
 	ctx.SetData("repositoryPath", git.RepositoryPath(gist.User.Username, gist.Uuid))
 
 	allow, err := auth.ShouldAllowUnauthenticatedGistAccess(handlers.ContextAuthInfo{Context: ctx}, true)
@@ -60,9 +67,10 @@ func GitHttp(ctx *context.Context) error {
 		log.Fatal().Err(err).Msg("Cannot check if unauthenticated access is allowed")
 	}
 
-	// No need to authenticate if the user wants to clone/pull ; a non-private
-	// gist ; that exists ; where unauthenticated access is allowed in the instance
-	if isPull && gist.Private != db.PrivateVisibility && gistExists && allow {
+	// No need to authenticate if the user wants to clone/pull (smart or dumb) ;
+	// a non-private gist ; that exists ; where unauthenticated access is allowed
+	// in the instance
+	if (isPull || isDumb) && gist.Private != db.PrivateVisibility && gistExists && allow {
 		return route.handler(ctx)
 	}
 
@@ -80,12 +88,12 @@ func GitHttp(ctx *context.Context) error {
 	switch {
 	case initKind != initNone:
 		return handleInit(ctx, route, initKind, initToken, authUsername, authPassword)
-	case isPull:
+	case isPull || isDumb:
 		return handlePull(ctx, route, gist, gistExists, authUsername, authPassword)
 	case isPush:
 		return handlePush(ctx, route, gist, gistExists, authUsername, authPassword)
 	default:
-		return route.handler(ctx)
+		return ctx.NotFound("Gist not found")
 	}
 }