diff --git a/internal/git/commands.go b/internal/git/commands.go index 0b1e854..84a3841 100644 --- a/internal/git/commands.go +++ b/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 diff --git a/internal/web/handlers/gist/revision_test.go b/internal/web/handlers/gist/revision_test.go new file mode 100644 index 0000000..52d535f --- /dev/null +++ b/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) + }) +} diff --git a/internal/web/handlers/git/dumb_private_test.go b/internal/web/handlers/git/dumb_private_test.go new file mode 100644 index 0000000..f870f19 --- /dev/null +++ b/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]) // " " + 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") + }) +} diff --git a/internal/web/handlers/git/http.go b/internal/web/handlers/git/http.go index 241c5eb..57d6ca6 100644 --- a/internal/web/handlers/git/http.go +++ b/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") } }