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: go get should require a command-line flag for insecure protocols #9637

Closed
dchest opened this Issue Jan 19, 2015 · 23 comments

Comments

Projects
None yet
10 participants
@dchest
Contributor

dchest commented Jan 19, 2015

Currently it is impossible to securely fetch a package via go get, because it downloads via an insecure protocol if secure protocols fail or are not available. It is possible for an attacker to block secure ports (443 for HTTPS and 22 for git+ssh) and serve malicious packages via plain-text HTTP or git protocols.

I propose making it an explicit option to enable insecure protocols, e.g. -allow-insecure.

As a compromise (e.g. for backwards compatibility), -secure option may be implemented instead, making insecure behavior the default, but allowing users to add the flag to disable plain-text protocols.

@0xdabbad00

This comment has been minimized.

Show comment
Hide comment
@0xdabbad00

0xdabbad00 Jan 19, 2015

Looks like the fix for this would be to remove "http" from

scheme: []string{"git", "https", "http", "git+ssh"},
and similar lines. Add an insecure_scheme variable to those and handle accordingly.

Also would need to do something for the fallback here:

urlStr, body, err := httpsOrHTTP(importPath)

0xdabbad00 commented Jan 19, 2015

Looks like the fix for this would be to remove "http" from

scheme: []string{"git", "https", "http", "git+ssh"},
and similar lines. Add an insecure_scheme variable to those and handle accordingly.

Also would need to do something for the fallback here:

urlStr, body, err := httpsOrHTTP(importPath)

@dchest

This comment has been minimized.

Show comment
Hide comment
@dchest

dchest Jan 19, 2015

Contributor

Here's an example of man-in-the-middle attack with blocked HTTPS and modified HTTP response:

~ $ go get -v golang.org/x/crypto/pbkdf2
Fetching https://golang.org/x/crypto/pbkdf2?go-get=1
https fetch failed.
Fetching http://golang.org/x/crypto/pbkdf2?go-get=1
Parsing meta tags from http://golang.org/x/crypto/pbkdf2?go-get=1 (status code 200)
get "golang.org/x/crypto/pbkdf2": found meta tag main.metaImport{Prefix:"golang.org/x/crypto", VCS:"git",
 RepoRoot:"https://example.org/MALICIOUS_REPO.git"} at http://golang.org/x/crypto/pbkdf2?go-get=1
Contributor

dchest commented Jan 19, 2015

Here's an example of man-in-the-middle attack with blocked HTTPS and modified HTTP response:

~ $ go get -v golang.org/x/crypto/pbkdf2
Fetching https://golang.org/x/crypto/pbkdf2?go-get=1
https fetch failed.
Fetching http://golang.org/x/crypto/pbkdf2?go-get=1
Parsing meta tags from http://golang.org/x/crypto/pbkdf2?go-get=1 (status code 200)
get "golang.org/x/crypto/pbkdf2": found meta tag main.metaImport{Prefix:"golang.org/x/crypto", VCS:"git",
 RepoRoot:"https://example.org/MALICIOUS_REPO.git"} at http://golang.org/x/crypto/pbkdf2?go-get=1
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 19, 2015

Member

I agree we should do this. The Java/Maven community went through this last year, too.

I also knew somebody who was given an offer to avoid jail time by instead writing active MITM tools to replace downloaded software over http. And that was 15+ years ago.

But I too am belaboring the point. Now that I'm also at fault for voting, let's stop adding votes to this bug. Votes can go here: support@github.com and tell them they should add silent voting support for Github Issues.

The obvious counter-argument to doing this is compatibility and user confusion, but I think a new flag to proceed anyway, and proper error messages would suffice. The other argument is less complexity and fewer flags, but I think security wins that argument.

