threads / patch / 13991

patchgitweb: fix support for repository directories with spaces

Subject: [PATCH] gitweb: fix support for repository directories with spaces

## tl;dr

9 messages between Jun 17, 2008 and Jun 17, 2008. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Lea Wiemann· Jun 17, 2008, 01:09 UTC · lore
git_cmd_str does not quote the directory names without this patch.
Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
git_cmd_str is really really bad from a security POV: Where it is
used, command lines are passed to the shell, which (I believe) just
*happen* to open no security holes.  Hence the function should
ultimately go away.  However, let's make the tests work for the
meantime while it's still there.
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +1 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 07e64da..0bddc31 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1502,7 +1502,7 @@ sub git_cmd {
 
 # returns path to the core git executable and the --git-dir parameter as string
 sub git_cmd_str {
-	return join(' ', git_cmd());
+	return join ' ', map("'$_'", git_cmd());
 }
 
 # get HEAD ref of given project as hash
-- 
1.5.6.rc3.7.ged9620
Junio C Hamano· Jun 17, 2008, 01:14 UTC · re: Lea Wiemann · lore

Re: [PATCH] gitweb: fix support for repository directories with spaces

Lea Wiemann <lewiemann@gmail.com> writes:
Show 24 quoted lines
> git_cmd_str does not quote the directory names without this patch.
>
> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
> ---
> git_cmd_str is really really bad from a security POV: Where it is
> used, command lines are passed to the shell, which (I believe) just
> *happen* to open no security holes.  Hence the function should
> ultimately go away.  However, let's make the tests work for the
> meantime while it's still there.
>
>  gitweb/gitweb.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 07e64da..0bddc31 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1502,7 +1502,7 @@ sub git_cmd {
>  
>  # returns path to the core git executable and the --git-dir parameter as string
>  sub git_cmd_str {
> -	return join(' ', git_cmd());
> +	return join ' ', map("'$_'", git_cmd());
>  }
What happens to a path or parameter that has a sq in it?
You are returing this from git_cmd():
	return $GIT, '--git-dir='.$git_dir;

How is this cmd_str() gets used? If you absolutely have to have a single string that can be safely passed to the shell, the easiest would be to quote mechanically in sq following the pattern illustrated at the beginning of quote.c

Junio C Hamano· Jun 17, 2008, 21:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitweb: fix support for repository directories with spaces

Junio C Hamano <gitster@pobox.com> writes:
Show 37 quoted lines
> Lea Wiemann <lewiemann@gmail.com> writes:
>
>> git_cmd_str does not quote the directory names without this patch.
>>
>> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
>> ---
>> git_cmd_str is really really bad from a security POV: Where it is
>> used, command lines are passed to the shell, which (I believe) just
>> *happen* to open no security holes.  Hence the function should
>> ultimately go away.  However, let's make the tests work for the
>> meantime while it's still there.
>>
>>  gitweb/gitweb.perl |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 07e64da..0bddc31 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -1502,7 +1502,7 @@ sub git_cmd {
>>  
>>  # returns path to the core git executable and the --git-dir parameter as string
>>  sub git_cmd_str {
>> -	return join(' ', git_cmd());
>> +	return join ' ', map("'$_'", git_cmd());
>>  }
>
> What happens to a path or parameter that has a sq in it?
>
> You are returing this from git_cmd():
>
> 	return $GIT, '--git-dir='.$git_dir;
>
> How is this cmd_str() gets used?  If you absolutely have to have a single
> string that can be safely passed to the shell, the easiest would be to
> quote mechanically in sq following the pattern illustrated at the
> beginning of quote.c
Just to be a tad more helpful, that would be something like:
	join(' ', map {
        	s/\047/\047\134\047\047/g;
                "'$_'";
	} git_cmd()));
Lea Wiemann· Jun 17, 2008, 21:46 UTC · re: Junio C Hamano · lore

[PATCH v2] gitweb: quote commands properly when calling the shell

This eliminates the function git_cmd_str, which was used for composing command lines, and adds a quote_command function, which quotes all of its arguments (as in quote.c).

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Changed since v1: Quote the whole command line, safely.

I've tested that the object and snapshot actions still work (which is where git_cmd_str was used), and I've hand-tested the quote_command function. *wait-for-test-suite-to-appear-in-later-revisions*

Hope this addresses your concerns, Junio!
-- Lea
 gitweb/gitweb.perl |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)
Show changes to gitweb/gitweb.perl +14 −10
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7b1b076..3a7adae 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1500,9 +1500,13 @@ sub git_cmd {
 	return $GIT, '--git-dir='.$git_dir;
 }
 
-# returns path to the core git executable and the --git-dir parameter as string
-sub git_cmd_str {
-	return join(' ', git_cmd());
+# quote the given arguments for passing them to the shell
+# quote_command("command", "arg 1", "arg with ' and ! characters")
+# => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
+# Try to avoid using this function wherever possible.
+sub quote_command {
+	return join(' ',
+		    map( { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ ));
 }
 
 # get HEAD ref of given project as hash
@@ -4493,7 +4497,6 @@ sub git_snapshot {
 		$hash = git_get_head_hash($project);
 	}
 
-	my $git_command = git_cmd_str();
 	my $name = $project;
 	$name =~ s,([^/])/*\.git$,$1,;
 	$name = basename($name);
@@ -4501,11 +4504,12 @@ sub git_snapshot {
 	$name =~ s/\047/\047\\\047\047/g;
 	my $cmd;
 	$filename .= "-$hash$known_snapshot_formats{$format}{'suffix'}";
-	$cmd = "$git_command archive " .
-		"--format=$known_snapshot_formats{$format}{'format'} " .
-		"--prefix=\'$name\'/ $hash";
+	$cmd = quote_command(
+		git_cmd(), 'archive',
+		"--format=$known_snapshot_formats{$format}{'format'}",
+		"--prefix=$name/", $hash);
 	if (exists $known_snapshot_formats{$format}{'compressor'}) {
-		$cmd .= ' | ' . join ' ', @{$known_snapshot_formats{$format}{'compressor'}};
+		$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});
 	}
 
 	print $cgi->header(
@@ -4718,8 +4722,8 @@ sub git_object {
 	if ($hash || ($hash_base && !defined $file_name)) {
 		my $object_id = $hash || $hash_base;
 
-		my $git_command = git_cmd_str();
-		open my $fd, "-|", "$git_command cat-file -t $object_id 2>/dev/null"
+		open my $fd, "-|", quote_command(
+			git_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'
 			or die_error('404 Not Found', "Object does not exist");
 		$type = <$fd>;
 		chomp $type;
-- 
1.5.6.rc3.7.ged9620
Lea Wiemann· Jun 17, 2008, 21:51 UTC · re: Lea Wiemann · lore

Re: [PATCH v2] gitweb: quote commands properly when calling the shell

Lea Wiemann wrote:
> Subject: Re: [PATCH v2] gitweb: quote commands properly when calling the shell

Junio, can you please drop me a line if/when you accept this, since I'll need to resend the HTTP status code patch; it conflicts as-is.

-- Lea
Junio C Hamano· Jun 17, 2008, 23:41 UTC · re: Lea Wiemann · lore

Re: [PATCH v2] gitweb: quote commands properly when calling the shell

Lea Wiemann <lewiemann@gmail.com> writes:
> This eliminates the function git_cmd_str, which was used for composing
> command lines, and adds a quote_command function, which quotes all of
> its arguments (as in quote.c).
Looks sane.  Thanks.
Jakub Narebski· Jun 17, 2008, 01:38 UTC · re: Lea Wiemann · lore

Re: [PATCH] gitweb: fix support for repository directories with spaces

Lea Wiemann <lewiemann@gmail.com> writes:
Show 9 quoted lines
> git_cmd_str does not quote the directory names without this patch.
> 
> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
> ---
> git_cmd_str is really really bad from a security POV: Where it is
> used, command lines are passed to the shell, which (I believe) just
> *happen* to open no security holes.  Hence the function should
> ultimately go away.  However, let's make the tests work for the
> meantime while it's still there.

I'd like to do away with need for git_cmd_str(), but unfortunately it is needed in a place where git has to form pipeline, namely in creating externally compressed snapshot (in git_snapshot), and to redirect stderr to /dev/null in git_object.

Perhaps we could simply do without second, but this pipeline is here
to stay (there was pipeline in git-search, but was replaced by
invoking git-log instead of rev-list | diff-tree pipeline).  And it is
not easy to create pipeline using some variant of list form of open;
if you search git mailing list archive you can find aborted (RFC only)
attempt to create pipeline safely
  http://thread.gmane.org/gmane.comp.version-control.git/76566

If you are extending Git.pm (please do not foget Cc Petr Baudis, as it is mainly his code) for gitweb, you can try to add this. It doesn't have to be very generic...

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Lea Wiemann· Jun 17, 2008, 22:07 UTC · re: Jakub Narebski · lore

Re: [PATCH] gitweb: fix support for repository directories with spaces

Jakub Narebski wrote:
> I'd like to do away with need for git_cmd_str(), but unfortunately it
> is needed in a place where git has to form pipeline, namely in
> creating externally compressed snapshot (in git_snapshot), and to
> redirect stderr to /dev/null in git_object.

git_objects's use of 2> /dev/null won't be necessary since the Git::Repo API uses cat-file --batch-check, which doesn't (well, shouldn't) write on stderr.

If the use of shell command lines in git_snapshot bothers us enough, we can (a) create the pipe ourselves and just have it not work on Windows, (b) create it ourselves and spend a lot of time working around Windows' horribly borked API, or (c) use Perl's Zlib/Bzip2/LZO libraries. If anything I'm in favor of (c), though it makes installation harder if you want compressed tarballs. I'm fine with leaving it as is.

-- Lea
Jakub Narebski· Jun 17, 2008, 22:27 UTC · re: Lea Wiemann · lore

Re: [PATCH] gitweb: fix support for repository directories with spaces

Lea Wiemann wrote:
Show 9 quoted lines
> Jakub Narebski wrote:
> > I'd like to do away with need for git_cmd_str(), but unfortunately it
> > is needed in a place where git has to form pipeline, namely in
> > creating externally compressed snapshot (in git_snapshot), and to
> > redirect stderr to /dev/null in git_object.
> 
> git_objects's use of 2> /dev/null won't be necessary since the Git::Repo 
> API uses cat-file --batch-check, which doesn't (well, shouldn't) write 
> on stderr.

Even without Git::Repo using git-cat-file new '--batch-check' option would be good replacement.

Show 6 quoted lines
> If the use of shell command lines in git_snapshot bothers us enough, we 
> can (a) create the pipe ourselves and just have it not work on Windows, 
> (b) create it ourselves and spend a lot of time working around Windows' 
> horribly borked API, or (c) use Perl's Zlib/Bzip2/LZO libraries.  If 
> anything I'm in favor of (c), though it makes installation harder if you 
> want compressed tarballs.  I'm fine with leaving it as is.

Please remember that gitweb is to be installed also in tightly controlled server installations, where anything outside default packages, or extras package repository, or at least trusted contrib packages repository is out of the question. Installing from CPAN is not an option.

That is why I'd rather avoid dependencies on modules which are not distributed with Perl by default.

And there is another solution, (d) add gzip/bzip2 compression support to git-archive ;-P

-- 
Jakub Narebski
Poland

← back to recent threads