code review 4248041: goinstall: protect against malicious filenames.

goinstall: protect against malicious filenames.
It was possible to make package run arbitrary
commands when installing if its filenames contained
make metacharacters.

[rog, 2011-02-25 13:02:51]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[niemeyer, 2011-02-25 13:29:26]

LGTM

http://codereview.appspot.com/4248041/diff/3001/src/cmd/goinstall/parse.go
File src/cmd/goinstall/parse.go (right):

http://codereview.appspot.com/4248041/diff/3001/src/cmd/goinstall/parse.go#ne...
src/cmd/goinstall/parse.go:58: if strings.IndexAny(d.Name, "$\n\r") >= 0 {
Backquotes should be included in that list.

[rog, 2011-02-25 14:14:24]

http://codereview.appspot.com/4248041/diff/3001/src/cmd/goinstall/parse.go
File src/cmd/goinstall/parse.go (right):

http://codereview.appspot.com/4248041/diff/3001/src/cmd/goinstall/parse.go#ne...
src/cmd/goinstall/parse.go:58: if strings.IndexAny(d.Name, "$\n\r") >= 0 {
On 2011/02/25 13:29:26, niemeyer wrote:
> Backquotes should be included in that list.

ok, but i didn't think backquotes were special to make (i tried them and they
weren't interpreted)

[niemeyer, 2011-02-25 14:27:39]

> ok, but i didn't think backquotes were special to make

Try this:

FOO=`echo Hi`
all:
        6g $(FOO)

[rog, 2011-02-25 14:48:21]

interesting. but they don't work in prerequisite lists, which
is where the filenames end up.

try this:

all: \
	`echo Hi'`
	echo $<

i could add it anyway, just for good measure.

On 25 February 2011 14:27,  <n13m3y3r@gmail.com> wrote:
>> ok, but i didn't think backquotes were special to make
>
> Try this:
>
> FOO=`echo Hi`
> all:
>        6g $(FOO)
>
> http://codereview.appspot.com/4248041/
>

[niemeyer, 2011-02-25 15:02:14]

> interesting. but they don't work in prerequisite lists, which
> is where the filenames end up.

goinstall github.com/niemeyer/shutdown

[rog, 2011-02-25 15:09:31]

ah, of course, it's not just make metacharacters but also
sh metacharacters because the filename ends up being passed
to the shell.

are there any more potentially dodgy bash metacharacters?
i'm not that familiar with bash.

On 25 February 2011 15:02,  <n13m3y3r@gmail.com> wrote:
>> interesting. but they don't work in prerequisite lists, which
>> is where the filenames end up.
>
> goinstall github.com/niemeyer/shutdown
>
>
> http://codereview.appspot.com/4248041/
>

[rog, 2011-02-25 15:15:18]

Hello rsc, niemeyer (cc: golang-dev@googlegroups.com),

Please take another look.

[niemeyer, 2011-02-25 15:21:01]

> are there any more potentially dodgy bash metacharacters?
> i'm not that familiar with bash.

Just reminded of another one: ;

These work with pretty much any shell.

[niemeyer, 2011-02-25 15:27:33]

> Just reminded of another one: ;

And: [({|&<>!*

What a can of worms.  I suggest reversing the approach and
whitelisting what is allowed instead.  Reasonable filenames
aren't hard to describe.

[rsc, 2011-02-25 15:31:04]

Isn't this problem with make, not the rest of goinstall?
If so, shoudn't the fix be in writeMakefile?

Russ

[rog, 2011-02-25 15:49:23]

the question is where to stop the rot.

i see three possibilities:

1) a template formatter that flags an error on invalid names (awkward
because of the way that templates work - it's difficult to pass a
one-time value through to the formatter)

2) check when we add the names in makeMakefile; that's
conceptually nicest, but we'll have to do the check three times.

3) check when the directory is scanned. least code, but
it's second-guessing the makefile writer.

i'm not sure about whitelisting - i think it's easier to blacklist
but YMMV.

[rog, 2011-02-25 16:01:06]

PTAL

On 25 February 2011 15:49, roger peppe <rogpeppe@gmail.com> wrote:
> the question is where to stop the rot.
>
> i see three possibilities:
>
> 1) a template formatter that flags an error on invalid names (awkward
> because of the way that templates work - it's difficult to pass a
> one-time value through to the formatter)
>
> 2) check when we add the names in makeMakefile; that's
> conceptually nicest, but we'll have to do the check three times.
>
> 3) check when the directory is scanned. least code, but
> it's second-guessing the makefile writer.
>
> i'm not sure about whitelisting - i think it's easier to blacklist
> but YMMV.
>

