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

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

[Lea Wiemann]

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());
 }
 
 # get HEAD ref of given project as hash
-- 
1.5.6.rc3.7.ged9620

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

[Junio C Hamano]

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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()));

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

[Lea Wiemann]

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(-)

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

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

[Lea Wiemann]

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

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

[Junio C Hamano]

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.

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

[Jakub Narebski]

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.

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

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

[Lea Wiemann]

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

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

[Jakub Narebski]

Lea Wiemann wrote:
> 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.

> 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