threads / patch / 27485

patch, 3 partsdo not write files outside of work-dir

Subject: [PATCH maint 0/3] do not write files outside of work-dir

## tl;dr

20 messages between May 27, 2011 and Jun 7, 2011. Diffs are folded; open one to read it.

replies: 19people: 6as markdown or json

Erik Faye-Lund· May 27, 2011, 16:00 UTC · lore

Theo Niessink has uncovered a serious sercurity issue in Git for Windows, where cloning an evil repository can arbitrarily overwrite files outside the repository. Since many Windows users run as administrators, this can be used for very nasty purposes.

The first two patches fix "git add" so it reject paths outside of the repository when specified in the "C:\..."-form on Windows.

Patch 3/3 makes sure we don't try to actually write to these files.

This series applies cleanly to 'maint', and I strongly encourage that we apply at the very least 3/3 there.

Erik Faye-Lund (1):
  verify_path: consider dos drive prefix
Theo Niessink (2):
  A Windows path starting with a backslash is absolute
  real_path: do not assume '/' is the path seperator
 abspath.c         |    4 ++--
 cache.h           |    2 +-
 compat/mingw.h    |    9 +++++++++
 git-compat-util.h |    4 ++++
 read-cache.c      |    5 ++++-
 5 files changed, 20 insertions(+), 4 deletions(-)
-- 
1.7.5.3.3.g435ff
Erik Faye-Lund· May 27, 2011, 16:00 UTC · re: Erik Faye-Lund · lore

[PATCH 1/3] A Windows path starting with a backslash is absolute

From: Theo Niessink <theo@taletn.com>

This fixes prefix_path() not recognizing e.g. \foo\bar as an absolute path on Windows.

Signed-off-by: Theo Niessink <theo@taletn.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 cache.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to cache.h +1 −1
diff --git a/cache.h b/cache.h
index dd34fed..555bf7f 100644
--- a/cache.h
+++ b/cache.h
@@ -734,7 +734,7 @@ extern char *expand_user_path(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
-	return path[0] == '/' || has_dos_drive_prefix(path);
+	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
 }
 int is_directory(const char *);
 const char *real_path(const char *path);

-- 
1.7.5.3.3.g435ff
Erik Faye-Lund· May 27, 2011, 16:00 UTC · re: Erik Faye-Lund · lore

[PATCH 2/3] real_path: do not assume '/' is the path seperator

From: Theo Niessink <theo@taletn.com>

real_path currently assumes it's input had '/' as path seperator. This assumption does not hold true for the code-path from prefix_path (on Windows), where real_path can be called before normalize_path_copy.

Fix real_path so it doesn't make this assumption. Create a helper function to reverse-search for the last path-seperator in a string.

Signed-off-by: Theo Niessink <theo@taletn.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 abspath.c         |    4 ++--
 compat/mingw.h    |    9 +++++++++
 git-compat-util.h |    4 ++++
 3 files changed, 15 insertions(+), 2 deletions(-)
Show changes to 3 files +15 −2

abspath.c, compat/mingw.h, git-compat-util.h

