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>

diff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt
index 512f31876e17ed6892ff7f1858a891f0f7938b29..fd8113d6d42f758e8e748eb7b4251411ebf84ccd 100644 (file)
--- 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 572f1785da7d3e3540d523e12cdedfbeaab6ce91..1392a54d5c2c3ef608dc56851774125709f9dd32 100644 (file)
--- 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 935b28a70f16ec788e9df0126ccfb00fa86de7cd..0364d436d242d6aec041ddd04ba5ea85ee0e9f75 100644 (file)
--- 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 {
 #define CREDENTIAL_INIT { \
        .helpers = STRING_LIST_INIT_DUP, \
        .password_expiry_utc = TIME_MAX, \
+       .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 cb91be1427f1d29c9bbf14bed6e5883cabff2cec..b62c70c193eff30f5ee342697bdea0d95437cfa9 100755 (executable)
--- 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_i18ngrep "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 d0211cd8bef450e0149dccd037b0f9e1607ef421..2cd2e1a05987603c4a244a137b8788d044e4e0d0 100755 (executable)
--- 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 8f182a3cbfe73cbcf1d61cbf96c14b78cf25a168..5d0e394609a08cc6f195857decfdd89ace275009 100755 (executable)
--- 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 0908534f2561f4bc68319f4dc4ffc464ba4e4a90..8a27768dfba583feea21b53696316e4b800d9a6e 100755 (executable)
--- 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' '