Potential security risk for devs: Makefile adds $GOPATH/bin in the beginning of PATH

[sasha-s]

Problem is in

cockroach/Makefile

Line 42 in c1d97b7

export PATH := $(GOPATH)/bin:$(PATH)

1. Please supply the header (i.e. the first few lines) of your most recent
   log file for each node in your cluster. On most unix-based systems
   running with defaults, this boils down to the output of

   Did it on c1d97b7

2. Please describe the issue you observed:
   For a longer discussion see cmd/go: should go install/get blacklist common binary names, such as go, rm, cp? golang/go#16601

• What did you do?
  I happened to have a binary named go in my $GOPATH/bin.

Steps to reproduce:

mkdir tmp
cd tmp
export GOPATH=$PWD
go get -u github.com/cockroachdb/cockroach/...
mkdir bin
echo '#!/bin/sh
echo "ACHTUNG! $0 has been invoked!"
false
' > bin/go
chmod a+x bin/go
make

• What did you expect to see?
  build proceeding normally
• What did you see instead?

 make
Makefile:219: .bootstrap: No such file or directory
installing githooks/post-checkout
installing githooks/post-merge
installing githooks/post-rewrite
go get github.com/robfig/glock
ACHTUNG! /Users/sasha/tmp/bin/go has been invoked!
make: *** [../../../../bin/glock] Error 1

---

Imagine that the go binary I managed to get into my $GOPATH/bin was malicious.

It would go unnoticed (and harmless) until I try to build cokroach.

Possible solutions

1. get some help from go toolchain, to prevent a binary named go from getting into my $GOPATH/bin. See cmd/go: should go install/get blacklist common binary names, such as go, rm, cp? golang/go#16601
2. clone the repo and reset GOPATH before building (only building what is committed, ignoring the dirty state). Might be slow.
3. whitelist a small set of tools needed and have Makefile call them directly from $GOPATH/bin

[bdarnell]

It's extremely common for Go developers to have $GOPATH/bin in their path (I think I'm the only one on the team that doesn't do this automatically and has to get it from the Makefile). Do you have a particular attack scenario in mind in which malicious code could get into $GOPATH/bin but would only be at risk of being executed due to our setting of $PATH?

How would option #3 address the security concerns? If someone could get a malicious go binary into $GOPATH/bin, couldn't they just as easily do the same with something like glock or golint? If you can't trust $GOPATH/bin then you can't trust it, whether you're running specific binaries from it or putting it on your $PATH.

We'll have to stop using $GOPATH/bin in order to switch to vendoring (#4182), so we will be changing this at some point, but I'm having a hard time seeing a realistic security risk here.

[sasha-s]

The problem arises when $GOPATH/bin shadows /usr/bin, /usr/local/binetc.

Option 3. addresses the security concerns, because potentially bad binaries that live in $GOPATH/bin are never executed (as long as devs have export PATH=$PATH:$GOPATH/bin).

Rephrasing myself from golang/go#16601 (comment) (potential scenario):

1. I do go install for a package that I know and trust, 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.
   • It can sit there for weeks, harmless, because in my PATH $GOPATH/bin is last in chain.
3. I build cockroach using make
   • It does export PATH := $(GOPATH)/bin:$(PATH), which makes malicious code run.
4. My universe is collapsing.

[bdarnell]

If you do something like go install github.com/kisielk/errcheck, it can only install a binary named errcheck, not go. There's a risk in installing a path ending with /..., since that could install things under any name, but the risk there is in using .... (and even if $GOPATH/bin is the last thing in your path instead of the first, a typosquatting binary could wait for you to make a misspelling).

Option 3 protects against scenarios involving bad binaries installed under an unexpected name, but it doesn't help if the package is modified in place without changing its name.

If you go install a package you "know and trust", don't you intend to run it? If you're going to look at the diffs before running it, you should do so before you even install it: go get -d (to download without building), inspect the source, and then go install as a separate step.

[sasha-s]

If you go install a package you "know and trust", don't you intend to run it?
I intend to look at the diff before running it (or at least to have such an option).

go get -d (to download without building), inspect the source, and then go install as a separate step.
Yes, it is ideal and I can do that myself just fine, but it might be unrealistic to expect every developer do this.

I agree, the problem is with using ....
Good point about typo-squatting.

I just want to minimize the risk.
I think option 3. might be compelling because in this case cockroach would not be responsible for triggering a potential security issue for a developer. Yes, the said developer might have done some stupid stuff, but we can not educate everybody.

BTW we are using option 2. -- for different reasons (clean builds, pre-push git hooks).
Works great with vendoring.

[dt]

Now that we've switched to vendored sources, I've started looking at being more precise about when we install/update tools (e.g. #12029) and can also add where we install them to that effort. Most callsites should be pretty easy to switch to GOBIN=.../build/bin go install ./vendor/mytool and then running ./build/bin/mytool

[dianasaur323]

@dt do you think this is still an issue, and if so, what milestone should we put it under? I'm trying to get rid of the Q1 2017 milestone.

[dt]

@benesch is actively working on this actually!

[bdarnell]

@benesch You've fixed this, right?

[benesch]

Yep! We no longer add $GOPATH/bin to PATH. We instead add REPO_ROOT/bin, which is entirely under our control. Marking as fixed.