From 05fbde99a87b8a55c78973b27093c05c4d3727aa Mon Sep 17 00:00:00 2001 From: Anirudh Oppiliappan Date: Sun, 26 Jul 2026 18:23:29 +0300 Subject: [PATCH] knotserver/git: harden git clone to prevent command injection Signed-off-by: Anirudh Oppiliappan --- knotserver/git/fork.go | 56 +++++++++++++- knotserver/git/fork_test.go | 143 ++++++++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+), 3 deletions(-) create mode 100644 knotserver/git/fork_test.go diff --git a/knotserver/git/fork.go b/knotserver/git/fork.go index 2996e43b..b03e4a06 100644 --- a/knotserver/git/fork.go +++ b/knotserver/git/fork.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/config" @@ -23,16 +24,19 @@ func Fork(repoPath, source string, cfg *knotconfig.Config) error { // post-clone configure step in sb. The initial clone itself is not sandboxed // because the target directory doesn't exist yet when the ruleset is applied. func ForkWithSandbox(repoPath, source string, cfg *knotconfig.Config, sb sandbox.Backend) error { - u, err := url.Parse(source) + u, err := validateSource(source) if err != nil { - return fmt.Errorf("failed to parse source URL: %w", err) + return err } + localClone := false if o := optimizeClone(u, cfg); o != nil { u = o + localClone = true } - cloneCmd := exec.Command("git", "clone", "--bare", u.String(), repoPath) + cloneCmd := exec.Command("git", cloneArgs(u, repoPath, localClone)...) + cloneCmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") if err := cloneCmd.Run(); err != nil { return fmt.Errorf("failed to bare clone repository: %w", err) } @@ -58,6 +62,52 @@ func ForkWithSandbox(repoPath, source string, cfg *knotconfig.Config, sb sandbox return nil } +// validateSource parses a user-supplied clone source and rejects anything that +// isn't a plain http(s) URL. This blocks argument injection (a leading "-" that +// git would read as an option) and non-http transports such as ext::, file://, +// or ssh that could be abused for command execution or local file access. +func validateSource(source string) (*url.URL, error) { + if strings.HasPrefix(source, "-") { + return nil, fmt.Errorf("invalid source: must not start with '-'") + } + + u, err := url.Parse(source) + if err != nil { + return nil, fmt.Errorf("failed to parse source URL: %w", err) + } + + switch u.Scheme { + case "http", "https": + default: + return nil, fmt.Errorf("invalid source: scheme must be http or https, got %q", u.Scheme) + } + + if u.Host == "" { + return nil, fmt.Errorf("invalid source: missing host") + } + + return u, nil +} + +// cloneArgs builds the argument list for a hardened `git clone`. +// +// protocol.allow=never denies every transport by default; only http and +// https are re-enabled. This blocks ext::, ssh, git, and other transports +// that can potentially lead to command execution. The file transport is allowed +// only for the trusted local-clone optimization. +func cloneArgs(u *url.URL, repoPath string, localClone bool) []string { + args := []string{ + "-c", "protocol.allow=never", + "-c", "protocol.http.allow=always", + "-c", "protocol.https.allow=always", + } + if localClone { + args = append(args, "-c", "protocol.file.allow=always") + } + args = append(args, "clone", "--bare", "--", u.String(), repoPath) + return args +} + func optimizeClone(u *url.URL, cfg *knotconfig.Config) *url.URL { // only optimize if it's the same host if u.Host != cfg.Server.Hostname { diff --git a/knotserver/git/fork_test.go b/knotserver/git/fork_test.go new file mode 100644 index 00000000..19d51c6a --- /dev/null +++ b/knotserver/git/fork_test.go @@ -0,0 +1,143 @@ +package git + +import ( + "os" + "path/filepath" + "slices" + "testing" + + "github.com/stretchr/testify/require" + knotconfig "tangled.org/core/knotserver/config" +) + +// TestValidateSource guards against the argument-injection / RCE reported for +// sh.tangled.repo.create: a source beginning with "--" (e.g. +// "--upload-pack=") was handed straight to `git clone`, which interpreted +// it as an option and executed the embedded command as the git user. +func TestValidateSource(t *testing.T) { + rejected := []struct { + name string + source string + }{ + {"upload-pack injection", "--upload-pack=touch /tmp/pwned"}, + {"upload-pack injection with IFS", "--upload-pack=touch$IFS/tmp/pwned"}, + {"leading dash", "-oProxyCommand=evil"}, + {"leading dash single", "-"}, + {"ext transport", "ext::sh -c 'touch /tmp/pwned'"}, + {"file scheme", "file:///etc/passwd"}, + {"ssh scheme", "ssh://git@example.com/repo"}, + {"git scheme", "git://example.com/repo"}, + {"scp-like syntax", "git@example.com:repo.git"}, + {"empty", ""}, + {"relative path", "some/local/path"}, + {"missing host", "https://"}, + } + + for _, tc := range rejected { + t.Run("reject/"+tc.name, func(t *testing.T) { + _, err := validateSource(tc.source) + require.Error(t, err, "source %q must be rejected", tc.source) + }) + } + + accepted := []struct { + name string + source string + }{ + {"https", "https://example.com/owner/repo"}, + {"http", "http://example.com/owner/repo"}, + {"https with port", "https://example.com:8443/owner/repo.git"}, + {"https with userinfo", "https://user:token@example.com/owner/repo"}, + } + + for _, tc := range accepted { + t.Run("accept/"+tc.name, func(t *testing.T) { + u, err := validateSource(tc.source) + require.NoError(t, err, "source %q must be accepted", tc.source) + require.NotNil(t, u) + }) + } +} + +// TestCloneArgs asserts the hardening flags are present on the clone command: +// the "--" option terminator and a deny-by-default protocol allowlist. +func TestCloneArgs(t *testing.T) { + t.Run("remote http source", func(t *testing.T) { + u, err := validateSource("https://example.com/owner/repo") + require.NoError(t, err) + + args := cloneArgs(u, "/scan/did:plc:abc", false) + + requireOrdered(t, args, "clone", "--bare", "--", "https://example.com/owner/repo", "/scan/did:plc:abc") + requireConfig(t, args, "protocol.allow=never") + requireConfig(t, args, "protocol.http.allow=always") + requireConfig(t, args, "protocol.https.allow=always") + // the file transport must NOT be enabled for a user-provided source. + require.False(t, hasConfig(args, "protocol.file.allow=always"), + "file transport must not be allowlisted for remote sources") + + // the source is the operand immediately after "--", so a leading-dash + // payload can never be read as a flag. + dashIdx := slices.Index(args, "--") + require.Positive(t, dashIdx) + require.Equal(t, "https://example.com/owner/repo", args[dashIdx+1]) + }) + + t.Run("local optimized clone allowlists file", func(t *testing.T) { + u, err := validateSource("https://example.com/owner/repo") + require.NoError(t, err) + + args := cloneArgs(u, "/scan/did:plc:abc", true) + requireConfig(t, args, "protocol.file.allow=always") + }) +} + +// TestForkWithSandboxRejectsInjection is the end-to-end regression: the PoC +// payload must fail before any command runs, and the sentinel file the payload +// would create must never appear. +func TestForkWithSandboxRejectsInjection(t *testing.T) { + tempDir := t.TempDir() + sentinel := filepath.Join(tempDir, "pwned") + + cfg := &knotconfig.Config{} + cfg.Repo.ScanPath = filepath.Join(tempDir, "scan") + cfg.Server.Hostname = "knot.example.com" + + repoPath := filepath.Join(cfg.Repo.ScanPath, "did:plc:victim") + + // mirrors the PoC: --upload-pack=touch , spaces as $IFS. + source := "--upload-pack=touch$IFS" + sentinel + + err := ForkWithSandbox(repoPath, source, cfg, nil) + require.Error(t, err, "malicious source must be rejected") + + _, statErr := os.Stat(sentinel) + require.True(t, os.IsNotExist(statErr), + "command injection executed: sentinel file %s was created", sentinel) +} + +func hasConfig(args []string, kv string) bool { + for i := 0; i+1 < len(args); i++ { + if args[i] == "-c" && args[i+1] == kv { + return true + } + } + return false +} + +func requireConfig(t *testing.T, args []string, kv string) { + t.Helper() + require.True(t, hasConfig(args, kv), "expected -c %s in %v", kv, args) +} + +// requireOrdered asserts want appears as a subsequence (in order) of args. +func requireOrdered(t *testing.T, args []string, want ...string) { + t.Helper() + i := 0 + for _, a := range args { + if i < len(want) && a == want[i] { + i++ + } + } + require.Equal(t, len(want), i, "expected ordered subsequence %v in %v", want, args) +} -- 2.51.2