diff --git a/abspath.c b/abspath.c
index 3005aed..01858eb 100644
--- a/abspath.c
+++ b/abspath.c
@@ -40,7 +40,7 @@ const char *real_path(const char *path)
 
 	while (depth--) {
 		if (!is_directory(buf)) {
-			char *last_slash = strrchr(buf, '/');
+			char *last_slash = find_last_dir_sep(buf);
 			if (last_slash) {
 				*last_slash = '\0';
 				last_elem = xstrdup(last_slash + 1);
@@ -65,7 +65,7 @@ const char *real_path(const char *path)
 			if (len + strlen(last_elem) + 2 > PATH_MAX)
 				die ("Too long path name: '%s/%s'",
 						buf, last_elem);
-			if (len && buf[len-1] != '/')
+			if (len && !is_dir_sep(buf[len-1]))
 				buf[len++] = '/';
 			strcpy(buf + len, last_elem);
 			free(last_elem);
diff --git a/compat/mingw.h b/compat/mingw.h
index 62eccd3..b188776 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -297,6 +297,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format
 
 #define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
+static inline char *mingw_find_last_dir_sep(const char *path)
+{
+	char *ret = NULL;
+	for (; *path; ++path)
+		if (is_dir_sep(*path))
+			ret = (char *)path;
+	return ret;
+}
+#define find_last_dir_sep mingw_find_last_dir_sep
 #define PATH_SEP ';'
 #define PRIuMAX "I64u"
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 40498b3..08d58f1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -215,6 +215,10 @@ extern char *gitbasename(char *);
 #define is_dir_sep(c) ((c) == '/')
 #endif
 
+#ifndef find_last_dir_sep
+#define find_last_dir_sep(path) strrchr(path, '/')
+#endif
+
 #if __HP_cc >= 61000
 #define NORETURN __attribute__((noreturn))
 #define NORETURN_PTR

-- 
1.7.5.3.3.g435ff
Erik Faye-Lund· May 27, 2011, 16:00 UTC · re: Erik Faye-Lund · lore

[PATCH 3/3] verify_path: consider dos drive prefix

If someone manage to create a repo with a 'C:' entry in the root-tree, files can be written outside of the working-dir. This opens up a can-of-worms of exploits.

Fix it by explicitly checking for a dos drive prefix when verifying a paht. While we're at it, make sure that paths beginning with '\' is considered absolute as well.

Noticed-by: Theo Niessink <theo@taletn.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 read-cache.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to read-cache.c +4 −1
diff --git a/read-cache.c b/read-cache.c
index f38471c..68faa51 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -753,11 +753,14 @@ int verify_path(const char *path)
 {
 	char c;
 
+	if (has_dos_drive_prefix(path))
+		return 0;
+
 	goto inside;
 	for (;;) {
 		if (!c)
 			return 1;
-		if (c == '/') {
+		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
 			switch (c) {

-- 
1.7.5.3.3.g435ff
Johannes Sixt· May 27, 2011, 18:58 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

Am 27.05.2011 18:00, schrieb Erik Faye-Lund:
Show 7 quoted lines
> If someone manage to create a repo with a 'C:' entry in the
> root-tree, files can be written outside of the working-dir. This
> opens up a can-of-worms of exploits.
> 
> Fix it by explicitly checking for a dos drive prefix when verifying
> a paht. While we're at it, make sure that paths beginning with '\' is
> considered absolute as well.

I think we do agree that the only way to avoid the security breach is to check a path before it is used to write a file. In practice, it means to disallow paths in the top-most level of the index that are two characters long and are letter-colon.

IMHO, it is pointless to avoid that an evil path enters the repository, because there are so many and a few more ways to create an evil repository.

Show 11 quoted lines
> diff --git a/read-cache.c b/read-cache.c
> index f38471c..68faa51 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -753,11 +753,14 @@ int verify_path(const char *path)
>  {
>  	char c;
>  
> +	if (has_dos_drive_prefix(path))
> +		return 0;
> +

Isn't verify_path used to avoid that a bogus path enters the index? (I don't know, I'm not familiar with this infrastructure.)

Show 7 quoted lines
>  	goto inside;
>  	for (;;) {
>  		if (!c)
>  			return 1;
> -		if (c == '/') {
> +		if (is_dir_sep(c)) {
>  inside:

And if so, at this point, all backslashes should have been converted to forward-slashes already. If not, then this would just paper over the real bug.

>  			c = *path++;
>  			switch (c) {
-- Hannes
Erik Faye-Lund· May 30, 2011, 09:32 UTC · re: Johannes Sixt · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

On Fri, May 27, 2011 at 8:58 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 17 quoted lines
> Am 27.05.2011 18:00, schrieb Erik Faye-Lund:
>> If someone manage to create a repo with a 'C:' entry in the
>> root-tree, files can be written outside of the working-dir. This
>> opens up a can-of-worms of exploits.
>>
>> Fix it by explicitly checking for a dos drive prefix when verifying
>> a paht. While we're at it, make sure that paths beginning with '\' is
>> considered absolute as well.
>
> I think we do agree that the only way to avoid the security breach is to
> check a path before it is used to write a file. In practice, it means to
> disallow paths in the top-most level of the index that are two
> characters long and are letter-colon.
>
> IMHO, it is pointless to avoid that an evil path enters the repository,
> because there are so many and a few more ways to create an evil repository.
>

Yes, but this patch doesn't prevent that; it prevents an evil path from entering the index and from being checked out if the index is evil.

Show 15 quoted lines
>> diff --git a/read-cache.c b/read-cache.c
>> index f38471c..68faa51 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -753,11 +753,14 @@ int verify_path(const char *path)
>>  {
>>       char c;
>>
>> +     if (has_dos_drive_prefix(path))
>> +             return 0;
>> +
>
> Isn't verify_path used to avoid that a bogus path enters the index? (I
> don't know, I'm not familiar with this infrastructure.)
>

Yes, it's being used to do that. But it's also being used when reading the index into memory, which is "the good stuf" for our purposes.

This is the same guard which makes Git on Linux bard on an index containing paths like "/tmp/foo"

Show 11 quoted lines
>>       goto inside;
>>       for (;;) {
>>               if (!c)
>>                       return 1;
>> -             if (c == '/') {
>> +             if (is_dir_sep(c)) {
>>  inside:
>
> And if so, at this point, all backslashes should have been converted to
> forward-slashes already. If not, then this would just paper over the
> real bug.

SHOULD, yes. But we could have an evil tree/index which doesn't, and this if intended to make sure we reject such paths.

So I don't see how this is papering over the bug; this IS the bug (as far as I can tell).

But I think I might have been a bit too care-less; I didn't fix the switch-case to check for multiple backslashes on Windows. It's not immediately obvious if this is needed or not, but I don't think it can cause harm; we should never have created an index like that anyway.

So something like this on top, perhaps?
Show changes to read-cache.c +3 −7
diff --git a/read-cache.c b/read-cache.c
index 68faa51..9367349 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -763,15 +763,11 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
+			if (c == '.') {
 				if (verify_dotfile(path))
 					continue;
-			}
+			} else if (!is_dir_sep(c) && c != '\0')
+				continue;
 			return 0;
 		}
 		c = *path++;
Theo Niessink· May 30, 2011, 10:58 UTC · re: Erik Faye-Lund · lore

RE: [PATCH 3/3] verify_path: consider dos drive prefix

Erik Faye-Lund wrote:
Show 6 quoted lines
> But I think I might have been a bit too care-less; I didn't fix the
> switch-case to check for multiple backslashes on Windows. It's not
> immediately obvious if this is needed or not, but I don't think it can
> cause harm; we should never have created an index like that anyway.
> 
> So something like this on top, perhaps?
Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
continue? It will check for '\0' and is_dir_sep(c) again, but you already
know that both ifs will be false. So you could just as easy jump straight to
c = *path++, which IMHO also makes the code easier to follow:
Show changes to read-cache.c +5 −7
diff --git a/read-cache.c b/read-cache.c
index 68faa51..089cd3e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -763,17 +763,15 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
+			if (c == '.') {
+				
 				if (verify_dotfile(path))
 					continue;
-			}
+			} else if (!is_dir_sep(c) && c != '\0')
+				goto next;
 			return 0;
 		}
+next:
 		c = *path++;
 	}
 }
Erik Faye-Lund· May 30, 2011, 11:17 UTC · re: Theo Niessink · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

On Mon, May 30, 2011 at 12:58 PM, Theo Niessink <theo@taletn.com> wrote:
Show 12 quoted lines
> Erik Faye-Lund wrote:
>> But I think I might have been a bit too care-less; I didn't fix the
>> switch-case to check for multiple backslashes on Windows. It's not
>> immediately obvious if this is needed or not, but I don't think it can
>> cause harm; we should never have created an index like that anyway.
>>
>> So something like this on top, perhaps?
>
> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
> continue? It will check for '\0' and is_dir_sep(c) again, but you already
> know that both ifs will be false. So you could just as easy jump straight to
> c = *path++, which IMHO also makes the code easier to follow:

Very good point, thanks for noticing. I just rewrote the logic from switch/case to if/else, but with the rewrite these redundant compares became more obvious. I think your version is better, indeed.

Junio C Hamano· Jun 7, 2011, 03:46 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 8 quoted lines
>> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
>> continue? It will check for '\0' and is_dir_sep(c) again, but you already
>> know that both ifs will be false. So you could just as easy jump straight to
>> c = *path++, which IMHO also makes the code easier to follow:
>
> Very good point, thanks for noticing. I just rewrote the logic from
> switch/case to if/else, but with the rewrite these redundant compares
> became more obvious. I think your version is better, indeed.

Let's not add an unnecessary goto while at it. How about this on top instead?

 read-cache.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)
Show changes to read-cache.c +3 −10
diff --git a/read-cache.c b/read-cache.c
index 31cf0b5..3593291 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -784,16 +784,9 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
-				if (verify_dotfile(path))
-					continue;
-			}
-			return 0;
+			if ((c == '.' && !verify_dotfile(path)) ||
+			    is_dir_sep(c) || c == '\0')
+				return 0;
 		}
 		c = *path++;
 	}
Erik Faye-Lund· Jun 7, 2011, 10:07 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

On Tue, Jun 7, 2011 at 5:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 41 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
>>> continue? It will check for '\0' and is_dir_sep(c) again, but you already
>>> know that both ifs will be false. So you could just as easy jump straight to
>>> c = *path++, which IMHO also makes the code easier to follow:
>>
>> Very good point, thanks for noticing. I just rewrote the logic from
>> switch/case to if/else, but with the rewrite these redundant compares
>> became more obvious. I think your version is better, indeed.
>
> Let's not add an unnecessary goto while at it.  How about this on top
> instead?
>
>  read-cache.c |   13 +++----------
>  1 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 31cf0b5..3593291 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -784,16 +784,9 @@ int verify_path(const char *path)
>                if (is_dir_sep(c)) {
>  inside:
>                        c = *path++;
> -                       switch (c) {
> -                       default:
> -                               continue;
> -                       case '/': case '\0':
> -                               break;
> -                       case '.':
> -                               if (verify_dotfile(path))
> -                                       continue;
> -                       }
> -                       return 0;
> +                       if ((c == '.' && !verify_dotfile(path)) ||
> +                           is_dir_sep(c) || c == '\0')
> +                               return 0;
>                }
>                c = *path++;
>        }

This change the "c == '.' && verify_dotfile(path)"-case to eat the '.' character without testing it against is_dir_sep, which is exactly what we want. The other cases return 0, as they used to. Good.

Indeed, this is a cleaner approach. Thanks!
Erik Faye-Lund· Jun 7, 2011, 19:09 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

On Tue, Jun 7, 2011 at 12:07 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 49 quoted lines
> On Tue, Jun 7, 2011 at 5:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>>> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
>>>> continue? It will check for '\0' and is_dir_sep(c) again, but you already
>>>> know that both ifs will be false. So you could just as easy jump straight to
>>>> c = *path++, which IMHO also makes the code easier to follow:
>>>
>>> Very good point, thanks for noticing. I just rewrote the logic from
>>> switch/case to if/else, but with the rewrite these redundant compares
>>> became more obvious. I think your version is better, indeed.
>>
>> Let's not add an unnecessary goto while at it.  How about this on top
>> instead?
>>
>>  read-cache.c |   13 +++----------
>>  1 files changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index 31cf0b5..3593291 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -784,16 +784,9 @@ int verify_path(const char *path)
>>                if (is_dir_sep(c)) {
>>  inside:
>>                        c = *path++;
>> -                       switch (c) {
>> -                       default:
>> -                               continue;
>> -                       case '/': case '\0':
>> -                               break;
>> -                       case '.':
>> -                               if (verify_dotfile(path))
>> -                                       continue;
>> -                       }
>> -                       return 0;
>> +                       if ((c == '.' && !verify_dotfile(path)) ||
>> +                           is_dir_sep(c) || c == '\0')
>> +                               return 0;
>>                }
>>                c = *path++;
>>        }
>
> This change the "c == '.' && verify_dotfile(path)"-case to eat the '.'
> character without testing it against is_dir_sep, which is exactly what
> we want. The other cases return 0, as they used to. Good.
>
> Indeed, this is a cleaner approach. Thanks!
>

I forgot to ask; do you want me to resend? I would imagine the commit message should be updated to reflect this change as well...

Junio C Hamano· Jun 7, 2011, 19:22 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

Erik Faye-Lund <kusmabite@gmail.com> writes:
> I forgot to ask; do you want me to resend? I would imagine the commit
> message should be updated to reflect this change as well...

Here is what I queued last night. If it looks Ok then I'll merge it down to 'next'.

-- >8 --
Subject: [PATCH] verify_path(): simplify check at the directory boundary

We simply want to say "At a directory boundary, be careful with a name that begins with a dot, forbid a name that ends with the boundary character or has duplicated bounadry characters".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)
Show changes to read-cache.c +3 −10
diff --git a/read-cache.c b/read-cache.c
index 31cf0b5..3593291 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -784,16 +784,9 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
-				if (verify_dotfile(path))
-					continue;
-			}
-			return 0;
+			if ((c == '.' && !verify_dotfile(path)) ||
+			    is_dir_sep(c) || c == '\0')
+				return 0;
 		}
 		c = *path++;
 	}

