Skip to content

Commit a8bf080

Browse files
chrisd8088chrisd8088
committed
Merge commit from GHSA-q6r2-x2cc-vrp7
Reject bare line-ending control characters in Git credential requests
2 parents e4df482 + 268628b commit a8bf080

File tree

4 files changed

+258
-4
lines changed

4 files changed

+258
-4
lines changed

‎creds/creds.go

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,21 +58,31 @@ func (c Creds) IsMultistage() bool {
5858
return slices.Contains([]string{"1", "true"}, FirstEntryForKey(c, "continue"))
5959
}
6060

61-
func bufferCreds(c Creds) *bytes.Buffer {
61+
func (c Creds) buffer(protectProtocol bool) (*bytes.Buffer, error) {
6262
buf := new(bytes.Buffer)
6363

6464
buf.Write([]byte("capability[]=authtype\n"))
6565
buf.Write([]byte("capability[]=state\n"))
6666
for k, v := range c {
6767
for _, item := range v {
68+
if strings.Contains(item, "\n") {
69+
return nil, errors.Errorf(tr.Tr.Get("credential value for %s contains newline: %q", k, item))
70+
}
71+
if protectProtocol && strings.Contains(item, "\r") {
72+
return nil, errors.Errorf(tr.Tr.Get("credential value for %s contains carriage return: %q\nIf this is intended, set `credential.protectProtocol=false`", k, item))
73+
}
74+
if strings.Contains(item, string(rune(0))) {
75+
return nil, errors.Errorf(tr.Tr.Get("credential value for %s contains null byte: %q", k, item))
76+
}
77+
6878
buf.Write([]byte(k))
6979
buf.Write([]byte("="))
7080
buf.Write([]byte(item))
7181
buf.Write([]byte("\n"))
7282
}
7383
}
7484

75-
return buf
85+
return buf, nil
7686
}
7787

7888
type CredentialHelperContext struct {
@@ -168,6 +178,9 @@ func (ctxt *CredentialHelperContext) GetCredentialHelper(helper CredentialHelper
168178
helpers = append(helpers, ctxt.askpassCredHelper)
169179
}
170180
}
181+
182+
ctxt.commandCredHelper.protectProtocol = ctxt.urlConfig.Bool("credential", rawurl, "protectProtocol", true)
183+
171184
return CredentialHelperWrapper{CredentialHelper: NewCredentialHelpers(append(helpers, ctxt.commandCredHelper)), Input: input, Url: u}
172185
}
173186

@@ -307,7 +320,8 @@ func (a *AskPassCredentialHelper) args(prompt string) []string {
307320
}
308321

309322
type commandCredentialHelper struct {
310-
SkipPrompt bool
323+
SkipPrompt bool
324+
protectProtocol bool
311325
}
312326

