{"thread":{"id":"13991","subject":"[PATCH] gitweb: fix support for repository directories with spaces","startedAt":"2008-06-17T01:09:37Z","lastAt":"2008-06-17T23:41:08Z","messageCount":9,"participants":["Lea Wiemann","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"80094","messageId":"1213664977-23964-1-git-send-email-LeWiemann@gmail.com","threadId":"13991","inReplyTo":null,"subject":"[PATCH] gitweb: fix support for repository directories with spaces","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T01:09:37Z","receivedAt":"2008-06-17T01:09:37Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"git_cmd_str does not quote the directory names without this patch.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\ngit_cmd_str is really really bad from a security POV: Where it is\nused, command lines are passed to the shell, which (I believe) just\n*happen* to open no security holes.  Hence the function should\nultimately go away.  However, let's make the tests work for the\nmeantime while it's still there.\n\n gitweb/gitweb.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 07e64da..0bddc31 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1502,7 +1502,7 @@ sub git_cmd {\n \n # returns path to the core git executable and the --git-dir parameter as string\n sub git_cmd_str {\n-\treturn join(' ', git_cmd());\n+\treturn join ' ', map(\"'$_'\", git_cmd());\n }\n \n # get HEAD ref of given project as hash\n-- \n1.5.6.rc3.7.ged9620\n"},{"id":"80095","messageId":"7vd4mg9824.fsf@gitster.siamese.dyndns.org","threadId":"13991","inReplyTo":"1213664977-23964-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: fix support for repository directories with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-17T01:14:27Z","receivedAt":"2008-06-17T01:14:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> git_cmd_str does not quote the directory names without this patch.\n>\n> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>\n> ---\n> git_cmd_str is really really bad from a security POV: Where it is\n> used, command lines are passed to the shell, which (I believe) just\n> *happen* to open no security holes.  Hence the function should\n> ultimately go away.  However, let's make the tests work for the\n> meantime while it's still there.\n>\n>  gitweb/gitweb.perl |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 07e64da..0bddc31 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1502,7 +1502,7 @@ sub git_cmd {\n>  \n>  # returns path to the core git executable and the --git-dir parameter as string\n>  sub git_cmd_str {\n> -\treturn join(' ', git_cmd());\n> +\treturn join ' ', map(\"'$_'\", git_cmd());\n>  }\n\nWhat happens to a path or parameter that has a sq in it?\n\nYou are returing this from git_cmd():\n\n\treturn $GIT, '--git-dir='.$git_dir;\n\nHow is this cmd_str() gets used?  If you absolutely have to have a single\nstring that can be safely passed to the shell, the easiest would be to\nquote mechanically in sq following the pattern illustrated at the\nbeginning of quote.c\n"},{"id":"80096","messageId":"m3k5goon7v.fsf@localhost.localdomain","threadId":"13991","inReplyTo":"1213664977-23964-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: fix support for repository directories with spaces","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-17T01:38:02Z","receivedAt":"2008-06-17T01:38:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> git_cmd_str does not quote the directory names without this patch.\n> \n> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>\n> ---\n> git_cmd_str is really really bad from a security POV: Where it is\n> used, command lines are passed to the shell, which (I believe) just\n> *happen* to open no security holes.  Hence the function should\n> ultimately go away.  However, let's make the tests work for the\n> meantime while it's still there.\n\nI'd like to do away with need for git_cmd_str(), but unfortunately it\nis needed in a place where git has to form pipeline, namely in\ncreating externally compressed snapshot (in git_snapshot), and to\nredirect stderr to /dev/null in git_object.\n\nPerhaps we could simply do without second, but this pipeline is here\nto stay (there was pipeline in git-search, but was replaced by\ninvoking git-log instead of rev-list | diff-tree pipeline).  And it is\nnot easy to create pipeline using some variant of list form of open;\nif you search git mailing list archive you can find aborted (RFC only)\nattempt to create pipeline safely\n  http://thread.gmane.org/gmane.comp.version-control.git/76566\n\nIf you are extending Git.pm (please do not foget Cc Petr Baudis, as it\nis mainly his code) for gitweb, you can try to add this.  It doesn't\nhave to be very generic...\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"80163","messageId":"7vk5gn69cq.fsf@gitster.siamese.dyndns.org","threadId":"13991","inReplyTo":"7vd4mg9824.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: fix support for repository directories with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-17T21:27:01Z","receivedAt":"2008-06-17T21:27:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Lea Wiemann <lewiemann@gmail.com> writes:\n>\n>> git_cmd_str does not quote the directory names without this patch.\n>>\n>> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>\n>> ---\n>> git_cmd_str is really really bad from a security POV: Where it is\n>> used, command lines are passed to the shell, which (I believe) just\n>> *happen* to open no security holes.  Hence the function should\n>> ultimately go away.  However, let's make the tests work for the\n>> meantime while it's still there.\n>>\n>>  gitweb/gitweb.perl |    2 +-\n>>  1 files changed, 1 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 07e64da..0bddc31 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1502,7 +1502,7 @@ sub git_cmd {\n>>  \n>>  # returns path to the core git executable and the --git-dir parameter as string\n>>  sub git_cmd_str {\n>> -\treturn join(' ', git_cmd());\n>> +\treturn join ' ', map(\"'$_'\", git_cmd());\n>>  }\n>\n> What happens to a path or parameter that has a sq in it?\n>\n> You are returing this from git_cmd():\n>\n> \treturn $GIT, '--git-dir='.$git_dir;\n>\n> How is this cmd_str() gets used?  If you absolutely have to have a single\n> string that can be safely passed to the shell, the easiest would be to\n> quote mechanically in sq following the pattern illustrated at the\n> beginning of quote.c\n\nJust to be a tad more helpful, that would be something like:\n\n\tjoin(' ', map {\n        \ts/\\047/\\047\\134\\047\\047/g;\n                \"'$_'\";\n\t} git_cmd()));\n"},{"id":"80167","messageId":"1213739195-29284-1-git-send-email-LeWiemann@gmail.com","threadId":"13991","inReplyTo":"7vd4mg9824.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v2] gitweb: quote commands properly when calling the shell","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T21:46:35Z","receivedAt":"2008-06-17T21:46:35Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"This eliminates the function git_cmd_str, which was used for composing\ncommand lines, and adds a quote_command function, which quotes all of\nits arguments (as in quote.c).\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\nChanged since v1: Quote the whole command line, safely.\n\nI've tested that the object and snapshot actions still work (which is\nwhere git_cmd_str was used), and I've hand-tested the quote_command\nfunction.  *wait-for-test-suite-to-appear-in-later-revisions*\n\nHope this addresses your concerns, Junio!\n\n-- Lea\n\n\n gitweb/gitweb.perl |   24 ++++++++++++++----------\n 1 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7b1b076..3a7adae 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1500,9 +1500,13 @@ sub git_cmd {\n \treturn $GIT, '--git-dir='.$git_dir;\n }\n \n-# returns path to the core git executable and the --git-dir parameter as string\n-sub git_cmd_str {\n-\treturn join(' ', git_cmd());\n+# quote the given arguments for passing them to the shell\n+# quote_command(\"command\", \"arg 1\", \"arg with ' and ! characters\")\n+# => \"'command' 'arg 1' 'arg with '\\'' and '\\!' characters'\"\n+# Try to avoid using this function wherever possible.\n+sub quote_command {\n+\treturn join(' ',\n+\t\t    map( { my $a = $_; $a =~ s/(['!])/'\\\\$1'/g; \"'$a'\" } @_ ));\n }\n \n # get HEAD ref of given project as hash\n@@ -4493,7 +4497,6 @@ sub git_snapshot {\n \t\t$hash = git_get_head_hash($project);\n \t}\n \n-\tmy $git_command = git_cmd_str();\n \tmy $name = $project;\n \t$name =~ s,([^/])/*\\.git$,$1,;\n \t$name = basename($name);\n@@ -4501,11 +4504,12 @@ sub git_snapshot {\n \t$name =~ s/\\047/\\047\\\\\\047\\047/g;\n \tmy $cmd;\n \t$filename .= \"-$hash$known_snapshot_formats{$format}{'suffix'}\";\n-\t$cmd = \"$git_command archive \" .\n-\t\t\"--format=$known_snapshot_formats{$format}{'format'} \" .\n-\t\t\"--prefix=\\'$name\\'/ $hash\";\n+\t$cmd = quote_command(\n+\t\tgit_cmd(), 'archive',\n+\t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n+\t\t\"--prefix=$name/\", $hash);\n \tif (exists $known_snapshot_formats{$format}{'compressor'}) {\n-\t\t$cmd .= ' | ' . join ' ', @{$known_snapshot_formats{$format}{'compressor'}};\n+\t\t$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});\n \t}\n \n \tprint $cgi->header(\n@@ -4718,8 +4722,8 @@ sub git_object {\n \tif ($hash || ($hash_base && !defined $file_name)) {\n \t\tmy $object_id = $hash || $hash_base;\n \n-\t\tmy $git_command = git_cmd_str();\n-\t\topen my $fd, \"-|\", \"$git_command cat-file -t $object_id 2>/dev/null\"\n+\t\topen my $fd, \"-|\", quote_command(\n+\t\t\tgit_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'\n \t\t\tor die_error('404 Not Found', \"Object does not exist\");\n \t\t$type = <$fd>;\n \t\tchomp $type;\n-- \n1.5.6.rc3.7.ged9620\n"},{"id":"80168","messageId":"485831F9.2090408@gmail.com","threadId":"13991","inReplyTo":"1213739195-29284-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] gitweb: quote commands properly when calling the shell","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T21:51:53Z","receivedAt":"2008-06-17T21:51:53Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Lea Wiemann wrote:\n> Subject: Re: [PATCH v2] gitweb: quote commands properly when calling the shell\n\nJunio, can you please drop me a line if/when you accept this, since I'll \nneed to resend the HTTP status code patch; it conflicts as-is.\n\n-- Lea\n"},{"id":"80171","messageId":"48583584.6030906@gmail.com","threadId":"13991","inReplyTo":"m3k5goon7v.fsf@localhost.localdomain","subject":"Re: [PATCH] gitweb: fix support for repository directories with spaces","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T22:07:00Z","receivedAt":"2008-06-17T22:07:00Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> I'd like to do away with need for git_cmd_str(), but unfortunately it\n> is needed in a place where git has to form pipeline, namely in\n> creating externally compressed snapshot (in git_snapshot), and to\n> redirect stderr to /dev/null in git_object.\n\ngit_objects's use of 2> /dev/null won't be necessary since the Git::Repo \nAPI uses cat-file --batch-check, which doesn't (well, shouldn't) write \non stderr.\n\nIf the use of shell command lines in git_snapshot bothers us enough, we \ncan (a) create the pipe ourselves and just have it not work on Windows, \n(b) create it ourselves and spend a lot of time working around Windows' \nhorribly borked API, or (c) use Perl's Zlib/Bzip2/LZO libraries.  If \nanything I'm in favor of (c), though it makes installation harder if you \nwant compressed tarballs.  I'm fine with leaving it as is.\n\n-- Lea\n"},{"id":"80174","messageId":"200806180027.47810.jnareb@gmail.com","threadId":"13991","inReplyTo":"48583584.6030906@gmail.com","subject":"Re: [PATCH] gitweb: fix support for repository directories with spaces","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-17T22:27:47Z","receivedAt":"2008-06-17T22:27:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Jakub Narebski wrote:\n> > I'd like to do away with need for git_cmd_str(), but unfortunately it\n> > is needed in a place where git has to form pipeline, namely in\n> > creating externally compressed snapshot (in git_snapshot), and to\n> > redirect stderr to /dev/null in git_object.\n> \n> git_objects's use of 2> /dev/null won't be necessary since the Git::Repo \n> API uses cat-file --batch-check, which doesn't (well, shouldn't) write \n> on stderr.\n\nEven without Git::Repo using git-cat-file new '--batch-check' option\nwould be good replacement.\n\n> If the use of shell command lines in git_snapshot bothers us enough, we \n> can (a) create the pipe ourselves and just have it not work on Windows, \n> (b) create it ourselves and spend a lot of time working around Windows' \n> horribly borked API, or (c) use Perl's Zlib/Bzip2/LZO libraries.  If \n> anything I'm in favor of (c), though it makes installation harder if you \n> want compressed tarballs.  I'm fine with leaving it as is.\n\nPlease remember that gitweb is to be installed also in tightly\ncontrolled server installations, where anything outside default\npackages, or extras package repository, or at least trusted contrib\npackages repository is out of the question.  Installing from CPAN\nis not an option.\n\nThat is why I'd rather avoid dependencies on modules which are not\ndistributed with Perl by default.\n\nAnd there is another solution, (d) add gzip/bzip2 compression support\nto git-archive ;-P\n-- \nJakub Narebski\nPoland\n"},{"id":"80184","messageId":"7vtzfr3a0b.fsf@gitster.siamese.dyndns.org","threadId":"13991","inReplyTo":"1213739195-29284-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] gitweb: quote commands properly when calling the shell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-17T23:41:08Z","receivedAt":"2008-06-17T23:41:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> This eliminates the function git_cmd_str, which was used for composing\n> command lines, and adds a quote_command function, which quotes all of\n> its arguments (as in quote.c).\n\nLooks sane.  Thanks.\n"}]}