Skip to content

Commit

Permalink
Fix command injection in go-getter when passing params to hg clone
Browse files Browse the repository at this point in the history
The fix for this is to add -- to the arguments of each hg command,
before any user-input. This indicates the end of optional arguments,
only positional arguments are allowed.

Test Results Before Change
```
~>  go test ./... -run=TestHg -v
=== RUN   TestHgGetter_impl
--- PASS: TestHgGetter_impl (0.00s)
=== RUN   TestHgGetter
--- PASS: TestHgGetter (0.60s)
=== RUN   TestHgGetter_branch
--- PASS: TestHgGetter_branch (0.96s)
=== RUN   TestHgGetter_GetFile
--- PASS: TestHgGetter_GetFile (0.61s)
=== RUN   TestHgGetter_HgArgumentsNotAllowed
=== RUN   TestHgGetter_HgArgumentsNotAllowed/arguments_allowed_in_destination
    get_hg_test.go:144: Expected no err, got: error running /usr/local/bin/hg:
=== RUN   TestHgGetter_HgArgumentsNotAllowed/arguments_passed_into_rev_parameter
    get_hg_test.go:163: Expected no err, got: /usr/local/bin/hg exited with 1:
=== RUN   TestHgGetter_HgArgumentsNotAllowed/arguments_passed_in_the_repository_URL
    get_hg_test.go:182: Expected no err, got: /usr/local/bin/hg exited with 255: hg clone: option -U not recognized
        alias 'clone' resolves to unknown command 'false'
--- FAIL: TestHgGetter_HgArgumentsNotAllowed (1.02s)
    --- FAIL: TestHgGetter_HgArgumentsNotAllowed/arguments_allowed_in_destination (0.15s)
    --- FAIL: TestHgGetter_HgArgumentsNotAllowed/arguments_passed_into_rev_parameter (0.56s)
    --- FAIL: TestHgGetter_HgArgumentsNotAllowed/arguments_passed_in_the_repository_URL (0.31s)
FAIL
```

Test Results After Change
```
~>  go test ./... -run=TestHg -v
=== RUN   TestHgGetter_impl
--- PASS: TestHgGetter_impl (0.00s)
=== RUN   TestHgGetter
--- PASS: TestHgGetter (0.61s)
=== RUN   TestHgGetter_branch
--- PASS: TestHgGetter_branch (0.99s)
=== RUN   TestHgGetter_GetFile
--- PASS: TestHgGetter_GetFile (0.61s)
=== RUN   TestHgGetter_HgArgumentsNotAllowed
=== RUN   TestHgGetter_HgArgumentsNotAllowed/arguments_allowed_in_destination
=== RUN   TestHgGetter_HgArgumentsNotAllowed/arguments_passed_into_rev_parameter
=== RUN   TestHgGetter_HgArgumentsNotAllowed/arguments_passed_in_the_repository_URL
--- PASS: TestHgGetter_HgArgumentsNotAllowed (1.37s)
    --- PASS: TestHgGetter_HgArgumentsNotAllowed/arguments_allowed_in_destination (0.62s)
    --- PASS: TestHgGetter_HgArgumentsNotAllowed/arguments_passed_into_rev_parameter (0.61s)
    --- PASS: TestHgGetter_HgArgumentsNotAllowed/arguments_passed_in_the_repository_URL (0.15s)
PASS
```
  • Loading branch information
nywilken committed May 12, 2022
1 parent 4e45866 commit 693f201
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 2 deletions.
4 changes: 2 additions & 2 deletions get_hg.go
Expand Up @@ -103,7 +103,7 @@ func (g *HgGetter) GetFile(ctx context.Context, req *Request) error {
}

func (g *HgGetter) clone(dst string, u *url.URL) error {
cmd := exec.Command("hg", "clone", "-U", u.String(), dst)
cmd := exec.Command("hg", "clone", "-U", "--", u.String(), dst)
return getRunCommand(cmd)
}

Expand All @@ -116,7 +116,7 @@ func (g *HgGetter) pull(dst string, u *url.URL) error {
func (g *HgGetter) update(ctx context.Context, dst string, u *url.URL, rev string) error {
args := []string{"update"}
if rev != "" {
args = append(args, rev)
args = append(args, "--", rev)
}

cmd := exec.CommandContext(ctx, "hg", args...)
Expand Down
82 changes: 82 additions & 0 deletions get_hg_test.go
Expand Up @@ -2,9 +2,11 @@ package getter

import (
"context"
"net/url"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"

testing_helper "github.com/hashicorp/go-getter/v2/helper/testing"
Expand Down Expand Up @@ -118,3 +120,83 @@ func TestHgGetter_GetFile(t *testing.T) {
}
testing_helper.AssertContents(t, dst, "Hello\n")
}
func TestHgGetter_HgArgumentsNotAllowed(t *testing.T) {
if !testHasHg {
t.Log("hg not found, skipping")
t.Skip()
}
ctx := context.Background()

tc := []struct {
name string
req Request
errChk func(testing.TB, error)
}{
{
// If arguments are allowed in the destination, this request to Get will fail
name: "arguments allowed in destination",
req: Request{
Dst: "--config=alias.clone=!touch ./TEST",
u: testModuleURL("basic-hg"),
},
errChk: func(t testing.TB, err error) {
if err != nil {
t.Errorf("Expected no err, got: %s", err)
}
},
},
{
// Test arguments passed into the `rev` parameter
// This clone call will fail regardless, but an exit code of 1 indicates
// that the `false` command executed
// We are expecting an hg parse error
name: "arguments passed into rev parameter",
req: Request{
u: testModuleURL("basic-hg?rev=--config=alias.update=!false"),
},
errChk: func(t testing.TB, err error) {
if err == nil {
return
}

if !strings.Contains(err.Error(), "hg: parse error") {
t.Errorf("Expected no err, got: %s", err)
}
},
},
{
// Test arguments passed in the repository URL
// This Get call will fail regardless, but it should fail
// because the repository can't be found.
// Other failures indicate that hg interpreted the argument passed in the URL
name: "arguments passed in the repository URL",
req: Request{
u: &url.URL{Path: "--config=alias.clone=false"},
},
errChk: func(t testing.TB, err error) {
if err == nil {
return
}

if !strings.Contains(err.Error(), "repository --config=alias.clone=false not found") {
t.Errorf("Expected no err, got: %s", err)
}
},
},
}
for _, tt := range tc {
tt := tt
t.Run(tt.name, func(t *testing.T) {
g := new(HgGetter)

if tt.req.Dst == "" {
dst := testing_helper.TempDir(t)
tt.req.Dst = dst
}

defer os.RemoveAll(tt.req.Dst)
err := g.Get(ctx, &tt.req)
tt.errChk(t, err)
})
}
}

0 comments on commit 693f201

Please sign in to comment.