Merge pull request #78 from entireio/fix/ssh-argument-injection

75a014a→main·

Soph·1mo ago·3 files·+79 added/-2 removed

Enforcing stricter input validation for SSH destinations

Changes

3

• internal

• gitproto

• Mssh.go+27/-2

• Mssh_test.go+49

• syncer

• Mssh_integration_test.go+3

143 unmodified lines

144
145
146
147
148
149
150
151
149
152
153
154
155
156
157
158
2 unmodified lines

161
162
163
164
165
166
167
159
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191

143 unmodified lines

}
    args := []string{"-o", "BatchMode=yes"}
    if port := ep.Port(); port != "" {
        if err := rejectOptionLike("SSH port", port); err != nil {
            return nil, err
        }
        args = append(args, "-p", port)
    }
    args = append(args, destination, remoteCommand)
    // The "--" terminates ssh option parsing so the destination can never be
    // consumed as a flag; rejectOptionLike below is the portable primary guard
    // (older clients ignore unknown operands but all support "--").
    args = append(args, "--", destination, remoteCommand)
    return args, nil
}

2 unmodified lines

return "", errors.New("missing SSH host")
    }
    host := ep.Hostname()
    if err := rejectOptionLike("SSH host", host); err != nil {
        return "", err
    }
    if ep.User != nil && ep.User.Username() != "" {
        return ep.User.Username() + "@" + host, nil
        user := ep.User.Username()
        if err := rejectOptionLike("SSH username", user); err != nil {
            return "", err
        }
        return user + "@" + host, nil
    }
    return host, nil
}

// rejectOptionLike refuses a destination component that begins with "-". Such
// a value would be parsed by ssh as an option rather than an operand — e.g. a
// host of "-oProxyCommand=..." turns into arbitrary local command execution
// (the class of git's CVE-2017-1000117) — and is never a legitimate host,
// username, or port.
func rejectOptionLike(what, value string) error {
    if strings.HasPrefix(value, "-") {
        return fmt.Errorf("refusing %s %q: must not begin with '-'", what, value)
    }
    return nil
}

func sshRemoteCommand(ep *url.URL, service string, gitProtocol string) (string, error) {
    if ep == nil || ep.Path == "" {
        return "", errors.New("missing SSH repository path")
    }

Minternal/gitproto/ssh.go+27/-2

3 unmodified lines

4
5
6
7
8
9
10
4 unmodified lines

15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
205 unmodified lines

268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285

3 unmodified lines

"context"
    "errors"
    "io"
    "net/url"
    "os"
    "path/filepath"
    "strconv"
4 unmodified lines

"github.com/go-git/go-git/v6/plumbing/transport"

// A host or username beginning with "-" must be refused, not handed to ssh as
// an operand: ssh would parse "-oProxyCommand=..." as an option and execute an
// arbitrary local command (git's CVE-2017-1000117 class).
func TestSSHInvocationArgsRejectsOptionLikeDestination(t *testing.T) {
    cases := []struct {
        name string
        ep   *url.URL
    }{
        {"host", &url.URL{Scheme: "ssh", Host: "-oProxyCommand=touch /tmp/pwned", Path: "/repo.git"}},
        {"username", &url.URL{Scheme: "ssh", User: url.User("-oProxyCommand=x"), Host: "example.com", Path: "/repo.git"}},
    }
    for _, tc := range cases {
        t.Run(tc.name, func(t *testing.T) {
            if _, err := sshInvocationArgs(tc.ep, "git-upload-pack", ""); err == nil {
                t.Fatalf("expected option-like %s to be rejected, got nil error", tc.name)
            }
        })
    }
}

// "--" must precede the destination so ssh stops parsing options at it.
func TestSSHInvocationArgsTerminatesOptionsBeforeDestination(t *testing.T) {
    ep := &url.URL{Scheme: "ssh", User: url.User("alice"), Host: "example.com:2222", Path: "/repo.git"}
    args, err := sshInvocationArgs(ep, "git-upload-pack", "")
    if err != nil {
        t.Fatalf("sshInvocationArgs: %v", err)
    }
    destIdx := -1
    for i, a := range args {
        if a == "alice@example.com" {
            destIdx = i
            break
        }
    }
    if destIdx <= 0 {
        t.Fatalf("destination not found in args: %v", args)
    }
    if args[destIdx-1] != "--" {
        t.Fatalf("expected \"--\" immediately before destination, got %v", args)
    }
}

func TestNewSSHConnRequiresBinary(t *testing.T) {
    orig := SSHLookPath
    t.Cleanup(func() { SSHLookPath = orig })
205 unmodified lines

"printf '%s' \"$count\" >" + shellQuote(countFile),
        "dest=\"\"",
        "remote=\"\"",
        "port=\"\"",
        "if [ \"$1\" = \"-o\" ]; then",
        "  shift 2",
        "fi",
        "if [ \"$1\" = \"-p\" ]; then",
        "  port=\"$2\"",
        "  shift 2",
        "fi",
        "if [ \"$1\" = \"--\" ]; then",
        "  shift",
        "fi",
        "if [ -n \"$port\" ]; then",
        "  dest=\"-p $port $1\"",
        "else",
        "  dest=\"$1\"",

Minternal/gitproto/ssh_test.go+49

37 unmodified lines

38
39
40
41
42
43
44
45
46

37 unmodified lines

"if [ \"$1\" = \"-p\" ]; then",
        "  shift 2",
        "fi",
        "if [ \"$1\" = \"--\" ]; then",
        "  shift",
        "fi",
        "dest=\"$1\"",
        "remote=\"$2\"",
        "printf '%s\t%s\n' \"$dest\" \"$remote\" >>" + shSingleQuote(logFile),