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.

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

cmd/go: should go install/get blacklist common binary names, such as go, rm, cp? #16601

Closed
sasha-s opened this issue Aug 4, 2016 · 10 comments
Closed
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@sasha-s
Copy link

sasha-s commented Aug 4, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.7rc5 darwin/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOHOSTARCH="amd64"
    GOHOSTOS="darwin"
    GOOS="darwin"
  3. What did you do?
    go install a binary named go.
  4. What did you expect to see?
    A warning.
  5. What did you see instead?
    go binary was populated in $GOPATH/bin.
    Since I have $GOPATH/bin at the end my $PATH I have not noticed anything.
    Some make files/buildscripts add inferred GOPATH/bin in front of $PATH, which leads to surprising behavior when they do go build later.

It seems to be a security risk: my system becomes vulnerable if I do go get targeting a bad package (or a good package that got hacked), even if I do not run/test the code.
Imagine malicious binary that intercepts regular go commands and injects some code.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 4, 2016

Are you proposing we hard-code a list of bad package names, or fail (since we don't do warnings) whenever the package being installed shadows something already in your path?

Both seem kinda gross.

Why did you install a binary named go? Were you tricked into it somehow? How?

I'm trying to understand the problem more.

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 4, 2016
@josharian josharian changed the title Should go install/get blacklist common binary names, such as go, rm, cp? cmd/go: should go install/get blacklist common binary names, such as go, rm, cp? Aug 4, 2016
@josharian
Copy link
Contributor

I'm not sure where I stand, but I can possibly see a case for a very small hard-coded list of bad package names. go, rm, and cp all seem like good candidates.

@ianlancetaylor
Copy link
Contributor

It seems to me that the bug is putting $GOPATH/bin on PATH before /bin:/usr/bin or for that matter $GOROOT/bin. Once you've done that you are in trouble and there is nothing reliable we can do to fix it--are we going to also block sed, awk, ls, all the programs that a Makefile might run?

If we are going to do something here, the PATH is the thing to tackle, though I don't know how. I don't think we should be in the business of telling people that they can not write a "rm" program in Go.

@sasha-s
Copy link
Author

sasha-s commented Aug 4, 2016

We have the following options (as I see it):

  1. for a small hard-coded list of bad binary names
    1. go prints a huge warning
    2. go refuses to install/get
  2. document it clearly that $GOPATH/bin should be the last in the $PATH (after /bin:/usr/bin and such)
  3. if $GOPATH/bin is before /bin:/usr/bin
  4. go prints a huge warning
  5. go refuses to install/get

Personally, I am in favor of doing 1.i, 2., and 3.i.


I was not tricked.
One night I was fooling around and wrote a quick and dirty something and named it go (without thinking), did go install and forgot about it.

It went unnoticed for months because my $GOPATH/bin is last in chain.
I saw weird behavior once when I was trying to build something (forgot what) using make, which (as I understand now) added $GOPATH/bin in the beginning of $PATH. I ruled it out as a buggy Makefile and ignored it.

I've been playing with cockroachdb recently, and the problem re-appeared.

See cockroachdb/cockroach#8325 for exact steps to reproduce (though I think it is clear what's going on).

The problematic scenario (as I see it):

  1. I do go install for a package that I am using often, but it got hacked.
    • it install malicious go in my $GOPATH/bin
  2. I notice suspicious diff and delete the bad code, not thinking about the implications of bad binary naming.
  3. I run make in a different, trustworthy project with a buggy Makefile, that puts $GOPATH/bin before everything else.
  4. My universe is collapsing.

@quentinmit
Copy link
Contributor

I don't think we should stop the user if they want to build "cp" with Go, nor do I think we should stop the user if they want to prefer that over their system cp. That's all up to the user. I'm fine with any form of warning, though.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 4, 2016

Go doesn't really do warnings, though.

I don't think we'll do anything here.

If you blindly compile & run a project from an upstream that's been hacked, all sorts of badness can happen. Shadowing something in your path is the least of your theoretical problems.

Happy to reopen if I'm missing something.

@bradfitz bradfitz closed this as completed Aug 4, 2016
@dlsniper
Copy link
Contributor

dlsniper commented Aug 4, 2016

Maybe this could be addressed in go vet or golint by adding an error to them if they detect such cases.

@sasha-s
Copy link
Author

sasha-s commented Aug 4, 2016

If you blindly compile & run a project from an upstream that's been hacked, all sorts of badness can happen. Shadowing something in your path is the least of your theoretical problems.

in my scenario I was blindly building from and upstream that's been hacked, but not running it.

@sasha-s
Copy link
Author

sasha-s commented Aug 5, 2016

@gopherbot
Copy link

CL https://golang.org/cl/25509 mentions this issue.

@golang golang locked and limited conversation to collaborators Aug 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

7 participants