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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: May 30, 2011, 09:32 UTC
Message-ID: <BANLkTikdeq7cuhi0uo7Q6wqDJK3nxjmP-g@mail.gmail.com>
In-Reply-To: <4DDFF473.7030104@kdbg.org>

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?

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