{"thread":{"id":"12655","subject":"[PATCH] git-quiltimport: fix security risk because of un-sanitized $level.","startedAt":"2008-03-12T20:46:48Z","lastAt":"2008-03-12T22:29:06Z","messageCount":3,"participants":["Pierre Habouzit","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"71841","messageId":"20080312204648.GA28729@artemis.madism.org","threadId":"12655","inReplyTo":null,"subject":"[PATCH] git-quiltimport: fix security risk because of un-sanitized $level.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-03-12T20:46:48Z","receivedAt":"2008-03-12T20:46:48Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n\n  I assume that nobody will have a series with -p1000 in it :)\n  sorry for this gross mistake in the first place.\n\n  [ for the inattentive readers $level was used without quoting, for\n    good reasons as it's sometime empty and then we don't want to pass\n    an empty argument to git-apply, though someone could use that to run\n    arbitrary commands, not nice ]\n\n git-quiltimport.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-quiltimport.sh b/git-quiltimport.sh\nindex 84c8b8e..d35cb02 100755\n--- a/git-quiltimport.sh\n+++ b/git-quiltimport.sh\n@@ -67,7 +67,7 @@ while read patch_name level garbage\n do\n \tcase \"$patch_name\" in ''|'#'*) continue;; esac\n \tcase \"$level\" in\n-\t-p*);;\n+\t-p[0-9]|-p[0-9][0-9]|-p[0-9][0-9][0-9]);;\n \t''|'#'*)\n \t\tlevel=;;\n \t*)\n-- \n1.5.4.4.599.gba501\n"},{"id":"71843","messageId":"7v63vr64rn.fsf@gitster.siamese.dyndns.org","threadId":"12655","inReplyTo":"20080312204648.GA28729@artemis.madism.org","subject":"Re: [PATCH] git-quiltimport: fix security risk because of un-sanitized $level.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-12T20:55:56Z","receivedAt":"2008-03-12T20:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit <madcoder@debian.org> writes:\n\n> Signed-off-by: Pierre Habouzit <madcoder@debian.org>\n> ---\n>\n>   I assume that nobody will have a series with -p1000 in it :)\n>   sorry for this gross mistake in the first place.\n>\n>   [ for the inattentive readers $level was used without quoting, for\n>     good reasons as it's sometime empty and then we don't want to pass\n>     an empty argument to git-apply, though someone could use that to run\n>     arbitrary commands, not nice ]\n\nA traditional way to deal with that situation in shell scripts is to use\nthis idiom:\n\n\t${var_that_may_not_be_set+\"$var_that_may_not_be_set\"}\n\nYou can use :+ in place of + to also reject empty string on modern\nsystems.\n\n#!/bin/sh\nnot_set=t ; unset not_set\nis_set=t\n\nreport () {\n\techo \"I got $# args\"\n        i=1\n        for it\n        do\n        \techo \"$i: $it\"\n                i=$(( $i+1 ))\n\tdone\n        echo\n}\n\nreport sending not_set ${not_set:+\"$not_set\"} string\nreport sending is_set ${is_set:+\"$is_set\"} string\n"},{"id":"71868","messageId":"20080312222906.GA12800@artemis.madism.org","threadId":"12655","inReplyTo":"7v63vr64rn.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-quiltimport: fix security risk because of un-sanitized $level.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-03-12T22:29:06Z","receivedAt":"2008-03-12T22:29:06Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Wed, Mar 12, 2008 at 08:55:56PM +0000, Junio C Hamano wrote:\n> Pierre Habouzit <madcoder@debian.org> writes:\n> \n> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>\n> > ---\n> >\n> >   I assume that nobody will have a series with -p1000 in it :)\n> >   sorry for this gross mistake in the first place.\n> >\n> >   [ for the inattentive readers $level was used without quoting, for\n> >     good reasons as it's sometime empty and then we don't want to pass\n> >     an empty argument to git-apply, though someone could use that to run\n> >     arbitrary commands, not nice ]\n> \n> A traditional way to deal with that situation in shell scripts is to use\n> this idiom:\n> \n> \t${var_that_may_not_be_set+\"$var_that_may_not_be_set\"}\n\n  Oh this is nicer than what I used indeed. My shell scripting skills\nare quite limited :)\n\n  Anyways, I happened to notice this stupid mistake when I re-read the\npatch you merged. You probably want to use that trick instead (rather\nthan my poor sanitizing thing), and let git-apply sort out the rest.\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"}]}