313327
func (h *commandCredentialHelper) Fill(creds Creds) (Creds, error) {
@@ -338,7 +352,10 @@ func (h *commandCredentialHelper) exec(subcommand string, input Creds) (Creds, e
338352
if err != nil {
339353
return nil, errors.New(tr.Tr.Get("failed to find `git credential %s`: %v", subcommand, err))
340354
}
341-
cmd.Stdin = bufferCreds(input)
355+
cmd.Stdin, err = input.buffer(h.protectProtocol)
356+
if err != nil {
357+
return nil, errors.New(tr.Tr.Get("invalid input to `git credential %s`: %v", subcommand, err))
358+
}
342359
cmd.Stdout = output
343360
/*
344361
There is a reason we don't read from stderr here:

‎creds/creds_test.go

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,101 @@
11
package creds
22

33
import (
4+
"bytes"
45
"errors"
6+
"slices"
7+
"strings"
58
"testing"
69

710
"github.com/stretchr/testify/assert"
811
)
912

13+
func assertCredsLinesMatch(t *testing.T, expected []string, buf *bytes.Buffer) {
14+
expected = append(expected, "")
15+
actual := strings.SplitAfter(buf.String(), "\n")
16+
17+
slices.Sort(expected)
18+
slices.Sort(actual)
19+
20+
assert.Equal(t, expected, actual)
21+
}
22+
23+
func TestCredsBufferFormat(t *testing.T) {
24+
creds := make(Creds)
25+
26+
expected := []string{"capability[]=authtype\n", "capability[]=state\n"}
27+
28+
buf, err := creds.buffer(true)
29+
assert.NoError(t, err)
30+
assertCredsLinesMatch(t, expected, buf)
31+
32+
creds["protocol"] = []string{"https"}
33+
creds["host"] = []string{"example.com"}
34+
35+
expectedPrefix := strings.Join(expected, "")
36+
expected = append(expected, "protocol=https\n", "host=example.com\n")
37+
38+
buf, err = creds.buffer(true)
39+
assert.NoError(t, err)
40+
assert.True(t, strings.HasPrefix(buf.String(), expectedPrefix))
41+
assertCredsLinesMatch(t, expected, buf)
42+
43+
creds["wwwauth[]"] = []string{"Basic realm=test", "Negotiate"}
44+
45+
expected = append(expected, "wwwauth[]=Basic realm=test\n")
46+
expected = append(expected, "wwwauth[]=Negotiate\n")
47+
48+
buf, err = creds.buffer(true)
49+
assert.NoError(t, err)
50+
assert.True(t, strings.HasPrefix(buf.String(), expectedPrefix))
51+
assertCredsLinesMatch(t, expected, buf)
52+
}
53+
54+
func TestCredsBufferProtect(t *testing.T) {
55+
creds := make(Creds)
56+
57+
// Always disallow LF characters
58+
creds["protocol"] = []string{"https"}
59+
creds["host"] = []string{"one.example.com\nhost=two.example.com"}
60+
61+
buf, err := creds.buffer(false)
62+
assert.Error(t, err)
63+
assert.Nil(t, buf)
64+
65+
buf, err = creds.buffer(true)
66+
assert.Error(t, err)
67+
assert.Nil(t, buf)
68+
69+
// Disallow CR characters unless protocol protection disabled
70+
creds["host"] = []string{"one.example.com\rhost=two.example.com"}
71+
72+
expected := []string{
73+
"capability[]=authtype\n",
74+
"capability[]=state\n",
75+
"protocol=https\n",
76+
"host=one.example.com\rhost=two.example.com\n",
77+
}
78+
79+
buf, err = creds.buffer(false)
80+
assert.NoError(t, err)
81+
assertCredsLinesMatch(t, expected, buf)
82+
83+
buf, err = creds.buffer(true)
84+
assert.Error(t, err)
85+
assert.Nil(t, buf)
86+
87+
// Always disallow null bytes
88+
creds["host"] = []string{"one.example.com\x00host=two.example.com"}
89+
90+
buf, err = creds.buffer(false)
91+
assert.Error(t, err)
92+
assert.Nil(t, buf)
93+
94+
buf, err = creds.buffer(true)
95+
assert.Error(t, err)
96+
assert.Nil(t, buf)
97+
}
98+
1099
type testCredHelper struct {
11100
fillErr error
12101
approveErr error

‎t/cmd/lfstest-gitserver.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"net/http"
2727
"net/http/httptest"
2828
"net/textproto"
29+
"net/url"
2930
"os"
3031
"os/exec"
3132
"regexp"
@@ -257,6 +258,7 @@ func lfsHandler(w http.ResponseWriter, r *http.Request, id string) {
257258
}
258259

259260
func lfsUrl(repo, oid string, redirect bool) string {
261+
repo = url.QueryEscape(repo)
260262
if redirect {
261263
return server.URL + "/redirect307/objects/" + oid + "?r=" + repo
262264
}

‎t/t-credentials-protect.sh

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
#!/usr/bin/env bash
2+
3+
. "$(dirname "$0")/testlib.sh"
4+
5+
ensure_git_version_isnt $VERSION_LOWER "2.3.0"
6+
7+
export CREDSDIR="$REMOTEDIR/creds-credentials-protect"
8+
setup_creds
9+
10+
# Copy the default record file for the test credential helper to match the
11+
# hostname used in the Git LFS configurations of the tests.
12+
cp "$CREDSDIR/127.0.0.1" "$CREDSDIR/localhost"
13+
14+
begin_test "credentials rejected with line feed"
15+
(
16+
set -e
17+
18+
reponame="protect-linefeed"
19+
setup_remote_repo "$reponame"
20+
clone_repo "$reponame" "$reponame"
21+
22+
contents="a"
23+
contents_oid=$(calc_oid "$contents")
24+
25+
git lfs track "*.dat"
26+
printf "%s" "$contents" >a.dat
27+
git add .gitattributes a.dat
28+
git commit -m "add a.dat"
29+
30+
# Using localhost instead of 127.0.0.1 in the LFS API URL ensures this URL
31+
# is used when filling credentials rather than the Git remote URL, which
32+
# would otherwise be used since it would have the same scheme and hostname.
33+
gitserver="$(echo "$GITSERVER" | sed 's/127\.0\.0\.1/localhost/')"
34+
testreponame="test%0a$reponame"
35+
git config lfs.url "$gitserver/$testreponame.git/info/lfs"
36+
37+
GIT_TRACE=1 git lfs push origin main 2>&1 | tee push.log
38+
if [ "0" -eq "${PIPESTATUS[0]}" ]; then
39+
echo >&2 "fatal: expected 'git lfs push' to fail ..."
40+
exit 1
41+
fi
42+
grep "batch response: Git credentials for $gitserver.* not found" push.log
43+
grep "credential value for path contains newline" push.log
44+
refute_server_object "$testreponame" "$contents_oid"
45+
46+
git config credential.protectProtocol false
47+
48+
GIT_TRACE=1 git lfs push origin main 2>&1 | tee push.log
49+
if [ "0" -eq "${PIPESTATUS[0]}" ]; then
50+
echo >&2 "fatal: expected 'git lfs push' to fail ..."
51+
exit 1
52+
fi
53+
grep "batch response: Git credentials for $gitserver.* not found" push.log
54+
grep "credential value for path contains newline" push.log
55+
refute_server_object "$testreponame" "$contents_oid"
56+
)
57+
end_test
58+
59+
begin_test "credentials rejected with carriage return"
60+
(
61+
set -e
62+
63+
reponame="protect-return"
64+
setup_remote_repo "$reponame"
65+
clone_repo "$reponame" "$reponame"
66+
67+
contents="a"
68+
contents_oid=$(calc_oid "$contents")
69+
70+
git lfs track "*.dat"
71+
printf "%s" "$contents" >a.dat
72+
git add .gitattributes a.dat
73+
git commit -m "add a.dat"
74+
75+
# Using localhost instead of 127.0.0.1 in the LFS API URL ensures this URL
76+
# is used when filling credentials rather than the Git remote URL, which
77+
# would otherwise be used since it would have the same scheme and hostname.
78+
gitserver="$(echo "$GITSERVER" | sed 's/127\.0\.0\.1/localhost/')"
79+
testreponame="test%0d$reponame"
80+
git config lfs.url "$gitserver/$testreponame.git/info/lfs"
81+
82+
GIT_TRACE=1 git lfs push origin main 2>&1 | tee push.log
83+
if [ "0" -eq "${PIPESTATUS[0]}" ]; then
84+
echo >&2 "fatal: expected 'git lfs push' to fail ..."
85+
exit 1
86+
fi
87+
grep "batch response: Git credentials for $gitserver.* not found" push.log
88+
grep "credential value for path contains carriage return" push.log
89+
refute_server_object "$testreponame" "$contents_oid"
90+
91+
git config credential.protectProtocol false
92+
93+
git lfs push origin main 2>&1 | tee push.log
94+
if [ "0" -ne "${PIPESTATUS[0]}" ]; then
95+
echo >&2 "fatal: expected 'git lfs push' to succeed ..."
96+
exit 1
97+
fi
98+
[ $(grep -c "Uploading LFS objects: 100% (1/1)" push.log) -eq 1 ]
99+
assert_server_object "$testreponame" "$contents_oid"
100+
)
101+
end_test
102+
103+
begin_test "credentials rejected with null byte"
104+
(
105+
set -e
106+
107+
reponame="protect-null"
108+
setup_remote_repo "$reponame"
109+
clone_repo "$reponame" "$reponame"
110+
111+
contents="a"
112+
contents_oid=$(calc_oid "$contents")
113+
114+
git lfs track "*.dat"
115+
printf "%s" "$contents" >a.dat
116+
git add .gitattributes a.dat
117+
git commit -m "add a.dat"
118+
119+
# Using localhost instead of 127.0.0.1 in the LFS API URL ensures this URL
120+
# is used when filling credentials rather than the Git remote URL, which
121+
# would otherwise be used since it would have the same scheme and hostname.
122+
gitserver="$(echo "$GITSERVER" | sed 's/127\.0\.0\.1/localhost/')"
123+
testreponame="test%00$reponame"
124+
git config lfs.url "$gitserver/$testreponame.git/info/lfs"
125+
126+
GIT_TRACE=1 git lfs push origin main 2>&1 | tee push.log
127+
if [ "0" -eq "${PIPESTATUS[0]}" ]; then
128+
echo >&2 "fatal: expected 'git lfs push' to fail ..."
129+
exit 1
130+
fi
131+
grep "batch response: Git credentials for $gitserver.* not found" push.log
132+
grep "credential value for path contains null byte" push.log
133+
refute_server_object "$testreponame" "$contents_oid"
134+
135+
git config credential.protectProtocol false
136+
137+
GIT_TRACE=1 git lfs push origin main 2>&1 | tee push.log
138+
if [ "0" -eq "${PIPESTATUS[0]}" ]; then
139+
echo >&2 "fatal: expected 'git lfs push' to fail ..."
140+
exit 1
141+
fi
142+
grep "batch response: Git credentials for $gitserver.* not found" push.log
143+
grep "credential value for path contains null byte" push.log
144+
refute_server_object "$testreponame" "$contents_oid"
145+
)
146+
end_test

0 commit comments

Comments
 (0)