FWIW, every notable code hosting side I can think of does TLS. As for discovery on custom import paths, camlistore.org is TLS (so I don't care myself), golang.org is TLS, but https://rsc.io/x86/x86asm isn't up, so anything importing @rsc's stuff with this would need a flag to bypass. I think having a TLS cert is a valid burden to pay for having a custom import path.

I am not a decider, though. I will mark this 1.5 for at least a decision either way.

/cc @adg, @rsc, @robpike, @ianlancetaylor

Member

bradfitz commented Jan 19, 2015

I agree we should do this. The Java/Maven community went through this last year, too.

I also knew somebody who was given an offer to avoid jail time by instead writing active MITM tools to replace downloaded software over http. And that was 15+ years ago.

But I too am belaboring the point. Now that I'm also at fault for voting, let's stop adding votes to this bug. Votes can go here: support@github.com and tell them they should add silent voting support for Github Issues.

The obvious counter-argument to doing this is compatibility and user confusion, but I think a new flag to proceed anyway, and proper error messages would suffice. The other argument is less complexity and fewer flags, but I think security wins that argument.

FWIW, every notable code hosting side I can think of does TLS. As for discovery on custom import paths, camlistore.org is TLS (so I don't care myself), golang.org is TLS, but https://rsc.io/x86/x86asm isn't up, so anything importing @rsc's stuff with this would need a flag to bypass. I think having a TLS cert is a valid burden to pay for having a custom import path.

I am not a decider, though. I will mark this 1.5 for at least a decision either way.

/cc @adg, @rsc, @robpike, @ianlancetaylor

@bradfitz bradfitz added this to the Go1.5 milestone Jan 19, 2015

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jan 19, 2015

Contributor

If we're going to do this, and I think we should, we should do it sooner rather than later. We need to give people with custom domains ample warning to install SSL certs.

We could also add a warning to godoc.org for packages with custom domains that cannot be resolved using HTTPS.

Contributor

adg commented Jan 19, 2015

If we're going to do this, and I think we should, we should do it sooner rather than later. We need to give people with custom domains ample warning to install SSL certs.

We could also add a warning to godoc.org for packages with custom domains that cannot be resolved using HTTPS.

@garyburd

This comment has been minimized.

Show comment
Hide comment
@garyburd

garyburd Jan 19, 2015

Contributor

Approximately 40 of the domains known to godoc.org are not listening on 443 or have an SSL certificate problem.

Contributor

garyburd commented Jan 19, 2015

Approximately 40 of the domains known to godoc.org are not listening on 443 or have an SSL certificate problem.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Jan 19, 2015

Member

I agree that go get should stop if the custom domain does not support
https, but do we need another flag? -f seems fine to override the default.

We will need to announce this well before 1.5 release.

Member

minux commented Jan 19, 2015

I agree that go get should stop if the custom domain does not support
https, but do we need another flag? -f seems fine to override the default.

We will need to announce this well before 1.5 release.

@dchest

This comment has been minimized.

Show comment
Hide comment
@dchest

dchest Jan 19, 2015

Contributor

@minux but what will be the meaning of -f flag then?

The -f flag, valid only when -u is set, forces get -u not to verify that
each package has been checked out from the source control repository
implied by its import path. This can be useful if the source is a local fork
of the original. Also, it enables insecure protocols when fetching packages.

The point of a separate flag with clear meaningful name is to point out insecurity (or rather, make it scream). Similar to InsecureSkipVerify in crypto/tls package, not just Force or SkipVerify , or hg push --insecure in Mercurial, or dangerouslySetInnerHTML in React.js.

Agreed on announcement before release.

Contributor

dchest commented Jan 19, 2015

@minux but what will be the meaning of -f flag then?

The -f flag, valid only when -u is set, forces get -u not to verify that
each package has been checked out from the source control repository
implied by its import path. This can be useful if the source is a local fork
of the original. Also, it enables insecure protocols when fetching packages.

The point of a separate flag with clear meaningful name is to point out insecurity (or rather, make it scream). Similar to InsecureSkipVerify in crypto/tls package, not just Force or SkipVerify , or hg push --insecure in Mercurial, or dangerouslySetInnerHTML in React.js.

Agreed on announcement before release.

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jan 19, 2015

Contributor

On 20 January 2015 at 09:04, Minux Ma notifications@github.com wrote:

-f seems fine to override the default.

I don't think so. The option to fetch packages insecurely should be its own
unique option. People shouldn't lose security as a side effect.

Contributor

adg commented Jan 19, 2015

On 20 January 2015 at 09:04, Minux Ma notifications@github.com wrote:

-f seems fine to override the default.

I don't think so. The option to fetch packages insecurely should be its own
unique option. People shouldn't lose security as a side effect.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Jan 19, 2015

Member

I mean https should be the default. Any problems accessing the https (but
http works) should fail the entire go get process.

-f forces it to continue even if https is not available.

Member

minux commented Jan 19, 2015

I mean https should be the default. Any problems accessing the https (but
http works) should fail the entire go get process.

-f forces it to continue even if https is not available.

@dchest

This comment has been minimized.

Show comment
Hide comment
@dchest

dchest Jan 19, 2015

Contributor

@minux yes, I understand. However, -f is already used for something else, so this flag should be named differently, and include some dangerously sounding word, such as insecure.

Contributor

dchest commented Jan 19, 2015

@minux yes, I understand. However, -f is already used for something else, so this flag should be named differently, and include some dangerously sounding word, such as insecure.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 19, 2015

Member

hg and curl use --insecure. Let's go with that. I agree we shouldn't overload -f, and -allow-insecure is just longer.

Member

bradfitz commented Jan 19, 2015

hg and curl use --insecure. Let's go with that. I agree we shouldn't overload -f, and -allow-insecure is just longer.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Jan 19, 2015

Member

I was thinking that we you use go get -u -f, you essentially make go get
not checking custom domains at all, so whether it enforces https is not
relevant. But then I realized that go get -u -f might still fetch
additional vanity import packages, so it seems you're right and we need to
introduce go get -unsafe instead.

Member

minux commented Jan 19, 2015

I was thinking that we you use go get -u -f, you essentially make go get
not checking custom domains at all, so whether it enforces https is not
relevant. But then I realized that go get -u -f might still fetch
additional vanity import packages, so it seems you're right and we need to
introduce go get -unsafe instead.

nodakai added a commit to nodakai/go that referenced this issue Mar 9, 2015

cmd/go: allow HTTP only under the new -insecure flag.
The flag propagates to go/http.go and go/vcs.go and changes their
behavior.  It also shortens the timeout threshold for HTTPS to 5 seconds
because the user expects it to fail from the beginning.

Fixes #9637, #10120

nodakai added a commit to nodakai/go that referenced this issue Mar 9, 2015

cmd/go: allow HTTP only under the new -insecure flag.
The flag propagates to go/http.go and go/vcs.go and changes their
behavior.  It also shortens the timeout threshold for HTTPS to conds
because the user expects it to fail from the beginning.

Fixes #9637, #10120

Change-Id: I61416cd453134062952225557dc5557c56f09761
@ioerror

This comment has been minimized.

Show comment
Hide comment
@ioerror

ioerror Mar 12, 2015

I was really surprised to learn that go get fails open - is there anyway to have this security critical issue backported to older versions of Go?

For those who use go over Tor - we're really in a bad way with this issue. :(

ioerror commented Mar 12, 2015

I was really surprised to learn that go get fails open - is there anyway to have this security critical issue backported to older versions of Go?

For those who use go over Tor - we're really in a bad way with this issue. :(

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Apr 7, 2015

Member

@ioerror, Go 1.5 should be a drop-in replacement for Go 1.4, like all Go 1.x releases so far. But distros could easily backport this change. It's beyond the scope of something we'd normally backport into our own stable releases, though. (which we stop maintaining entirely once Go 1.N+1 is out)

Member

bradfitz commented Apr 7, 2015

@ioerror, Go 1.5 should be a drop-in replacement for Go 1.4, like all Go 1.x releases so far. But distros could easily backport this change. It's beyond the scope of something we'd normally backport into our own stable releases, though. (which we stop maintaining entirely once Go 1.N+1 is out)

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Apr 7, 2015

Contributor

I agree we should do this. I just went through the whole process for moving rsc.io and 9fans.net to dedicated f1-micro VMs on Google Compute Engine with real SSL certs, and it was fairly straightforward. I'll post more later but rsc.io/go-import-redirector has details. We should probably announce this around the time of the freeze.

Contributor

rsc commented Apr 7, 2015

I agree we should do this. I just went through the whole process for moving rsc.io and 9fans.net to dedicated f1-micro VMs on Google Compute Engine with real SSL certs, and it was fairly straightforward. I'll post more later but rsc.io/go-import-redirector has details. We should probably announce this around the time of the freeze.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Apr 20, 2015

Contributor

I hadn't realized until just now, but it looks like there are two changes here:

(1) metaImportsForPrefix, which looks up tags, should require https by default.

(2) The vcs checkouts should also require https or ssh by default, so for example an hg clone would no longer try "http", and a git clone would no longer try "git" or "http", just "https" and "git+ssh".

To be very clear, we're talking about both (1) and (2), right? The discussion so far has focused only on (1), but without (2) it seems a bit useless.

Contributor

rsc commented Apr 20, 2015

I hadn't realized until just now, but it looks like there are two changes here:

(1) metaImportsForPrefix, which looks up tags, should require https by default.

(2) The vcs checkouts should also require https or ssh by default, so for example an hg clone would no longer try "http", and a git clone would no longer try "git" or "http", just "https" and "git+ssh".

To be very clear, we're talking about both (1) and (2), right? The discussion so far has focused only on (1), but without (2) it seems a bit useless.

@rsc rsc changed the title from cmd/go: insecure protocols in go get should require a command-line flag to cmd/go: go get should require a command-line flag for insecure protocols Apr 20, 2015

@dchest

This comment has been minimized.

Show comment
Hide comment
@dchest

dchest Apr 20, 2015

Contributor

@rsc correct, we're talking about both.

Contributor

dchest commented Apr 20, 2015

@rsc correct, we're talking about both.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Apr 25, 2015

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

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented May 5, 2015

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

@nealmcb

This comment has been minimized.

Show comment
Hide comment
@nealmcb

nealmcb May 25, 2015

Downloading packages over a secure connection is helpful, so I support this change. But note that it still leaves the user vulnerable to attacks on the servers, and online code servers are unfortunately vulnerable to a wide variety of insider and outsider attacks.

It is generally much better to provide public key signatures on the package itself, since the signing process can be done in a much more secure offline environment. With robust signing and verification, you wouldn't care how the package was distributed.

I don't see any evidence after a quick browse that go packages can be signed via gpg/pgp. Is that possible? Could it be required for official packages?

Of course the big problem with signatures is how to verify that a trusted key was used to generate the signature, since the web-of-trust model is unwieldy. But we're making progress on that front with the rollout of DNSSEC making portions of DNS a good place to securely retrieve keys from, and the gpg --pka-lookups and --pka-trust-increase options. See e.g. Publishing PGP Keys in DNS.

Has that been discussed? Does it merit a new issue?

nealmcb commented May 25, 2015

Downloading packages over a secure connection is helpful, so I support this change. But note that it still leaves the user vulnerable to attacks on the servers, and online code servers are unfortunately vulnerable to a wide variety of insider and outsider attacks.

It is generally much better to provide public key signatures on the package itself, since the signing process can be done in a much more secure offline environment. With robust signing and verification, you wouldn't care how the package was distributed.

I don't see any evidence after a quick browse that go packages can be signed via gpg/pgp. Is that possible? Could it be required for official packages?

Of course the big problem with signatures is how to verify that a trusted key was used to generate the signature, since the web-of-trust model is unwieldy. But we're making progress on that front with the rollout of DNSSEC making portions of DNS a good place to securely retrieve keys from, and the gpg --pka-lookups and --pka-trust-increase options. See e.g. Publishing PGP Keys in DNS.

Has that been discussed? Does it merit a new issue?

@dchest

This comment has been minimized.

Show comment
Hide comment
@dchest

dchest May 25, 2015

Contributor

@nealmcb go get downloads packages using Git/Mercurial/Subversion. At least two of them can sign and verify commits and tags with GPG, so if you solved the key distribution problem, it would be easy to write a simple wrapper around go get (name suggestion: gogpggetgood) which will download and verify GPG signatures (that's what I'm doing for StableLib, although I have no key distribution problem, as the repository is centralized).

Contributor

dchest commented May 25, 2015

@nealmcb go get downloads packages using Git/Mercurial/Subversion. At least two of them can sign and verify commits and tags with GPG, so if you solved the key distribution problem, it would be easy to write a simple wrapper around go get (name suggestion: gogpggetgood) which will download and verify GPG signatures (that's what I'm doing for StableLib, although I have no key distribution problem, as the repository is centralized).

@dchest

This comment has been minimized.

Show comment
Hide comment
@dchest

dchest Jul 8, 2015

Contributor

@adg this change probably should be mentioned in released notes for Go 1.5

Contributor

dchest commented Jul 8, 2015

@adg this change probably should be mentioned in released notes for Go 1.5

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jul 14, 2015

Contributor

@dchest it's in the docs: "The get subcommand now has a -insecure flag that must be enabled if fetching from an insecure repository, one that does not encrypt the connection."

Contributor

adg commented Jul 14, 2015

@dchest it's in the docs: "The get subcommand now has a -insecure flag that must be enabled if fetching from an insecure repository, one that does not encrypt the connection."

@golang golang locked and limited conversation to collaborators Jul 13, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.