-- 
1.7.6.rc0.129.gbe6ef
Erik Faye-Lund· Jun 7, 2011, 19:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

On Tue, Jun 7, 2011 at 9:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 44 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> I forgot to ask; do you want me to resend? I would imagine the commit
>> message should be updated to reflect this change as well...
>
> Here is what I queued last night. If it looks Ok then I'll merge it down
> to 'next'.
>
> -- >8 --
> Subject: [PATCH] verify_path(): simplify check at the directory boundary
>
> We simply want to say "At a directory boundary, be careful with a name
> that begins with a dot, forbid a name that ends with the boundary
> character or has duplicated bounadry characters".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  read-cache.c |   13 +++----------
>  1 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 31cf0b5..3593291 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -784,16 +784,9 @@ int verify_path(const char *path)
>                if (is_dir_sep(c)) {
>  inside:
>                        c = *path++;
> -                       switch (c) {
> -                       default:
> -                               continue;
> -                       case '/': case '\0':
> -                               break;
> -                       case '.':
> -                               if (verify_dotfile(path))
> -                                       continue;
> -                       }
> -                       return 0;
> +                       if ((c == '.' && !verify_dotfile(path)) ||
> +                           is_dir_sep(c) || c == '\0')
> +                               return 0;
>                }
>                c = *path++;
>        }
Looks good to me, thanks for following up on it :)
Theo Niessink· Jun 7, 2011, 11:46 UTC · re: Junio C Hamano · lore

