317 lines
12 KiB
Diff
317 lines
12 KiB
Diff
From 7725b8100ffbbff2750ee4d61a0fcc1f53a086e8 Mon Sep 17 00:00:00 2001
|
|
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
|
Date: Wed, 30 Oct 2024 13:26:10 +0100
|
|
Subject: [PATCH] credential: sanitize the user prompt
|
|
|
|
When asking the user interactively for credentials, we want to avoid
|
|
misleading them e.g. via control sequences that pretend that the URL
|
|
targets a trusted host when it does not.
|
|
|
|
While Git learned, over the course of the preceding commits, to disallow
|
|
URLs containing URL-encoded control characters by default, credential
|
|
helpers are still allowed to specify values very freely (apart from Line
|
|
Feed and NUL characters, anything is allowed), and this would allow,
|
|
say, a username containing control characters to be specified that would
|
|
then be displayed in the interactive terminal prompt asking the user for
|
|
the password, potentially sending those control characters directly to
|
|
the terminal. This is undesirable because control characters can be used
|
|
to mislead users to divulge secret information to untrusted sites.
|
|
|
|
To prevent such an attack vector, let's add a `git_prompt()` that forces
|
|
the displayed text to be sanitized, i.e. displaying question marks
|
|
instead of control characters.
|
|
|
|
Note: While this commit's diff changes a lot of `user@host` strings to
|
|
`user%40host`, which may look suspicious on the surface, there is a good
|
|
reason for that: this string specifies a user name, not a
|
|
<username>@<hostname> combination! In the context of t5541, the actual
|
|
combination looks like this: `user%40@127.0.0.1:5541`. Therefore, these
|
|
string replacements document a net improvement introduced by this
|
|
commit, as `user@host@127.0.0.1` could have left readers wondering where
|
|
the user name ends and where the host name begins.
|
|
|
|
Hinted-at-by: Jeff King <peff@peff.net>
|
|
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
|
---
|
|
Documentation/config/credential.txt | 6 ++++++
|
|
credential.c | 7 ++++++-
|
|
credential.h | 4 +++-
|
|
t/t0300-credentials.sh | 20 ++++++++++++++++++++
|
|
t/t5541-http-push-smart.sh | 6 +++---
|
|
t/t5550-http-fetch-dumb.sh | 14 +++++++-------
|
|
t/t5551-http-fetch-smart.sh | 16 ++++++++--------
|
|
7 files changed, 53 insertions(+), 20 deletions(-)
|
|
|
|
diff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt
|
|
index 512f3187..fd8113d6 100644
|
|
--- a/Documentation/config/credential.txt
|
|
+++ b/Documentation/config/credential.txt
|
|
@@ -14,6 +14,12 @@ credential.useHttpPath::
|
|
or https URL to be important. Defaults to false. See
|
|
linkgit:gitcredentials[7] for more information.
|
|
|
|
+credential.sanitizePrompt::
|
|
+ By default, user names and hosts that are shown as part of the
|
|
+ password prompt are not allowed to contain control characters (they
|
|
+ will be URL-encoded by default). Configure this setting to `false` to
|
|
+ override that behavior.
|
|
+
|
|
credential.username::
|
|
If no username is set for a network authentication, use this username
|
|
by default. See credential.<context>.* below, and
|
|
diff --git a/credential.c b/credential.c
|
|
index 572f1785..1392a54d 100644
|
|
--- a/credential.c
|
|
+++ b/credential.c
|
|
@@ -67,6 +67,8 @@ static int credential_config_callback(const char *var, const char *value,
|
|
}
|
|
else if (!strcmp(key, "usehttppath"))
|
|
c->use_http_path = git_config_bool(var, value);
|
|
+ else if (!strcmp(key, "sanitizeprompt"))
|
|
+ c->sanitize_prompt = git_config_bool(var, value);
|
|
|
|
return 0;
|
|
}
|
|
@@ -179,7 +181,10 @@ static char *credential_ask_one(const char *what, struct credential *c,
|
|
struct strbuf prompt = STRBUF_INIT;
|
|
char *r;
|
|
|
|
- credential_describe(c, &desc);
|
|
+ if (c->sanitize_prompt)
|
|
+ credential_format(c, &desc);
|
|
+ else
|
|
+ credential_describe(c, &desc);
|
|
if (desc.len)
|
|
strbuf_addf(&prompt, "%s for '%s': ", what, desc.buf);
|
|
else
|
|
diff --git a/credential.h b/credential.h
|
|
index 935b28a7..0364d436 100644
|
|
--- a/credential.h
|
|
+++ b/credential.h
|
|
@@ -119,7 +119,8 @@ struct credential {
|
|
configured:1,
|
|
quit:1,
|
|
use_http_path:1,
|
|
- username_from_proto:1;
|
|
+ username_from_proto:1,
|
|
+ sanitize_prompt:1;
|
|
|
|
char *username;
|
|
char *password;
|
|
@@ -132,6 +133,7 @@ struct credential {
|
|
.helpers = STRING_LIST_INIT_DUP, \
|
|
.password_expiry_utc = TIME_MAX, \
|
|
.wwwauth_headers = STRVEC_INIT, \
|
|
+ .sanitize_prompt = 1, \
|
|
}
|
|
|
|
/* Initialize a credential structure, setting all fields to empty. */
|
|
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
|
|
index cb91be14..b62c70c1 100755
|
|
--- a/t/t0300-credentials.sh
|
|
+++ b/t/t0300-credentials.sh
|
|
@@ -45,6 +45,10 @@ test_expect_success 'setup helper scripts' '
|
|
test -z "$pexpiry" || echo password_expiry_utc=$pexpiry
|
|
EOF
|
|
|
|
+ write_script git-credential-cntrl-in-username <<-\EOF &&
|
|
+ printf "username=\\007latrix Lestrange\\n"
|
|
+ EOF
|
|
+
|
|
PATH="$PWD:$PATH"
|
|
'
|
|
|
|
@@ -825,4 +829,20 @@ test_expect_success 'credential config with partial URLs' '
|
|
test_grep "skipping credential lookup for key" stderr
|
|
'
|
|
|
|
+BEL="$(printf '\007')"
|
|
+
|
|
+test_expect_success 'interactive prompt is sanitized' '
|
|
+ check fill cntrl-in-username <<-EOF
|
|
+ protocol=https
|
|
+ host=example.org
|
|
+ --
|
|
+ protocol=https
|
|
+ host=example.org
|
|
+ username=${BEL}latrix Lestrange
|
|
+ password=askpass-password
|
|
+ --
|
|
+ askpass: Password for ${SQ}https://%07latrix%20Lestrange@example.org${SQ}:
|
|
+ EOF
|
|
+'
|
|
+
|
|
test_done
|
|
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
|
|
index d0211cd8..2cd2e1a0 100755
|
|
--- a/t/t5541-http-push-smart.sh
|
|
+++ b/t/t5541-http-push-smart.sh
|
|
@@ -351,7 +351,7 @@ test_expect_success 'push over smart http with auth' '
|
|
git push "$HTTPD_URL"/auth/smart/test_repo.git &&
|
|
git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
|
|
log -1 --format=%s >actual &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
test_cmp expect actual
|
|
'
|
|
|
|
@@ -363,7 +363,7 @@ test_expect_success 'push to auth-only-for-push repo' '
|
|
git push "$HTTPD_URL"/auth-push/smart/test_repo.git &&
|
|
git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
|
|
log -1 --format=%s >actual &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
test_cmp expect actual
|
|
'
|
|
|
|
@@ -393,7 +393,7 @@ test_expect_success 'push into half-auth-complete requires password' '
|
|
git push "$HTTPD_URL/half-auth-complete/smart/half-auth.git" &&
|
|
git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/half-auth.git" \
|
|
log -1 --format=%s >actual &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
test_cmp expect actual
|
|
'
|
|
|
|
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
|
|
index 8f182a3c..5d0e3946 100755
|
|
--- a/t/t5550-http-fetch-dumb.sh
|
|
+++ b/t/t5550-http-fetch-dumb.sh
|
|
@@ -90,13 +90,13 @@ test_expect_success 'http auth can use user/pass in URL' '
|
|
test_expect_success 'http auth can use just user in URL' '
|
|
set_askpass wrong pass@host &&
|
|
git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-pass &&
|
|
- expect_askpass pass user@host
|
|
+ expect_askpass pass user%40host
|
|
'
|
|
|
|
test_expect_success 'http auth can request both user and pass' '
|
|
set_askpass user@host pass@host &&
|
|
git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-both &&
|
|
- expect_askpass both user@host
|
|
+ expect_askpass both user%40host
|
|
'
|
|
|
|
test_expect_success 'http auth respects credential helper config' '
|
|
@@ -114,14 +114,14 @@ test_expect_success 'http auth can get username from config' '
|
|
test_config_global "credential.$HTTPD_URL.username" user@host &&
|
|
set_askpass wrong pass@host &&
|
|
git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-user &&
|
|
- expect_askpass pass user@host
|
|
+ expect_askpass pass user%40host
|
|
'
|
|
|
|
test_expect_success 'configured username does not override URL' '
|
|
test_config_global "credential.$HTTPD_URL.username" wrong &&
|
|
set_askpass wrong pass@host &&
|
|
git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-user2 &&
|
|
- expect_askpass pass user@host
|
|
+ expect_askpass pass user%40host
|
|
'
|
|
|
|
test_expect_success 'set up repo with http submodules' '
|
|
@@ -142,7 +142,7 @@ test_expect_success 'cmdline credential config passes to submodule via clone' '
|
|
set_askpass wrong pass@host &&
|
|
git -c "credential.$HTTPD_URL.username=user@host" \
|
|
clone --recursive super super-clone &&
|
|
- expect_askpass pass user@host
|
|
+ expect_askpass pass user%40host
|
|
'
|
|
|
|
test_expect_success 'cmdline credential config passes submodule via fetch' '
|
|
@@ -153,7 +153,7 @@ test_expect_success 'cmdline credential config passes submodule via fetch' '
|
|
git -C super-clone \
|
|
-c "credential.$HTTPD_URL.username=user@host" \
|
|
fetch --recurse-submodules &&
|
|
- expect_askpass pass user@host
|
|
+ expect_askpass pass user%40host
|
|
'
|
|
|
|
test_expect_success 'cmdline credential config passes submodule update' '
|
|
@@ -170,7 +170,7 @@ test_expect_success 'cmdline credential config passes submodule update' '
|
|
git -C super-clone \
|
|
-c "credential.$HTTPD_URL.username=user@host" \
|
|
submodule update &&
|
|
- expect_askpass pass user@host
|
|
+ expect_askpass pass user%40host
|
|
'
|
|
|
|
test_expect_success 'fetch changes via http' '
|
|
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
|
|
index 0908534f..8a27768d 100755
|
|
--- a/t/t5551-http-fetch-smart.sh
|
|
+++ b/t/t5551-http-fetch-smart.sh
|
|
@@ -181,7 +181,7 @@ test_expect_success 'clone from password-protected repository' '
|
|
echo two >expect &&
|
|
set_askpass user@host pass@host &&
|
|
git clone --bare "$HTTPD_URL/auth/smart/repo.git" smart-auth &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
git --git-dir=smart-auth log -1 --format=%s >actual &&
|
|
test_cmp expect actual
|
|
'
|
|
@@ -199,7 +199,7 @@ test_expect_success 'clone from auth-only-for-objects repository' '
|
|
echo two >expect &&
|
|
set_askpass user@host pass@host &&
|
|
git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
git --git-dir=half-auth log -1 --format=%s >actual &&
|
|
test_cmp expect actual
|
|
'
|
|
@@ -224,14 +224,14 @@ test_expect_success 'redirects send auth to new location' '
|
|
set_askpass user@host pass@host &&
|
|
git -c credential.useHttpPath=true \
|
|
clone $HTTPD_URL/smart-redir-auth/repo.git repo-redir-auth &&
|
|
- expect_askpass both user@host auth/smart/repo.git
|
|
+ expect_askpass both user%40host auth/smart/repo.git
|
|
'
|
|
|
|
test_expect_success 'GIT_TRACE_CURL redacts auth details' '
|
|
rm -rf redact-auth trace &&
|
|
set_askpass user@host pass@host &&
|
|
GIT_TRACE_CURL="$(pwd)/trace" git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
|
|
# Ensure that there is no "Basic" followed by a base64 string, but that
|
|
# the auth details are redacted
|
|
@@ -243,7 +243,7 @@ test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '
|
|
rm -rf redact-auth trace &&
|
|
set_askpass user@host pass@host &&
|
|
GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth 2>trace &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
|
|
# Ensure that there is no "Basic" followed by a base64 string, but that
|
|
# the auth details are redacted
|
|
@@ -256,7 +256,7 @@ test_expect_success 'GIT_TRACE_CURL does not redact auth details if GIT_TRACE_RE
|
|
set_askpass user@host pass@host &&
|
|
GIT_TRACE_REDACT=0 GIT_TRACE_CURL="$(pwd)/trace" \
|
|
git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
|
|
grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace
|
|
'
|
|
@@ -568,7 +568,7 @@ test_expect_success 'http auth remembers successful credentials' '
|
|
# the first request prompts the user...
|
|
set_askpass user@host pass@host &&
|
|
git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
|
|
- expect_askpass both user@host &&
|
|
+ expect_askpass both user%40host &&
|
|
|
|
# ...and the second one uses the stored value rather than
|
|
# prompting the user.
|
|
@@ -599,7 +599,7 @@ test_expect_success 'http auth forgets bogus credentials' '
|
|
# us to prompt the user again.
|
|
set_askpass user@host pass@host &&
|
|
git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
|
|
- expect_askpass both user@host
|
|
+ expect_askpass both user%40host
|
|
'
|
|
|
|
test_expect_success 'client falls back from v2 to v0 to match server' '
|
|
--
|
|
2.23.0
|