From 0d606d8c2a387189bd5cf453e64519c314283a96 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 9 Sep 2024 16:00:20 -0700 Subject: [PATCH 01/12] ci: remove 'Upload failed tests' directories' step from linux32 jobs Linux32 jobs seem to be getting: Error: This request has been automatically failed because it uses a deprecated version of `actions/upload-artifact: v1`. Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/ before doing anything useful. For now, disable the step. Ever since actions/upload-artifact@v1 got disabled, mentioning the offending version of it seems to stop anything from happening. At least this should run the same build and test. See https://github.com/git/git/actions/runs/10780030750/job/29894867249 for example. [Backported from 90f2c7240cc (ci: remove 'Upload failed tests' directories' step from linux32 jobs, 2024-09-09).] Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bd390ab5870267..8eedf35011e906 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -334,12 +334,6 @@ jobs: with: name: failed-tests-${{matrix.vector.jobname}} path: ${{env.FAILED_TEST_ARTIFACTS}} - - name: Upload failed tests' directories - if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname == 'linux32' - uses: actions/upload-artifact@v1 # cannot be upgraded because Node.js Actions aren't supported in this container - with: - name: failed-tests-${{matrix.vector.jobname}} - path: ${{env.FAILED_TEST_ARTIFACTS}} static-analysis: needs: ci-config if: needs.ci-config.outputs.enabled == 'yes' From 5d828879f320501ba8fa75099ee01dc6f4c3cf77 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 19 Jun 2024 06:09:58 +0000 Subject: [PATCH 02/12] mingw: drop bogus (and unneeded) declaration of `_pgmptr` In 08809c09aa13 (mingw: add a helper function to attach GDB to the current process, 2020-02-13), I added a declaration that was not needed. Back then, that did not matter, but now that the declaration of that symbol was changed in mingw-w64's headers, it causes the following compile error: CC compat/mingw.o compat/mingw.c: In function 'open_in_gdb': compat/mingw.c:35:9: error: function declaration isn't a prototype [-Werror=strict-prototypes] 35 | extern char *_pgmptr; | ^~~~~~ In file included from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/mm_malloc.h:27, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/xmmintrin.h:34, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/immintrin.h:31, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/x86intrin.h:32, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/winnt.h:1658, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/minwindef.h:163, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/windef.h:9, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/windows.h:69, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/winsock2.h:23, from compat/../git-compat-util.h:215, from compat/mingw.c:1: compat/mingw.c:35:22: error: '__p__pgmptr' redeclared without dllimport attribute: previous dllimport ignored [-Werror=attributes] 35 | extern char *_pgmptr; | ^~~~~~~ Let's just drop the declaration and get rid of this compile error. [Backported from 3c295c87c25 (mingw: drop bogus (and unneeded) declaration of `_pgmptr`, 2024-06-19).] Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- compat/mingw.c | 1 - 1 file changed, 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index e433740381be9c..e08db82635932d 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -20,7 +20,6 @@ static const int delay[] = { 0, 1, 10, 20, 40 }; void open_in_gdb(void) { static struct child_process cp = CHILD_PROCESS_INIT; - extern char *_pgmptr; strvec_pushl(&cp.args, "mintty", "gdb", NULL); strvec_pushf(&cp.args, "--pid=%d", getpid()); From 7e6073d27083054773f3c3b21a608f400cf7348d Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 11 May 2024 23:25:04 -0700 Subject: [PATCH 03/12] compat/regex: fix argument order to calloc(3) Windows compiler suddenly started complaining that calloc(3) takes its arguments in order. Indeed, there are many calls that has their arguments in a _wrong_ order. Fix them all. A sample breakage can be seen at https://github.com/git/git/actions/runs/9046793153/job/24857988702#step:4:272 [Backported from f01301aabe1 (compat/regex: fix argument order to calloc(3), 2024-05-11).] Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- compat/regex/regcomp.c | 12 ++++++------ compat/regex/regex_internal.c | 4 ++-- compat/regex/regexec.c | 10 +++++----- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c index d1bc09e49b6675..2bc0f1187a18f2 100644 --- a/compat/regex/regcomp.c +++ b/compat/regex/regcomp.c @@ -868,7 +868,7 @@ init_dfa (re_dfa_t *dfa, size_t pat_len) if (table_size > pat_len) break; - dfa->state_table = calloc (sizeof (struct re_state_table_entry), table_size); + dfa->state_table = calloc (table_size, sizeof (struct re_state_table_entry)); dfa->state_hash_mask = table_size - 1; dfa->mb_cur_max = MB_CUR_MAX; @@ -936,7 +936,7 @@ init_dfa (re_dfa_t *dfa, size_t pat_len) { int i, j, ch; - dfa->sb_char = (re_bitset_ptr_t) calloc (sizeof (bitset_t), 1); + dfa->sb_char = (re_bitset_ptr_t) calloc (1, sizeof (bitset_t)); if (BE (dfa->sb_char == NULL, 0)) return REG_ESPACE; @@ -3079,9 +3079,9 @@ parse_bracket_exp (re_string_t *regexp, re_dfa_t *dfa, re_token_t *token, _NL_COLLATE_SYMB_EXTRAMB); } #endif - sbcset = (re_bitset_ptr_t) calloc (sizeof (bitset_t), 1); + sbcset = (re_bitset_ptr_t) calloc (1, sizeof (bitset_t)); #ifdef RE_ENABLE_I18N - mbcset = (re_charset_t *) calloc (sizeof (re_charset_t), 1); + mbcset = (re_charset_t *) calloc (1, sizeof (re_charset_t)); #endif /* RE_ENABLE_I18N */ #ifdef RE_ENABLE_I18N if (BE (sbcset == NULL || mbcset == NULL, 0)) @@ -3626,9 +3626,9 @@ build_charclass_op (re_dfa_t *dfa, RE_TRANSLATE_TYPE trans, re_token_t br_token; bin_tree_t *tree; - sbcset = (re_bitset_ptr_t) calloc (sizeof (bitset_t), 1); + sbcset = (re_bitset_ptr_t) calloc (1, sizeof (bitset_t)); #ifdef RE_ENABLE_I18N - mbcset = (re_charset_t *) calloc (sizeof (re_charset_t), 1); + mbcset = (re_charset_t *) calloc (1, sizeof (re_charset_t)); #endif /* RE_ENABLE_I18N */ #ifdef RE_ENABLE_I18N diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c index ec51cf34461ef2..ec5cc5d2dd10f8 100644 --- a/compat/regex/regex_internal.c +++ b/compat/regex/regex_internal.c @@ -1628,7 +1628,7 @@ create_ci_newstate (const re_dfa_t *dfa, const re_node_set *nodes, reg_errcode_t err; re_dfastate_t *newstate; - newstate = (re_dfastate_t *) calloc (sizeof (re_dfastate_t), 1); + newstate = (re_dfastate_t *) calloc (1, sizeof (re_dfastate_t)); if (BE (newstate == NULL, 0)) return NULL; err = re_node_set_init_copy (&newstate->nodes, nodes); @@ -1678,7 +1678,7 @@ create_cd_newstate (const re_dfa_t *dfa, const re_node_set *nodes, reg_errcode_t err; re_dfastate_t *newstate; - newstate = (re_dfastate_t *) calloc (sizeof (re_dfastate_t), 1); + newstate = (re_dfastate_t *) calloc (1, sizeof (re_dfastate_t)); if (BE (newstate == NULL, 0)) return NULL; err = re_node_set_init_copy (&newstate->nodes, nodes); diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c index 49358ae475c1f7..e92be5741d14c5 100644 --- a/compat/regex/regexec.c +++ b/compat/regex/regexec.c @@ -2796,8 +2796,8 @@ get_subexp (re_match_context_t *mctx, int bkref_node, int bkref_str_idx) continue; /* No. */ if (sub_top->path == NULL) { - sub_top->path = calloc (sizeof (state_array_t), - sl_str - sub_top->str_idx + 1); + sub_top->path = calloc (sl_str - sub_top->str_idx + 1, + sizeof (state_array_t)); if (sub_top->path == NULL) return REG_ESPACE; } @@ -3361,7 +3361,7 @@ build_trtable (const re_dfa_t *dfa, re_dfastate_t *state) if (ndests == 0) { state->trtable = (re_dfastate_t **) - calloc (sizeof (re_dfastate_t *), SBC_MAX); + calloc (SBC_MAX, sizeof (re_dfastate_t *)); return 1; } return 0; @@ -3457,7 +3457,7 @@ build_trtable (const re_dfa_t *dfa, re_dfastate_t *state) discern by looking at the character code: allocate a 256-entry transition table. */ trtable = state->trtable = - (re_dfastate_t **) calloc (sizeof (re_dfastate_t *), SBC_MAX); + (re_dfastate_t **) calloc (SBC_MAX, sizeof (re_dfastate_t *)); if (BE (trtable == NULL, 0)) goto out_free; @@ -3488,7 +3488,7 @@ build_trtable (const re_dfa_t *dfa, re_dfastate_t *state) transition tables, one starting at trtable[0] and one starting at trtable[SBC_MAX]. */ trtable = state->word_trtable = - (re_dfastate_t **) calloc (sizeof (re_dfastate_t *), 2 * SBC_MAX); + (re_dfastate_t **) calloc (2 * SBC_MAX, sizeof (re_dfastate_t *)); if (BE (trtable == NULL, 0)) goto out_free; From 83b08eb19f05710a92d565124606dcaf68bcc68b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 9 Oct 2024 15:25:18 +0200 Subject: [PATCH 04/12] t7300: work around platform-specific behaviour with long paths on MinGW Windows by default has a restriction in place to only allow paths up to 260 characters. This restriction can nowadays be lifted by setting a registry key, but is still active by default. In t7300 we have one test that exercises the behaviour of git-clean(1) with such long paths. Interestingly enough, this test fails on my system that uses Windows 10 with mingw-w64 installed via MSYS2: instead of observing ENAMETOOLONG, we observe ENOENT. This behaviour is consistent across multiple different environments I have tried. I cannot say why exactly we observe a different error here, but I would not be surprised if this was either dependent on the Windows version, the version of MinGW, the current working directory of Git or any kind of combination of these. Work around the issue by handling both errors. [Backported from 106834e34a2 (t7300: work around platform-specific behaviour with long paths on MinGW, 2024-10-09).] Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- t/t7300-clean.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index c975eb54d234d7..a831438f74221b 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -735,7 +735,7 @@ test_expect_success MINGW 'handle clean & core.longpaths = false nicely' ' test_must_fail git clean -xdf 2>.git/err && # grepping for a strerror string is unportable but it is OK here with # MINGW prereq - test_i18ngrep "too long" .git/err + test_i18ngrep -e "too long" -e "No such file or directory" .git/err ' test_expect_success 'clean untracked paths by pathspec' ' From c903985bf7e772e2d08275c1a95c8a55ab011577 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Nov 2024 08:57:52 +0100 Subject: [PATCH 05/12] credential_format(): also encode [:] An upcoming change wants to sanitize the credential password prompt where a URL is displayed that may potentially come from a `.gitmodules` file. To this end, the `credential_format()` function is employed. To sanitize the host name (and optional port) part of the URL, we need a new mode of the `strbuf_add_percentencode()` function because the current mode is both too strict and too lenient: too strict because it encodes `:`, `[` and `]` (which should be left unencoded in `:` and in IPv6 addresses), and too lenient because it does not encode invalid host name characters `/`, `_` and `~`. So let's introduce and use a new mode specifically to encode the host name and optional port part of a URI, leaving alpha-numerical characters, periods, colons and brackets alone and encoding all others. This only leads to a change of behavior for URLs that contain invalid host names. Signed-off-by: Johannes Schindelin --- credential.c | 3 ++- strbuf.c | 4 +++- strbuf.h | 1 + t/t0300-credentials.sh | 13 +++++++++++++ 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/credential.c b/credential.c index f32011343f9400..572f1785da7d3e 100644 --- a/credential.c +++ b/credential.c @@ -164,7 +164,8 @@ static void credential_format(struct credential *c, struct strbuf *out) strbuf_addch(out, '@'); } if (c->host) - strbuf_addstr(out, c->host); + strbuf_add_percentencode(out, c->host, + STRBUF_ENCODE_HOST_AND_PORT); if (c->path) { strbuf_addch(out, '/'); strbuf_add_percentencode(out, c->path, 0); diff --git a/strbuf.c b/strbuf.c index c383f41a3c5ccc..756b96c56157c3 100644 --- a/strbuf.c +++ b/strbuf.c @@ -492,7 +492,9 @@ void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags) unsigned char ch = src[i]; if (ch <= 0x1F || ch >= 0x7F || (ch == '/' && (flags & STRBUF_ENCODE_SLASH)) || - strchr(URL_UNSAFE_CHARS, ch)) + ((flags & STRBUF_ENCODE_HOST_AND_PORT) ? + !isalnum(ch) && !strchr("-.:[]", ch) : + !!strchr(URL_UNSAFE_CHARS, ch))) strbuf_addf(dst, "%%%02X", (unsigned char)ch); else strbuf_addch(dst, ch); diff --git a/strbuf.h b/strbuf.h index f6dbb9681ee768..f9f8bb0381b3c5 100644 --- a/strbuf.h +++ b/strbuf.h @@ -380,6 +380,7 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src); #define STRBUF_ENCODE_SLASH 1 +#define STRBUF_ENCODE_HOST_AND_PORT 2 /** * Append the contents of a string to a strbuf, percent-encoding any characters diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index c66d91e82d8bc7..cb91be1427f1d2 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -514,6 +514,19 @@ test_expect_success 'match percent-encoded values in username' ' EOF ' +test_expect_success 'match percent-encoded values in hostname' ' + test_config "credential.https://a%20b%20c/.helper" "$HELPER" && + check fill <<-\EOF + url=https://a b c/ + -- + protocol=https + host=a b c + username=foo + password=bar + -- + EOF +' + test_expect_success 'fetch with multiple path components' ' test_unconfig credential.helper && test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && From 5b257412e25ad29410c389300324886aa59e1f83 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 6 Nov 2024 20:34:50 +0100 Subject: [PATCH 06/12] sideband: mask control characters The output of `git clone` is a vital component for understanding what has happened when things go wrong. However, these logs are partially under the control of the remote server (via the "sideband", which typically contains what the remote `git pack-objects` process sends to `stderr`), and is currently not sanitized by Git. This makes Git susceptible to ANSI escape sequence injection (see CWE-150, https://cwe.mitre.org/data/definitions/150.html), which allows attackers to corrupt terminal state, to hide information, and even to insert characters into the input buffer (i.e. as if the user had typed those characters). To plug this vulnerability, disallow any control character in the sideband, replacing them instead with the common `^` (e.g. `^[` for `\x1b`, `^A` for `\x01`). There is likely a need for more fine-grained controls instead of using a "heavy hammer" like this, which will be introduced subsequently. Signed-off-by: Johannes Schindelin --- sideband.c | 17 +++++++++++++++-- t/t5409-colorize-remote-messages.sh | 12 ++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/sideband.c b/sideband.c index 85bddfdcd4f57a..9384cb02d56a04 100644 --- a/sideband.c +++ b/sideband.c @@ -61,6 +61,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref list_config_item(list, prefix, keywords[i].keyword); } +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) +{ + strbuf_grow(dest, n); + for (; n && *src; src++, n--) { + if (!iscntrl(*src) || *src == '\t' || *src == '\n') + strbuf_addch(dest, *src); + else { + strbuf_addch(dest, '^'); + strbuf_addch(dest, 0x40 + *src); + } + } +} + /* * Optionally highlight one keyword in remote output if it appears at the start * of the line. This should be called for a single line only, which is @@ -73,7 +86,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) int i; if (!want_color_stderr(use_sideband_colors())) { - strbuf_add(dest, src, n); + strbuf_add_sanitized(dest, src, n); return; } @@ -106,7 +119,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) } } - strbuf_add(dest, src, n); + strbuf_add_sanitized(dest, src, n); } diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index fa5de4500a4f50..6a6e0d15b21050 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -98,4 +98,16 @@ test_expect_success 'fallback to color.ui' ' grep "error: error" decoded ' +test_expect_success 'disallow (color) control sequences in sideband' ' + write_script .git/color-me-surprised <<-\EOF && + printf "error: Have you \\033[31mread\\033[m this?\\n" >&2 + exec "$@" + EOF + test_config_global uploadPack.packObjectshook ./color-me-surprised && + test_commit need-at-least-one-commit && + git clone --no-local . throw-away 2>stderr && + test_decode_color decoded && + test_i18ngrep ! RED decoded +' + test_done From 7725b8100ffbbff2750ee4d61a0fcc1f53a086e8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 30 Oct 2024 13:26:10 +0100 Subject: [PATCH 07/12] 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 @ 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 Signed-off-by: Johannes Schindelin --- 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 512f31876e17ed..fd8113d6d42f75 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..* below, and diff --git a/credential.c b/credential.c index 572f1785da7d3e..1392a54d5c2c3e 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 935b28a70f16ec..0364d436d242d6 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 { #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 cb91be1427f1d2..b62c70c193eff3 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_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 d0211cd8bef450..2cd2e1a0598760 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 8f182a3cbfe73c..5d0e394609a08c 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 0908534f2561f4..8a27768dfba583 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' ' From a8c289b0a531d25336a96eaa5e3584414ed4c6c4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 6 Nov 2024 21:07:51 +0100 Subject: [PATCH 08/12] sideband: introduce an "escape hatch" to allow control characters The preceding commit fixed the vulnerability whereas sideband messages (that are under the control of the remote server) could contain ANSI escape sequences that would be sent to the terminal verbatim. However, this fix may not be desirable under all circumstances, e.g. when remote servers deliberately add coloring to their messages to increase their urgency. To help with those use cases, give users a way to opt-out of the protections: `sideband.allowControlCharacters`. Signed-off-by: Johannes Schindelin --- Documentation/config.txt | 2 ++ Documentation/config/sideband.txt | 5 +++++ sideband.c | 10 ++++++++++ t/t5409-colorize-remote-messages.sh | 8 +++++++- 4 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 Documentation/config/sideband.txt diff --git a/Documentation/config.txt b/Documentation/config.txt index 0e93aef86264db..abdbfba9bd756a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -511,6 +511,8 @@ include::config/sequencer.txt[] include::config/showbranch.txt[] +include::config/sideband.txt[] + include::config/sparse.txt[] include::config/splitindex.txt[] diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt new file mode 100644 index 00000000000000..3fb5045cd79581 --- /dev/null +++ b/Documentation/config/sideband.txt @@ -0,0 +1,5 @@ +sideband.allowControlCharacters:: + By default, control characters that are delivered via the sideband + are masked, to prevent potentially unwanted ANSI escape sequences + from being sent to the terminal. Use this config setting to override + this behavior. diff --git a/sideband.c b/sideband.c index 9384cb02d56a04..8ebf1f0743e6b6 100644 --- a/sideband.c +++ b/sideband.c @@ -20,6 +20,8 @@ static struct keyword_entry keywords[] = { { "error", GIT_COLOR_BOLD_RED }, }; +static int allow_control_characters; + /* Returns a color setting (GIT_COLOR_NEVER, etc). */ static int use_sideband_colors(void) { @@ -33,6 +35,9 @@ static int use_sideband_colors(void) if (use_sideband_colors_cached >= 0) return use_sideband_colors_cached; + git_config_get_bool("sideband.allowcontrolcharacters", + &allow_control_characters); + if (!git_config_get_string(key, &value)) { use_sideband_colors_cached = git_config_colorbool(key, value); } else if (!git_config_get_string("color.ui", &value)) { @@ -63,6 +68,11 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) { + if (allow_control_characters) { + strbuf_add(dest, src, n); + return; + } + strbuf_grow(dest, n); for (; n && *src; src++, n--) { if (!iscntrl(*src) || *src == '\t' || *src == '\n') diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index 6a6e0d15b21050..1cd0640f200009 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -105,9 +105,15 @@ test_expect_success 'disallow (color) control sequences in sideband' ' EOF test_config_global uploadPack.packObjectshook ./color-me-surprised && test_commit need-at-least-one-commit && + git clone --no-local . throw-away 2>stderr && test_decode_color decoded && - test_i18ngrep ! RED decoded + test_i18ngrep ! RED decoded && + + rm -rf throw-away && + git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr && + test_decode_color decoded && + test_i18ngrep RED decoded ' test_done From b01b9b81d36759cdcd07305e78765199e1bc2060 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 4 Nov 2024 14:48:22 +0100 Subject: [PATCH 09/12] credential: disallow Carriage Returns in the protocol by default While Git has documented that the credential protocol is line-based, with newlines as terminators, the exact shape of a newline has not been documented. From Git's perspective, which is firmly rooted in the Linux ecosystem, it is clear that "a newline" means a Line Feed character. However, even Git's credential protocol respects Windows line endings (a Carriage Return character followed by a Line Feed character, "CR/LF") by virtue of using `strbuf_getline()`. There is a third category of line endings that has been used originally by MacOS, and that is respected by the default line readers of .NET and node.js: bare Carriage Returns. Git cannot handle those, and what is worse: Git's remedy against CVE-2020-5260 does not catch when credential helpers are used that interpret bare Carriage Returns as newlines. Git Credential Manager addressed this as CVE-2024-50338, but other credential helpers may still be vulnerable. So let's not only disallow Line Feed characters as part of the values in the credential protocol, but also disallow Carriage Return characters. In the unlikely event that a credential helper relies on Carriage Returns in the protocol, introduce an escape hatch via the `credential.protectProtocol` config setting. This addresses CVE-2024-52006. Signed-off-by: Johannes Schindelin --- Documentation/config/credential.txt | 5 +++++ credential.c | 21 ++++++++++++++------- credential.h | 4 +++- t/t0300-credentials.sh | 16 ++++++++++++++++ 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt index fd8113d6d42f75..9cadca7f73db67 100644 --- a/Documentation/config/credential.txt +++ b/Documentation/config/credential.txt @@ -20,6 +20,11 @@ credential.sanitizePrompt:: will be URL-encoded by default). Configure this setting to `false` to override that behavior. +credential.protectProtocol:: + By default, Carriage Return characters are not allowed in the protocol + that is used when Git talks to a credential helper. This setting allows + users to override this default. + credential.username:: If no username is set for a network authentication, use this username by default. See credential..* below, and diff --git a/credential.c b/credential.c index 1392a54d5c2c3e..b76a730901f5ea 100644 --- a/credential.c +++ b/credential.c @@ -69,6 +69,8 @@ static int credential_config_callback(const char *var, const char *value, c->use_http_path = git_config_bool(var, value); else if (!strcmp(key, "sanitizeprompt")) c->sanitize_prompt = git_config_bool(var, value); + else if (!strcmp(key, "protectprotocol")) + c->protect_protocol = git_config_bool(var, value); return 0; } @@ -262,7 +264,8 @@ int credential_read(struct credential *c, FILE *fp) return 0; } -static void credential_write_item(FILE *fp, const char *key, const char *value, +static void credential_write_item(const struct credential *c, + FILE *fp, const char *key, const char *value, int required) { if (!value && required) @@ -271,19 +274,23 @@ static void credential_write_item(FILE *fp, const char *key, const char *value, return; if (strchr(value, '\n')) die("credential value for %s contains newline", key); + if (c->protect_protocol && strchr(value, '\r')) + die("credential value for %s contains carriage return\n" + "If this is intended, set `credential.protectProtocol=false`", + key); fprintf(fp, "%s=%s\n", key, value); } void credential_write(const struct credential *c, FILE *fp) { - credential_write_item(fp, "protocol", c->protocol, 1); - credential_write_item(fp, "host", c->host, 1); - credential_write_item(fp, "path", c->path, 0); - credential_write_item(fp, "username", c->username, 0); - credential_write_item(fp, "password", c->password, 0); + credential_write_item(c, fp, "protocol", c->protocol, 1); + credential_write_item(c, fp, "host", c->host, 1); + credential_write_item(c, fp, "path", c->path, 0); + credential_write_item(c, fp, "username", c->username, 0); + credential_write_item(c, fp, "password", c->password, 0); if (c->password_expiry_utc != TIME_MAX) { char *s = xstrfmt("%"PRItime, c->password_expiry_utc); - credential_write_item(fp, "password_expiry_utc", s, 0); + credential_write_item(c, fp, "password_expiry_utc", s, 0); free(s); } } diff --git a/credential.h b/credential.h index 0364d436d242d6..2c0b39a925405f 100644 --- a/credential.h +++ b/credential.h @@ -120,7 +120,8 @@ struct credential { quit:1, use_http_path:1, username_from_proto:1, - sanitize_prompt:1; + sanitize_prompt:1, + protect_protocol:1; char *username; char *password; @@ -134,6 +135,7 @@ struct credential { .helpers = STRING_LIST_INIT_DUP, \ .password_expiry_utc = TIME_MAX, \ .sanitize_prompt = 1, \ + .protect_protocol = 1, \ } /* Initialize a credential structure, setting all fields to empty. */ diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index b62c70c193eff3..168ae76550155d 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -720,6 +720,22 @@ test_expect_success 'url parser rejects embedded newlines' ' test_cmp expect stderr ' +test_expect_success 'url parser rejects embedded carriage returns' ' + test_config credential.helper "!true" && + test_must_fail git credential fill 2>stderr <<-\EOF && + url=https://example%0d.com/ + EOF + cat >expect <<-\EOF && + fatal: credential value for host contains carriage return + If this is intended, set `credential.protectProtocol=false` + EOF + test_cmp expect stderr && + GIT_ASKPASS=true \ + git -c credential.protectProtocol=false credential fill <<-\EOF + url=https://example%0d.com/ + EOF +' + test_expect_success 'host-less URLs are parsed as empty host' ' check fill "verbatim foo bar" <<-\EOF url=cert:///path/to/cert.pem From c7049c2a7f47c99a67fd869f1ee89d7aa1a328d2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 18 Nov 2024 21:42:57 +0100 Subject: [PATCH 10/12] sideband: do allow ANSI color sequences by default The preceding two commits introduced special handling of the sideband channel to neutralize ANSI escape sequences before sending the payload to the terminal, and `sideband.allowControlCharacters` to override that behavior. However, some `pre-receive` hooks that are actively used in practice want to color their messages and therefore rely on the fact that Git passes them through to the terminal. In contrast to other ANSI escape sequences, it is highly unlikely that coloring sequences can be essential tools in attack vectors that mislead Git users e.g. by hiding crucial information. Therefore we can have both: Continue to allow ANSI coloring sequences to be passed to the terminal, and neutralize all other ANSI escape sequences. Signed-off-by: Johannes Schindelin --- Documentation/config/sideband.txt | 17 ++++++-- sideband.c | 61 ++++++++++++++++++++++++++--- t/t5409-colorize-remote-messages.sh | 16 +++++++- 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt index 3fb5045cd79581..f347fd6b33004a 100644 --- a/Documentation/config/sideband.txt +++ b/Documentation/config/sideband.txt @@ -1,5 +1,16 @@ sideband.allowControlCharacters:: By default, control characters that are delivered via the sideband - are masked, to prevent potentially unwanted ANSI escape sequences - from being sent to the terminal. Use this config setting to override - this behavior. + are masked, except ANSI color sequences. This prevents potentially + unwanted ANSI escape sequences from being sent to the terminal. Use + this config setting to override this behavior: ++ +-- + color:: + Allow ANSI color sequences, line feeds and horizontal tabs, + but mask all other control characters. This is the default. + false:: + Mask all control characters other than line feeds and + horizontal tabs. + true:: + Allow all control characters to be sent to the terminal. +-- diff --git a/sideband.c b/sideband.c index 8ebf1f0743e6b6..afd62aa008154b 100644 --- a/sideband.c +++ b/sideband.c @@ -20,7 +20,11 @@ static struct keyword_entry keywords[] = { { "error", GIT_COLOR_BOLD_RED }, }; -static int allow_control_characters; +static enum { + ALLOW_NO_CONTROL_CHARACTERS = 0, + ALLOW_ALL_CONTROL_CHARACTERS = 1, + ALLOW_ANSI_COLOR_SEQUENCES = 2 +} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES; /* Returns a color setting (GIT_COLOR_NEVER, etc). */ static int use_sideband_colors(void) @@ -35,8 +39,24 @@ static int use_sideband_colors(void) if (use_sideband_colors_cached >= 0) return use_sideband_colors_cached; - git_config_get_bool("sideband.allowcontrolcharacters", - &allow_control_characters); + switch (git_config_get_maybe_bool("sideband.allowcontrolcharacters", &i)) { + case 0: /* Boolean value */ + allow_control_characters = i ? ALLOW_ALL_CONTROL_CHARACTERS : + ALLOW_NO_CONTROL_CHARACTERS; + break; + case -1: /* non-Boolean value */ + if (git_config_get_string("sideband.allowcontrolcharacters", + &value)) + ; /* huh? `get_maybe_bool()` returned -1 */ + else if (!strcmp(value, "color")) + allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES; + else + warning(_("unrecognized value for `sideband." + "allowControlCharacters`: '%s'"), value); + break; + default: + break; /* not configured */ + } if (!git_config_get_string(key, &value)) { use_sideband_colors_cached = git_config_colorbool(key, value); @@ -66,9 +86,37 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref list_config_item(list, prefix, keywords[i].keyword); } +static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n) +{ + int i; + + /* + * Valid ANSI color sequences are of the form + * + * ESC [ [ [; ]*] m + */ + + if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES || + n < 3 || src[0] != '\x1b' || src[1] != '[') + return 0; + + for (i = 2; i < n; i++) { + if (src[i] == 'm') { + strbuf_add(dest, src, i + 1); + return i; + } + if (!isdigit(src[i]) && src[i] != ';') + break; + } + + return 0; +} + static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) { - if (allow_control_characters) { + int i; + + if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) { strbuf_add(dest, src, n); return; } @@ -77,7 +125,10 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n) for (; n && *src; src++, n--) { if (!iscntrl(*src) || *src == '\t' || *src == '\n') strbuf_addch(dest, *src); - else { + else if ((i = handle_ansi_color_sequence(dest, src, n))) { + src += i; + n -= i; + } else { strbuf_addch(dest, '^'); strbuf_addch(dest, 0x40 + *src); } diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index 1cd0640f200009..43296ea51c5db1 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -100,7 +100,7 @@ test_expect_success 'fallback to color.ui' ' test_expect_success 'disallow (color) control sequences in sideband' ' write_script .git/color-me-surprised <<-\EOF && - printf "error: Have you \\033[31mread\\033[m this?\\n" >&2 + printf "error: Have you \\033[31mread\\033[m this?\\a\\n" >&2 exec "$@" EOF test_config_global uploadPack.packObjectshook ./color-me-surprised && @@ -108,12 +108,24 @@ test_expect_success 'disallow (color) control sequences in sideband' ' git clone --no-local . throw-away 2>stderr && test_decode_color decoded && + test_i18ngrep RED decoded && + test_i18ngrep "\\^G" stderr && + tr -dc "\\007" actual && + test_must_be_empty actual && + + rm -rf throw-away && + git -c sideband.allowControlCharacters=false \ + clone --no-local . throw-away 2>stderr && + test_decode_color decoded && test_i18ngrep ! RED decoded && + test_i18ngrep "\\^G" stderr && rm -rf throw-away && git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr && test_decode_color decoded && - test_i18ngrep RED decoded + test_i18ngrep RED decoded && + tr -dc "\\007" actual && + test_file_not_empty actual ' test_done From 4099ea91734fe6f56c927413dd83c72131842783 Mon Sep 17 00:00:00 2001 From: Sven Strickroth Date: Tue, 6 Aug 2024 18:16:59 +0200 Subject: [PATCH 11/12] fixup! mingw: optionally enable wsl compability file mode bits Signed-off-by: Johannes Schindelin --- compat/win32/wsl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compat/win32/wsl.c b/compat/win32/wsl.c index 0d4662078aadc6..5dcef67a3fca69 100644 --- a/compat/win32/wsl.c +++ b/compat/win32/wsl.c @@ -53,6 +53,7 @@ int copy_wsl_mode_bits_from_disk(const wchar_t *wpath, ssize_t wpathlen, return ret; } +#ifndef LX_FILE_METADATA_HAS_UID #define LX_FILE_METADATA_HAS_UID 0x1 #define LX_FILE_METADATA_HAS_GID 0x2 #define LX_FILE_METADATA_HAS_MODE 0x4 @@ -77,6 +78,7 @@ typedef struct _FILE_STAT_LX_INFORMATION { uint32_t LxDeviceIdMajor; uint32_t LxDeviceIdMinor; } FILE_STAT_LX_INFORMATION, *PFILE_STAT_LX_INFORMATION; +#endif /* * This struct is extended from the original FILE_FULL_EA_INFORMATION of From ff405069c4b7159370bc648456221067fb1f5d93 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 30 Oct 2024 19:48:46 +0100 Subject: [PATCH 12/12] unix-socket: avoid leak when initialization fails When a Unix socket is initialized, the current directory's path is stored so that the cleanup code can `chdir()` back to where it was before exit. If the path that needs to be stored exceeds the default size of the `sun_path` attribute of `struct sockaddr_un` (which is defined as a 108-sized byte array on Linux), a larger buffer needs to be allocated so that it can hold the path, and it is the responsibility of the `unix_sockaddr_cleanup()` function to release that allocated memory. In Git's CI, this stack allocation is not necessary because the code is checked out to `/home/runner/work/git/git`. Concatenate the path `t/trash directory.t0301-credential-cache/.cache/git/credential/socket` and a terminating NUL, and you end up with 96 bytes, 12 shy of the default `sun_path` size. However, I use worktrees with slightly longer paths: `/home/me/projects/git/yes/i/nest/worktrees/to/organize/them/` is more in line with what I have. When I recently tried to locally reproduce a failure of the `linux-leaks` CI job, this t0301 test failed (where it had not failed in CI). The reason: When `credential-cache` tries to reach its daemon initially by calling `unix_sockaddr_init()`, it is expected that the daemon cannot be reached (the idea is to spin up the daemon in that case and try again). However, when this first call to `unix_sockaddr_init()` fails, the code returns early from the `unix_stream_connect()` function _without_ giving the cleanup code a chance to run, skipping the deallocation of above-mentioned path. The fix is easy: do not return early but instead go directly to the cleanup code. Signed-off-by: Johannes Schindelin --- unix-socket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unix-socket.c b/unix-socket.c index 79800d80636fc5..eb45bfa56e4852 100644 --- a/unix-socket.c +++ b/unix-socket.c @@ -80,7 +80,7 @@ int unix_stream_connect(const char *path, int disallow_chdir) struct unix_sockaddr_context ctx; if (unix_sockaddr_init(&sa, path, &ctx, disallow_chdir) < 0) - return -1; + goto fail; fd = socket(AF_UNIX, SOCK_STREAM, 0); if (fd < 0) goto fail;