RE: [PATCH 3/3] verify_path: consider dos drive prefix

Junio C Hamano wrote: 
> Let's not add an unnecessary goto while at it.  How about this on top
> instead?
Yeah, that is much cleaner indeed.
- Theo
Johannes Sixt· May 30, 2011, 20:23 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

Am 30.05.2011 11:32, schrieb Erik Faye-Lund:
Show 41 quoted lines
> On Fri, May 27, 2011 at 8:58 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Am 27.05.2011 18:00, schrieb Erik Faye-Lund:
>>> If someone manage to create a repo with a 'C:' entry in the
>>> root-tree, files can be written outside of the working-dir. This
>>> opens up a can-of-worms of exploits.
>>>
>>> Fix it by explicitly checking for a dos drive prefix when verifying
>>> a paht. While we're at it, make sure that paths beginning with '\' is
>>> considered absolute as well.
>>
>> I think we do agree that the only way to avoid the security breach is to
>> check a path before it is used to write a file. In practice, it means to
>> disallow paths in the top-most level of the index that are two
>> characters long and are letter-colon.
>>
>> IMHO, it is pointless to avoid that an evil path enters the repository,
>> because there are so many and a few more ways to create an evil repository.
>>
> 
> Yes, but this patch doesn't prevent that; it prevents an evil path
> from entering the index and from being checked out if the index is
> evil.
> 
>>> diff --git a/read-cache.c b/read-cache.c
>>> index f38471c..68faa51 100644
>>> --- a/read-cache.c
>>> +++ b/read-cache.c
>>> @@ -753,11 +753,14 @@ int verify_path(const char *path)
>>>  {
>>>       char c;
>>>
>>> +     if (has_dos_drive_prefix(path))
>>> +             return 0;
>>> +
>>
>> Isn't verify_path used to avoid that a bogus path enters the index? (I
>> don't know, I'm not familiar with this infrastructure.)
>>
> 
> Yes, it's being used to do that. But it's also being used when reading
> the index into memory, which is "the good stuf" for our purposes.

