Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/go: cgo flags drops quotes. #12281

Open
omeid opened this issue Aug 23, 2015 · 10 comments
Open

cmd/go: cgo flags drops quotes. #12281

omeid opened this issue Aug 23, 2015 · 10 comments
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@omeid
Copy link

omeid commented Aug 23, 2015

There doesn't seem to be a way to pass a quoted value with cgo flags.

package libsass

/*
#cgo CFLAGS: -D VERSION="3.2.4-0ae11a4"


char* version(void) {
  return VERSION;
}
*/
import "C"

func Version() string {
    return C.GoString(C.version())
}
$ go build -x -v
WORK=/tmp/go-build030587731
test/cgoflag
mkdir -p $WORK/test/cgoflag/_obj/
mkdir -p $WORK/test/cgoflag/_obj/exe/
cd /home/ome/go/src/test/cgoflag
CGO_LDFLAGS="-g" "-O2" /usr/local/go/pkg/tool/linux_amd64/cgo -objdir $WORK/test/cgoflag/_obj/ -importpath test/cgoflag -- -I $WORK/test/cgoflag/_obj/ -D VERSION=3.2.4-0ae11a4 main.go
# test/cgoflag
./main.go: In function 'version':
<command-line>:0:9: error: too many decimal points in number
./main.go:8:10: note: in expansion of macro 'VERSION'
   return VERSION;
          ^
<command-line>:0:15: error: invalid suffix "ae11a4" on integer constant
./main.go:8:10: note: in expansion of macro 'VERSION'
   return VERSION;

I have also tried '"3.2.4-0ae11a4"' and some other variations, they all yield:
can't load package: package test/cgoflag: /home/ome/go/src/test/cgoflag/main.go: malformed #cgo argument: VERSION="3.2.4-0ae11a4"
@ianlancetaylor ianlancetaylor changed the title cmd/compile: cgo flags drops quotes. cmd/go: cgo flags drops quotes. Aug 25, 2015
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 25, 2015
@c4milo
Copy link
Member

c4milo commented Sep 21, 2015

I'm running into this same issue.

@juamedgod juamedgod mentioned this issue Jan 24, 2017
Open
@zengming00
Copy link

I had the same problem, When I try to use quickjs
quickjs has a macro CONFIG_VERSION is a string, Using the #cgo directives is always wrong, I made a successful attempt with the CGO_CFLAGS environment, but this may not be a good approach

CGO_CFLAGS="-DCONFIG_VERSION=\"1.0.0\"" go build

@omeid
Copy link
Author

omeid commented Dec 22, 2020

@ianlancetaylor Would you accept a fix for this? I would much rather not having to use a bash/makefile to just pass a CFLAG.

@ianlancetaylor
Copy link
Member

I don't know what the fix would look like, but, yes, in principle, a fix for this would be fine (for the 1.17 release).

@omeid
Copy link
Author

omeid commented Dec 22, 2020

I believe the bug is in go/build.expandSrcDir, I will investigate.

go/src/go/build/build.go

Lines 1650 to 1660 in bc7e4d9

args, err := splitQuoted(argstr)
if err != nil {
return fmt.Errorf("%s: invalid #cgo line: %s", filename, orig)
}
var ok bool
for i, arg := range args {
if arg, ok = expandSrcDir(arg, di.Dir); !ok {
return fmt.Errorf("%s: malformed #cgo argument: %s", filename, arg)
}
args[i] = arg
}

@omeid
Copy link
Author

omeid commented Dec 22, 2020

Okay, it seems like safeCgoName does not allow quote marks, even though the function that splits cflags, splitQuoted, is quotes aware as the name suggests.

The proper solution would be to make safeCgoName quote aware, in that it should only consider a string valid if there is a matching closing quote for any opened?

go/src/go/build/build.go

Lines 1740 to 1759 in bc7e4d9

// NOTE: $ is not safe for the shell, but it is allowed here because of linker options like -Wl,$ORIGIN.
// We never pass these arguments to a shell (just to programs we construct argv for), so this should be okay.
// See golang.org/issue/6038.
// The @ is for OS X. See golang.org/issue/13720.
// The % is for Jenkins. See golang.org/issue/16959.
// The ! is because module paths may use them. See golang.org/issue/26716.
// The ~ and ^ are for sr.ht. See golang.org/issue/32260.
const safeString = "+-.,/0123456789=ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz:$@%! ~^"
func safeCgoName(s string) bool {
if s == "" {
return false
}
for i := 0; i < len(s); i++ {
if c := s[i]; c < utf8.RuneSelf && strings.IndexByte(safeString, c) < 0 {
return false
}
}
return true
}

@ianlancetaylor
Copy link
Member

I think we need a clearer definition of what is safe and what is not. safeCgoName was originally intended to only allow obviously safe constructs. We've gotten farther and farther away from that over time. Permitting pairs of quotation marks seems fairly safe, until we start permitting backslashes.

On the other hand, safeCgoName precedes the code in cmd/go/internal/work/security.go, and perhaps we can delegate to that. I'm not sure offhand.

The attack we are protecting against is someone who provides a package with a #cgo CFLAGS line that includes arguments that cause arbitrary execution on the machine of the person running go get.

@omeid
Copy link
Author

omeid commented Mar 28, 2023

Based on dba926d, I guess " should just go in the safeString and we can call it a day.

@omeid
Copy link
Author

omeid commented Mar 28, 2023

cc @rsc

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 13, 2024
@bminer
Copy link

bminer commented Nov 21, 2024

I know that in the past, parentheses have been deemed unsafe, as well. See #23911. I feel like there should be a better way to sanitize these strings. Thoughts?

@seankhliao seankhliao added the GoCommand cmd/go label Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@c4milo @bminer @omeid @ianlancetaylor @zengming00 @seankhliao