worktree: Fix out of bounds read that causes data loss and reject invalid empty input in worktree add
cover worktree: Fix out of bounds read that causes data loss and reject invalid empty input in worktree add
Passing an empty string to git worktree add (typically via an unset variable, e.g. git worktree add "$UNSET_VAR" -b tb origin/main) can result in BUG: How come '' becomes empty after sanitization? but it can also have worse consequences: recursively deleting the current working directory, including .git. The inconsistent behaviour is caused by worktree_basename reading unrelated bytes from the memory before path and passing that back to add_worktree, which can circumvent the check for the BUG call.
Matthias Aßhauer (2):
worktree: don't read out of bounds
worktree: reject empty string
builtin/worktree.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2187%2Frimrul%2Fworktree-fix-oob-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2187/rimrul/worktree-fix-oob-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2187
Signature · 1 line
gitgitgadget
PATCH 1/2 worktree: don't read out of bounds
From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
worktree_basename tries to read from memory before the passed path string, if path is empty (or only consists of directory separators). That results in unexpected nonsense data being returned to the caller, which can lead to issues, such as git worktree add "" recursively deleting the current working directory, including .git.
Stop reading out of bounds in these cases to avoid that behaviour.
This leads to git worktree add "" consistently exiting with the message BUG: How come '' becomes empty after sanitization?, which is still undesirable, but at least it doesn't result in data loss anymore.
This fixes https://github.com/git-for-windows/git/issues/6346
--- builtin/worktree.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
builtin/worktree.c +11 -7
@@ -297,17 +297,21 @@ static void remove_junk_on_signal(int signo)static const char *worktree_basename (const char *path,int *olen) {const char *name; -int len; +int len, len2; - len =strlen (path); + len2 = len =strlen (path);while (len &&is_dir_sep (path[len -1 ])) len--; -for (name = path + len -1 ; name > path; name--) -if (is_dir_sep (*name)) { - name++; -break ; - } +if (len) { +for (name = path + len -1 ; name > path; name--) +if (is_dir_sep (*name)) { + name++; +break ; + } + } +else + name = path + len2; *olen = len;return name;
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] worktree: don't read out of bounds
Quoted 5 lines · Matthias Aßhauer via GitGitGadget
worktree_basenametries to read from memory before the passedpathstring, ifpathis empty (or only consists of directory separators). That results in unexpected nonsense data being returned to the caller, which can lead to issues, such asgit worktree add ""recursively deleting the current working directory, including.git.
OK, so you do want to handle a case where path is something silly like "///".
Quoted 5 lines
Stop reading out of bounds in these cases to avoid that behaviour.
This leads to
git worktree add ""consistently exiting with the messageBUG: How come '' becomes empty after sanitization?, which is still undesirable, but at least it doesn't result in data loss anymore.
OK.
Quoted 5 lines
builtin/worktree.c +0
-0
builtin/worktree.c static void remove_junk_on_signal(int signo) · line 304
Quoted 10 lines
builtin/worktree.c +2
-2@@ -297,17 +297,21 @@ static void remove_junk_on_signal(int signo)static const char *worktree_basename (const char *path,int *olen) {const char *name; -int len; +int len, len2; - len =strlen (path); + len2 = len =strlen (path);while (len &&is_dir_sep (path[len -1 ])) len--;
These two 'len' variables should have clear names to distinguish what each length represents. Rather than introducing a cryptic 'len2', give it a more meaningful name, and rename 'len' as well if necessary.
I suspect that it is to remember the original length of the 'path' before stripping the trailing directory separators?
builtin/worktree.c static void remove_junk_on_signal(int signo) · line 310
Quoted 5 lines
builtin/worktree.c +0
-5-for (name = path + len -1 ; name > path; name--) -if (is_dir_sep (*name)) { - name++; -break ; - }
When 'len' is 0, the original code sets 'name' to '&path[-1]' and does not enter the loop. However, '*olen' is set to 0, and 'name', pointing before the start of the string, is returned. If left unfixed, callers pass it to xstrndup(), strbuf_add(), and the like, reading memory before the start of the string, which is horrible and worth fixing.
builtin/worktree.c static void remove_junk_on_signal(int signo) · line 314
Quoted 9 lines
builtin/worktree.c +9
-0+if (len) { +for (name = path + len -1 ; name > path; name--) +if (is_dir_sep (*name)) { + name++; +break ; + } + } +else + name = path + len2;
Style:
(1) Missing SP between 'if' and '(len'.
(2) 'else' sits on the same line as '}' that closes the 'if' clause.
(3) When any one branch of an 'if'...'else if'...'else' cascade needs a pair of braces to group multiple statements, all other branches must use braces as well.
Taken together:
if (len) {
...
} else {
...
}As for what the patch intends to do, setting 'name = path + len2' when 'len' is 0 breaks when 'path' consists only of directory separators (for example, "/" or "///"), no?
In that case, 'len2' is positive (for example, 3) while 'len' is 0. In add_worktree(), 'path + len - name' evaluates to (path + 0) - (path + 3) = -3. Passed as size_t to strbuf_add(), this wraps around to SIZE_MAX - 2 (approx. 18 exabytes), leading to a buffer allocation failure or a crash.
Rather than calculating 'path - 1' out of bounds or introducing 'len2', worktree_basename() can simply keep 'name = path' when 'len' is 0. Using an integer index loop 'for (int i = len - 1; 0 <= i; i--)' avoids pointer arithmetic before the start of the buffer entirely, I would think. Or am I missing something?
Thanks.
Re: [PATCH 1/2] worktree: don't read out of bounds
Quoted 95 lines · Junio C Hamano
Quoted 5 lines · Matthias Aßhauer via GitGitGadget
worktree_basenametries to read from memory before the passedpathstring, ifpathis empty (or only consists of directory separators). That results in unexpected nonsense data being returned to the caller, which can lead to issues, such asgit worktree add ""recursively deleting the current working directory, including.git.OK, so you do want to handle a case where path is something silly like "///".
Quoted 5 lines
Stop reading out of bounds in these cases to avoid that behaviour.
This leads to
git worktree add ""consistently exiting with the messageBUG: How come '' becomes empty after sanitization?, which is still undesirable, but at least it doesn't result in data loss anymore.OK.
Quoted 5 lines
builtin/worktree.c +0
-0builtin/worktree.c static void remove_junk_on_signal(int signo) · line 304
Quoted 10 lines
builtin/worktree.c +2
-2@@ -297,17 +297,21 @@ static void remove_junk_on_signal(int signo)static const char *worktree_basename (const char *path,int *olen) {const char *name; -int len; +int len, len2; - len =strlen (path); + len2 = len =strlen (path);while (len &&is_dir_sep (path[len -1 ])) len--;These two 'len' variables should have clear names to distinguish what each length represents. Rather than introducing a cryptic 'len2', give it a more meaningful name, and rename 'len' as well if necessary.
I suspect that it is to remember the original length of the 'path' before stripping the trailing directory separators?
builtin/worktree.c static void remove_junk_on_signal(int signo) · line 310
Quoted 5 lines
builtin/worktree.c +0
-5-for (name = path + len -1 ; name > path; name--) -if (is_dir_sep (*name)) { - name++; -break ; - }When 'len' is 0, the original code sets 'name' to '&path[-1]' and does not enter the loop. However, '*olen' is set to 0, and 'name', pointing before the start of the string, is returned. If left unfixed, callers pass it to
xstrndup(),strbuf_add(), and the like, reading memory before the start of the string, which is horrible and worth fixing.builtin/worktree.c static void remove_junk_on_signal(int signo) · line 314
Quoted 9 lines
builtin/worktree.c +9
-0+if (len) { +for (name = path + len -1 ; name > path; name--) +if (is_dir_sep (*name)) { + name++; +break ; + } + } +else + name = path + len2;Style:
(1) Missing SP between 'if' and '(len'.
(2) 'else' sits on the same line as '}' that closes the 'if' clause.
(3) When any one branch of an 'if'...'else if'...'else' cascade needs a pair of braces to group multiple statements, all other branches must use braces as well.
Taken together:
if (len) { ... } else { ... }As for what the patch intends to do, setting 'name = path + len2' when 'len' is 0 breaks when 'path' consists only of directory separators (for example, "/" or "///"), no?
In that case, 'len2' is positive (for example, 3) while 'len' is 0. In
add_worktree(), 'path + len - name' evaluates to (path + 0) - (path + 3) = -3. Passed assize_ttostrbuf_add(), this wraps around toSIZE_MAX- 2 (approx. 18 exabytes), leading to a buffer allocation failure or a crash.
Quoted 5 lines
Rather than calculating 'path - 1' out of bounds or introducing 'len2',
worktree_basename()can simply keep 'name = path' when 'len' is 0. Using an integer index loop 'for (int i = len - 1; 0 <= i; i--)' avoids pointer arithmetic before the start of the buffer entirely, I would think. Or am I missing something?
Interesting. This function has two types of callers. add_worktree() does:
name = worktree_basename(path, &len); strbuf_add(&sb, name, path + len - name);
While dwim_branch() and add() do basically:
name = worktree_basename(path, &len); copy = xstrndup(name, len);
If path is empty or all separators, len is 0 and name invalid, as noted. add_worktree() breaks, the other callers are mostly fine because xstrndup() calls memchr(3) and memcpy(3) with a size of 0 internally and thus doesn't dereference the pointer in practice. So patch 2 should suffice to prevent the out of bounds read.
If path contains one component ("foo/"), add_worktree() adds "foo" to the strbuf and the others similarly duplicate "foo". OK.
If path contains more components ("foo/bar/"), add_worktree() adds "bar", while the others duplicate "bar/", because len is the length of path without any trailing separator (7 in this example).
xstrndup("bar/", 7) could read out of bounds because it's calling memchr(3) internally, which doesn't have to stop at the found byte. But more practically, do we really want to keep trailing separators?
worktree_basename() would be harder to misuse if it gave the length of the basename instead. Then add_worktree() wouldn't have to do any pointer arithmetic and the others wouldn't risk reading out of bounds.
René
PATCH 2/2 worktree: reject empty string
In reply to Matthias Aßhauer via GitGitGadgetFrom: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
git worktree add "" errors out with the message BUG: How come '' becomes empty after sanitization?, but not due to a bug in the sanitization code. An empty string should remain empty during sanitization. Instead reject the argument as invalid user input, if it's already empty before sanitization.
--- builtin/worktree.c | 2 ++ 1 file changed, 2 insertions(+)
builtin/worktree.c +2 -0
@@ -496,6 +496,8 @@ static int add_worktree(const char *path, const char *refname,die (_ ("invalid reference: %s" ), refname); name =worktree_basename (path, &len); +if (!len) +die (_ ("the empty string is not a valid worktree" ));strbuf_add (&sb, name, path + len - name);sanitize_refname_component (sb.buf, &sb_name);if (!sb_name.len)
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] worktree: reject empty string
Quoted 18 lines · Matthias AÃhauer via GitGitGadget
From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
git worktree add ""errors out with the messageBUG: How come '' becomes empty after sanitization?, but not due to a bug in the sanitization code. An empty string should remain empty during sanitization. Instead reject the argument as invalid user input, if it's already empty before sanitization.Signed-off-by Matthias Aßhauer <[email protected]>--- builtin/worktree.c | 2 ++ 1 file changed, 2 insertions(+)builtin/worktree.c +0
-0
builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 503
Quoted 8 lines
builtin/worktree.c +2
-0@@ -496,6 +496,8 @@ static int add_worktree(const char *path, const char *refname,die (_ ("invalid reference: %s" ), refname); name =worktree_basename (path, &len); +if (!len) +die (_ ("the empty string is not a valid worktree" ));strbuf_add (&sb, name, path + len - name);sanitize_refname_component (sb.buf, &sb_name);if (!sb_name.len)
Hmm, on my machine, with or without this patch:
$ git worktree add "" Preparing worktree (new branch '') fatal: '' is not a valid branch name hint: See 'git help check-ref-format' hint: Disable this message with "git config set advice.refSyntax false"
and
$ git worktree add / Preparing worktree (new branch '') fatal: '' is not a valid branch name hint: See 'git help check-ref-format' hint: Disable this message with "git config set advice.refSyntax false"
This error message is produced by the command 'git branch "" HEAD' issued using run_command() in add(), just before the the add_worktree() call, which is then skipped.
René
Re: [PATCH 2/2] worktree: reject empty string
Quoted 30 lines · René Scharfe
Quoted 18 lines · Matthias AÃhauer via GitGitGadget
From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
git worktree add ""errors out with the messageBUG: How come '' becomes empty after sanitization?, but not due to a bug in the sanitization code. An empty string should remain empty during sanitization. Instead reject the argument as invalid user input, if it's already empty before sanitization.Signed-off-by Matthias Aßhauer <[email protected]>--- builtin/worktree.c | 2 ++ 1 file changed, 2 insertions(+)builtin/worktree.c +0
-0builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 503
Quoted 8 lines
builtin/worktree.c +2
-0@@ -496,6 +496,8 @@ static int add_worktree(const char *path, const char *refname,die (_ ("invalid reference: %s" ), refname); name =worktree_basename (path, &len); +if (!len) +die (_ ("the empty string is not a valid worktree" ));strbuf_add (&sb, name, path + len - name);sanitize_refname_component (sb.buf, &sb_name);if (!sb_name.len)Hmm, on my machine, with or without this patch:
Quoted 5 lines
$ git worktree add "" Preparing worktree (new branch '') fatal: '' is not a valid branch name hint: See 'git help check-ref-format' hint: Disable this message with "git config set advice.refSyntax false"
This hits the BUG by passing the empty string directly to add_worktree():
$ git worktree add "" HEAD Preparing worktree (detached HEAD a97fcc37c2) BUG: builtin/worktree.c:498: How come '' becomes empty after sanitization?
René
PATCH 1/1 worktree: Fix out of bounds read that causes data loss and reject invalid empty input in worktree add
In reply to Matthias Aßhauer via GitGitGadgetFrom: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
worktree_basename tries to read from memory before the passed path string, if path is empty (or only consists of directory separators). That results in unexpected nonsense data being returned to the caller, which can lead to issues, such as git worktree add "" recursively deleting the current working directory, including .git.
Stop reading out of bounds in these cases to avoid that behaviour.
This leads to git worktree add "" consistently exiting with the message BUG: How come '' becomes empty after sanitization?, which is still undesirable, but at least it doesn't result in data loss anymore.
This fixes https://github.com/git-for-windows/git/issues/6346
---
How about this while we're waiting for a reroll? It implements what the commit message says, nothing more. Follows the style of the first loop.
builtin/worktree.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
builtin/worktree.c +3 -5
@@ -303,11 +303,9 @@ static const char *worktree_basename(const char *path, int *olen)while (len &&is_dir_sep (path[len -1 ])) len--; -for (name = path + len -1 ; name > path; name--) -if (is_dir_sep (*name)) { - name++; -break ; - } + name = path + len; +while (name > path && !is_dir_sep (name[-1 ])) + name--; *olen = len;return name;
Signature · 1 line
2.55.0
Quoted 14 lines · René Scharfe
From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
worktree_basenametries to read from memory before the passedpathstring, ifpathis empty (or only consists of directory separators). That results in unexpected nonsense data being returned to the caller, which can lead to issues, such asgit worktree add ""recursively deleting the current working directory, including.git.Stop reading out of bounds in these cases to avoid that behaviour.
This leads to
git worktree add ""consistently exiting with the messageBUG: How come '' becomes empty after sanitization?, which is still undesirable, but at least it doesn't result in data loss anymore.
Quoted 6 lines
This fixes https://github.com/git-for-windows/git/issues/6346
Signed-off-by René Scharfe <[email protected]>---
How about this while we're waiting for a reroll? It implements what the commit message says, nothing more. Follows the style of the first loop.
This one I think is obvious and clear. Why not take the authorship too so that we do not have to worry about DCO?
Quoted 23 lines
builtin/worktree.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)builtin/worktree.c +3
-5@@ -303,11 +303,9 @@ static const char *worktree_basename(const char *path, int *olen)while (len &&is_dir_sep (path[len -1 ])) len--; -for (name = path + len -1 ; name > path; name--) -if (is_dir_sep (*name)) { - name++; -break ; - } + name = path + len; +while (name > path && !is_dir_sep (name[-1 ])) + name--; *olen = len;return name;
Quoted 18 lines · Junio C Hamano
Quoted 16 lines · René Scharfe
From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
worktree_basenametries to read from memory before the passedpathstring, ifpathis empty (or only consists of directory separators). That results in unexpected nonsense data being returned to the caller, which can lead to issues, such asgit worktree add ""recursively deleting the current working directory, including.git.Stop reading out of bounds in these cases to avoid that behaviour.
This leads to
git worktree add ""consistently exiting with the messageBUG: How come '' becomes empty after sanitization?, which is still undesirable, but at least it doesn't result in data loss anymore.This fixes https://github.com/git-for-windows/git/issues/6346
Quoted 7 lines
Quoted 4 lines
Signed-off-by René Scharfe <[email protected]>---
How about this while we're waiting for a reroll? It implements what the commit message says, nothing more. Follows the style of the first loop.This one I think is obvious and clear. Why not take the authorship too so that we do not have to worry about DCO?
That feels unfair: Matthias did most of the work by identifying the bug and removing the premature subtraction from the loop doesn't seem very original to me. Ultimately my main concern is getting this surprisingly impactful bug fixed in a reasonable amount of time, though..
René
Quoted 24 lines
Quoted 24 lines
builtin/worktree.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)builtin/worktree.c +3
-5@@ -303,11 +303,9 @@ static const char *worktree_basename(const char *path, int *olen)while (len &&is_dir_sep (path[len -1 ])) len--; -for (name = path + len -1 ; name > path; name--) -if (is_dir_sep (*name)) { - name++; -break ; - } + name = path + len; +while (name > path && !is_dir_sep (name[-1 ])) + name--; *olen = len;return name;
Quoted 27 lines · René Scharfe
Quoted 18 lines · Junio C Hamano
Quoted 16 lines · René Scharfe
From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
worktree_basenametries to read from memory before the passedpathstring, ifpathis empty (or only consists of directory separators). That results in unexpected nonsense data being returned to the caller, which can lead to issues, such asgit worktree add ""recursively deleting the current working directory, including.git.Stop reading out of bounds in these cases to avoid that behaviour.
This leads to
git worktree add ""consistently exiting with the messageBUG: How come '' becomes empty after sanitization?, which is still undesirable, but at least it doesn't result in data loss anymore.This fixes https://github.com/git-for-windows/git/issues/6346
Quoted 7 lines
Quoted 4 lines
Signed-off-by René Scharfe <[email protected]>---
How about this while we're waiting for a reroll? It implements what the commit message says, nothing more. Follows the style of the first loop.This one I think is obvious and clear. Why not take the authorship too so that we do not have to worry about DCO?
Quoted 6 lines
That feels unfair: Matthias did most of the work by identifying the bug and removing the premature subtraction from the loop doesn't seem very original to me. Ultimately my main concern is getting this surprisingly impactful bug fixed in a reasonable amount of time, though..
René
If we can get Matthias sign this patch off, that would work for me, too.