[niemeyer, 2011-02-25 16:16:48]

> i'm not sure about whitelisting - i think it's easier to blacklist
> but YMMV.

What do you mean by easier?  This whole thread is based on the fact
you are blacklisting and have no idea about what to blacklist.

[rog, 2011-02-25 16:23:08]

On 25 February 2011 16:16,  <n13m3y3r@gmail.com> wrote:
>> i'm not sure about whitelisting - i think it's easier to blacklist
>> but YMMV.
>
> What do you mean by easier?  This whole thread is based on the fact
> you are blacklisting and have no idea about what to blacklist.

i agree, but perhaps a list of shell and make metacharacters
(a known quantity, even if i don't know it) is easier to
manage than a list of all possible valid pathname characters.
for example, do you accept all unicode characters, or provide a list
of acceptable unicode ranges?

[niemeyer, 2011-02-25 17:09:31]

> for example, do you accept all unicode characters, or provide a list
> of acceptable unicode ranges?

How about this:

func goodName(filename string) bool {
    for _, c := range filename {
        if !unicode.IsLetter(c) && c != '.' && c != '_' && c != '-' {
            return false
        }
    }
    return true
}

[niemeyer, 2011-02-25 17:11:12]

Sorry, digits are obviously missing, but you get the idea.

[rog, 2011-02-25 17:17:37]

On 25 February 2011 17:09,  <n13m3y3r@gmail.com> wrote:
>> for example, do you accept all unicode characters, or provide a list
>> of acceptable unicode ranges?
>
> How about this:
>
> func goodName(filename string) bool {
>    for _, c := range filename {
>        if !unicode.IsLetter(c) && c != '.' && c != '_' && c != '-' {
>            return false
>        }
>    }
>    return true
> }

that's pretty restrictive and would amount to a language change.
you'd need to allow '/' too (the whole package path needs to be checked).
digits would be ok too.

if we're going to do this, it should be documented.

[niemeyer, 2011-02-25 17:21:25]

> that's pretty restrictive and would amount to a language change.

Yes, it is pretty restrictive to reasonable filenames.  But no,
it's not a language change at all.

> if we're going to do this, it should be documented.

Sounds good.

[rsc, 2011-02-25 17:26:25]

we seem to be falling into lots of bikeshed discussions recently.
to end this one, here is the function to use:

var safeBytes =
"+-./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"

func safeName(b []byte) bool {
	for _, c := range b {
		if c < 0x80 && strings.IndexByte(safeBytes, c) < 0 {
			return false
		}
	}
	return true
}

[rog, 2011-03-01 12:48:51]

PTAL

On 2011/02/25 17:26:25, rsc wrote:
> "+-./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"

i might be inclined to add ':', '@' and ',' to that list.

[rsc, 2011-03-02 19:59:56]

LGTM

[rsc, 2011-03-02 20:04:10]

*** Submitted as c4a961e2e4bc ***

goinstall: protect against malicious filenames.
It was possible to make package run arbitrary
commands when installing if its filenames contained
make metacharacters.

R=rsc, niemeyer
CC=golang-dev
http://codereview.appspot.com/4248041

Committer: Russ Cox <rsc@golang.org>