OK, I agree with the changes proposed in this patch. git reset and git checkout go through this function via unpack_trees(). Are there other ways to write a file, e.g., in merge-recursive?

-- Hannes
Junio C Hamano· May 27, 2011, 17:57 UTC · re: Erik Faye-Lund · lore

Re: [PATCH maint 0/3] do not write files outside of work-dir

Erik Faye-Lund <kusmabite@gmail.com> writes:
> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,
> where cloning an evil repository can arbitrarily overwrite files outside
> the repository. Since many Windows users run as administrators, this can
> be used for very nasty purposes.

Which of my integration branches do msysGit/Git for Windows folks base their releases these days? I could carry this through the regular "next to master and then sometime later to maint" schedule, but if you are not using maint and basing primarily on master then I'd rather skip the "and then sometime later to maint" part.

Johannes Schindelin· May 27, 2011, 18:09 UTC · re: Junio C Hamano · lore

Re: [PATCH maint 0/3] do not write files outside of work-dir

Hi Junio,
On Fri, 27 May 2011, Junio C Hamano wrote:
Show 12 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
> 
> > Theo Niessink has uncovered a serious sercurity issue in Git for 
> > Windows, where cloning an evil repository can arbitrarily overwrite 
> > files outside the repository. Since many Windows users run as 
> > administrators, this can be used for very nasty purposes.
> 
> Which of my integration branches do msysGit/Git for Windows folks base 
> their releases these days? I could carry this through the regular "next 
> to master and then sometime later to maint" schedule, but if you are not 
> using maint and basing primarily on master then I'd rather skip the "and 
> then sometime later to maint" part.
We follow 'next'.

