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

--

[ message continues ]

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

--

[ message continues ]

Just to be a tad more helpful, that would be something like:

	join(' ', map {
        	s/\047/\047\134\047\047/g;
                "'$_'";
	} git_cmd()));


--

[ message continues ]

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(), ...
[ message continues ]

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

[ message continues ]

Looks sane.  Thanks.
--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

Even without Git::Repo using git-cat-file new '--batch-check' option

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

[ message continues ]