code review 4532092: cgo: restrict #cgo directives to prevent shell expansion

cgo: restrict #cgo directives to prevent shell expansion

Fixes issue #1879.

Directives were not directly expanded, but since their
content ended up in makefiles, further expansion would
take place there.  This prevents such artifacts by
restricting the set of characters that may be used in
a directive value.

To build the list of safe characters I went through the
contents of /usr/lib/pkgconfig and extracted LDFLAGS
and CFLAGS information, so hopefully this is a
reasonable default to get started.

[niemeyer, 2011-05-27 02:02:44]

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

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

[rsc, 2011-05-27 03:18:13]

Lgtm
On May 26, 2011 10:02 PM, <n13m3y3r@gmail.com> wrote:
> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cgo: restrict #cgo directives to prevent shell expansion
>
> Fixes issue #1879.
>
> Directives were not directly expanded, but since their
> content ended up in makefiles, further expansion would
> take place there. This prevents such artifacts by
> restricting the set of characters that may be used in
> a directive value.
>
> To build the list of safe characters I went through the
> contents of /usr/lib/pkgconfig and extracted LDFLAGS
> and CFLAGS information, so hopefully this is a
> reasonable default to get started.
>
> Please review this at http://codereview.appspot.com/4532092/
>
> Affected files:
> M src/cmd/cgo/gcc.go
>
>
> Index: src/cmd/cgo/gcc.go
> ===================================================================
> --- a/src/cmd/cgo/gcc.go
> +++ b/src/cmd/cgo/gcc.go
> @@ -104,6 +104,11 @@
> if err != nil {
> fatalf("%s: bad #cgo option %s: %s", srcfile, k, err)
> }
> + for _, arg := range args {
> + if !safeName(arg) {
> + fatalf("%s: #cgo option %s is unsafe: %s", srcfile, k, arg)
> + }
> + }
>
> switch k {
>
> @@ -144,7 +149,7 @@
> // for packages.
> func pkgConfig(packages []string) (cflags, ldflags []string, err os.Error)

> {
> for _, name := range packages {
> - if len(name) == 0 || !safeName(name) || name[0] == '-' {
> + if len(name) == 0 || name[0] == '-' {
> return nil, nil, os.NewError(fmt.Sprintf("invalid name: %q", name))
> }
> }
> @@ -231,7 +236,7 @@
> return args, err
> }
>
> -var safeBytes =
>
[]byte("+-./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz")
> +var safeBytes =
>
[]byte("+-.,/0123456789=ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz")
>
> func safeName(s string) bool {
> if s == "" {
>
>

[niemeyer, 2011-05-27 11:46:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=8e99d1651850 ***

cgo: restrict #cgo directives to prevent shell expansion

Fixes issue #1879.

Directives were not directly expanded, but since their
content ended up in makefiles, further expansion would
take place there.  This prevents such artifacts by
restricting the set of characters that may be used in
a directive value.

To build the list of safe characters I went through the
contents of /usr/lib/pkgconfig and extracted LDFLAGS
and CFLAGS information, so hopefully this is a
reasonable default to get started.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4532092