threads / patch / 12655

patchgit-quiltimport: fix security risk because of un-sanitized $level.

Subject: [PATCH] git-quiltimport: fix security risk because of un-sanitized $level.

## tl;dr

3 messages between Mar 12, 2008 and Mar 12, 2008. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Pierre Habouzit· Mar 12, 2008, 20:46 UTC · lore
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
  I assume that nobody will have a series with -p1000 in it :)
  sorry for this gross mistake in the first place.
  [ for the inattentive readers $level was used without quoting, for
    good reasons as it's sometime empty and then we don't want to pass
    an empty argument to git-apply, though someone could use that to run
    arbitrary commands, not nice ]
 git-quiltimport.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-quiltimport.sh +1 −1
diff --git a/git-quiltimport.sh b/git-quiltimport.sh
index 84c8b8e..d35cb02 100755
--- a/git-quiltimport.sh
+++ b/git-quiltimport.sh
@@ -67,7 +67,7 @@ while read patch_name level garbage
 do
 	case "$patch_name" in ''|'#'*) continue;; esac
 	case "$level" in
-	-p*);;
+	-p[0-9]|-p[0-9][0-9]|-p[0-9][0-9][0-9]);;
 	''|'#'*)
 		level=;;
 	*)
-- 
1.5.4.4.599.gba501
Junio C Hamano· Mar 12, 2008, 20:55 UTC · re: Pierre Habouzit · lore

Re: [PATCH] git-quiltimport: fix security risk because of un-sanitized $level.

Pierre Habouzit <madcoder@debian.org> writes:
Show 10 quoted lines
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>
>   I assume that nobody will have a series with -p1000 in it :)
>   sorry for this gross mistake in the first place.
>
>   [ for the inattentive readers $level was used without quoting, for
>     good reasons as it's sometime empty and then we don't want to pass
>     an empty argument to git-apply, though someone could use that to run
>     arbitrary commands, not nice ]

A traditional way to deal with that situation in shell scripts is to use this idiom:

	${var_that_may_not_be_set+"$var_that_may_not_be_set"}

You can use :+ in place of + to also reject empty string on modern systems.

#!/bin/sh not_set=t ; unset not_set is_set=t

report () {
	echo "I got $# args"
        i=1
        for it
        do
        	echo "$i: $it"
                i=$(( $i+1 ))
	done
        echo
}

report sending not_set ${not_set:+"$not_set"} string report sending is_set ${is_set:+"$is_set"} string

Pierre Habouzit· Mar 12, 2008, 22:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-quiltimport: fix security risk because of un-sanitized $level.

On Wed, Mar 12, 2008 at 08:55:56PM +0000, Junio C Hamano wrote:
Show 17 quoted lines
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> >
> >   I assume that nobody will have a series with -p1000 in it :)
> >   sorry for this gross mistake in the first place.
> >
> >   [ for the inattentive readers $level was used without quoting, for
> >     good reasons as it's sometime empty and then we don't want to pass
> >     an empty argument to git-apply, though someone could use that to run
> >     arbitrary commands, not nice ]
> 
> A traditional way to deal with that situation in shell scripts is to use
> this idiom:
> 
> 	${var_that_may_not_be_set+"$var_that_may_not_be_set"}
  Oh this is nicer than what I used indeed. My shell scripting skills
are quite limited :)
  Anyways, I happened to notice this stupid mistake when I re-read the
patch you merged. You probably want to use that trick instead (rather
than my poor sanitizing thing), and let git-apply sort out the rest.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

← back to recent threads