@@ -270,6 +270,35 @@ helper_test() {
password=
EOF
'
+
+ : ${GIT_TEST_LONG_CRED_BUFFER:=1024}
+ # 23 bytes accounts for "wwwauth[]=basic realm=" plus NUL
+ LONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))
+ LONG_VALUE=$(perl -e 'print "a" x shift' $LONG_VALUE_LEN)
+
+ test_expect_success "helper ($HELPER) not confused by long header" '
+ check approve $HELPER <<-\EOF &&
+ protocol=https
+ host=victim.example.com
+ username=user
+ password=to-be-stolen
+ EOF
+
+ check fill $HELPER <<-EOF
+ protocol=https
+ host=badguy.example.com
+ wwwauth[]=basic realm=${LONG_VALUE}host=victim.example.com
+ --
+ protocol=https
+ host=badguy.example.com
+ username=askpass-username
+ password=askpass-password
+ wwwauth[]=basic realm=${LONG_VALUE}host=victim.example.com
+ --
+ askpass: Username for '\''https://badguy.example.com'\'':
+ askpass: Password for '\''https://askpass-username@badguy.example.com'\'':
+ EOF
+ '
}
helper_test_timeout() {
Add a test ensuring that the "wwwauth[]" field cannot be used to inject malicious data into the credential helper stream. Many of the credential helpers in contrib/credential read the newline-delimited protocol stream one line at a time by repeatedly calling fgets() into a fixed-size buffer. This assumes that each line is no more than 1024 characters long, since each iteration of the loop assumes that it is parsing starting at the beginning of a new line in the stream. However, similar to a5bb10fd5e (config: avoid fixed-sized buffer when renaming/deleting a section, 2023-04-06), if a line is longer than 1024 characters, a malicious actor can embed another command within an existing line, bypassing the usual checks introduced in 9a6bbee800 (credential: avoid writing values with newlines, 2020-03-11). As with the problem fixed in that commit, specially crafted input can cause the helper to return the credential for the wrong host, letting an attacker trick the victim into sending credentials for one host to another. Luckily, all parts of the credential helper protocol that are available in a tagged release of Git are immune to this attack: - "protocol" is restricted to known values, and is thus immune. - "host" is immune because curl will reject hostnames that have a '=' character in them, which would be required to carry out this attack. - "username" is immune, because the buffer characters to fill out the first `fgets()` call would pollute the `username` field, causing the credential helper to return nothing (because it would match a username if present, and the username of the credential to be stolen is likely not 1024 characters). - "password" is immune because providing a password instructs credential helpers to avoid filling credentials in the first place. - "path" is similar to username; if present, it is not likely to match any credential the victim is storing. It's also not enabled by default; the victim would have to set credential.useHTTPPath explicitly. However, the new "wwwauth[]" field introduced via 5f2117b24f (credential: add WWW-Authenticate header to cred requests, 2023-02-27) can be used to inject data into the credential helper stream. For example, running: { printf 'HTTP/1.1 401\r\n' printf 'WWW-Authenticate: basic realm=' perl -e 'print "a" x 1024' printf 'host=victim.com\r\n' } | nc -Nlp 8080 in one terminal, and then: git clone http://localhost:8080 in another would result in a line like: wwwauth[]=basic realm=aaa[...]aaahost=victim.com being sent to the credential helper. If we tweak that "1024" to align our output with the helper's buffer size and the rest of the data on the line, it can cause the helper to see "host=victim.com" on its own line, allowing motivated attackers to exfiltrate credentials belonging to "victim.com". The below test demonstrates these failures and provides us with a test to ensure that our fix is correct. That said, it has a couple of shortcomings: - it's in t0303, since that's the only mechanism we have for testing random helpers. But that means nobody is going to run it under normal circumstances. - to get the attack right, it has to line up the stuffed name with the buffer size, so we depend on the exact buffer size. I parameterized it so it could be used to test other helpers, but in practice it's not likely for anybody to do that. Still, it's the best we can do, and will help us confirm the presence of the problem (and our fixes) in the new few patches. Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/lib-credential.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)