[Cc:ing the msysGit list, as I don't know whether Pat or Sebastian follow git@vger]

Thanks, Johannes

Junio C Hamano· May 27, 2011, 19:16 UTC · re: Johannes Schindelin · lore

Re: [PATCH maint 0/3] do not write files outside of work-dir

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
>> Which of my integration branches do msysGit/Git for Windows folks base 
>> their releases these days? I could carry this through the regular "next 
>> to master and then sometime later to maint" schedule, but if you are not 
>> using maint and basing primarily on master then I'd rather skip the "and 
>> then sometime later to maint" part.
>
> We follow 'next'.
>
> [Cc:ing the msysGit list, as I don't know whether Pat or Sebastian follow 
> git@vger]
Thanks.

Then my preference would be to queue this to "next", wait for msysGit to cut a release based on that, and then graduate it to "master" on my side.

Tait· Jun 1, 2011, 04:14 UTC · re: Erik Faye-Lund · lore

Re: [PATCH maint 0/3] do not write files outside of work-dir

> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,
> where cloning an evil repository can arbitrarily overwrite files outside
> the repository...

Filenames starting with C: are not necessarily absolute. Consider "c:foo.txt" where c: is the current directory on drive C, or "c:stream1" where c is a single-letter filename in the current directory with an alternate data stream such as would be shown by dir /r. The has_dos_drive_prefix check is overly broad. Maybe this is intentional and just needs to be documented. Absolute paths like \\localhost\C$\file.txt and \\?\C:\file.txt do seem to be caught, because they start with '\'.

Microsoft says[1] a path is relative unless:
  - it begins with "\\"
  - it begins with a disk designator followed by a directory separator
  - it begins with a single "\"
On that basis, has_dos_drive_prefix(path) should be:
  isalpha(*(path)) && (path)[1] == ':' && is_dir_sep((path)[2])

However, there are also paths within the NT namespace (as opposed to the Win32 namespace, [1] again) that might be considered absolute, or at least to which git should not try to write. Examples would be PRN, CONOUT$, AUX, etc. These will not be caught by the current form of has_dos_drive_prefix, if that is even the right place to catch them. I think the QueryDosDevice function (given the part of the path up to the first directory separator, if one is present [2]) would detect them, and logical drive mappings as well. However, QueryDosDevice seems to also include many things that are not worthy of concern, like (on my computer) "DISPLAY5". Does anyone know the correct approach here?

I gather that other programs can create names like these (with DefineDosDevice), so a hard-coded exception list from [1] (that being: CON, PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, and LPT9) might not be adequate?

[1] http://msdn.microsoft.com/en-us/library/aa365247(v=vs.85).aspx [2] http://msdn.microsoft.com/en-us/library/aa365461(v=vs.85).aspx

Johannes Sixt· Jun 1, 2011, 06:31 UTC · re: Tait · lore

Re: [PATCH maint 0/3] do not write files outside of work-dir

Am 6/1/2011 6:14, schrieb Tait:
Show 6 quoted lines
>> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,
>> where cloning an evil repository can arbitrarily overwrite files outside
>> the repository...
> 
> Filenames starting with C: are not necessarily absolute. Consider
> "c:foo.txt" where c: is the current directory on drive C, or

We have a different notion of "absolute path". This one *is* absolute per our definition. See below.

> "c:stream1" where c is a single-letter filename in the current directory
> with an alternate data stream such as would be shown by dir /r. The

On my system, this does not create a file in the current directory with an alternate data stream, but - while the working directory is somewhere on drive D - a file is created on drive C.

Show 11 quoted lines
> has_dos_drive_prefix check is overly broad. Maybe this is intentional and
> just needs to be documented. Absolute paths like \\localhost\C$\file.txt
> and \\?\C:\file.txt do seem to be caught, because they start with '\'.
> 
> Microsoft says[1] a path is relative unless:
>   - it begins with "\\"
>   - it begins with a disk designator followed by a directory separator
>   - it begins with a single "\"
> 
> On that basis, has_dos_drive_prefix(path) should be:
>   isalpha(*(path)) && (path)[1] == ':' && is_dir_sep((path)[2])

This is not the definition of "relative path" that we are interested in. Let $PWD be the current directory. For our purposes, a path $P is relative if $P and $PWD/$P designate the same file system entry. Otherwise, $P is an absolute path.

With this definition, the current has_dos_drive_prefix() is good enough.
> However, there are also paths within the NT namespace (as opposed to the
> Win32 namespace, [1] again) that might be considered absolute, or at least
> to which git should not try to write. Examples would be PRN, CONOUT$, AUX,

For our purposes, these names are all relative paths. It's a case of "Doctor, it hurts when I stick my finger in my eye" if you have a repository with these names.

Note that git never writes to these files: It always first allocates a temporary file, eg. nul.123456; but this will already fail because these special file names are forbidden even when a file extension is attached.

-- Hannes

← back to recent threads