[PATCH 0/2] worktree: Fix out of bounds read that causes data loss and reject invalid empty input in worktree add

[PATCH 0/2] worktree: Fix out of bounds read that causes data loss and reject invalid empty input in worktree add

[Matthias Aßhauer via GitGitGadget]

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
-- 
gitgitgadget

[PATCH 1/2] worktree: don't read out of bounds

[Matthias Aßhauer via GitGitGadget]

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>

`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

Signed-off-by: Matthias Aßhauer <mha1993@live.de>
---
 builtin/worktree.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4bc7b4f6e7..d8188035db 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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;
-- 
gitgitgadget

Re: [PATCH 1/2] worktree: don't read out of bounds

[Junio C Hamano]

"Matthias Aßhauer via GitGitGadget" <gitgitgadget@gmail.com> writes:

> `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`.

OK, so you do want to handle a case where path is something silly
like "///".

> 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.

OK.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 4bc7b4f6e7..d8188035db 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -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?

> -	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.

> +	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.

[PATCH 2/2] worktree: reject empty string

[Matthias Aßhauer via GitGitGadget]

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>

`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.

Signed-off-by: Matthias Aßhauer <mha1993@live.de>
---
 builtin/worktree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d8188035db..113dbf98d3 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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)
-- 
gitgitgadget