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

x/crypto/ssh: make ClientConfig HostKeyCallback non-permissive by default #19767

Closed
rsc opened this Issue Mar 29, 2017 · 9 comments

Comments

Projects
None yet
8 participants
@rsc
Contributor

rsc commented Mar 29, 2017

ClientConfig.HostKeyCallback interprets nil as "accept any host keys". This is not a great default from a security perspective. Many clients probably should set HostKeyCallback to something real but are not.

It was written this way in golang.org/cl/9922043 to preserve backwards compatibility with the original implementation, but that was probably not the right balance to strike.

This issue is to make HostKeyCallback=nil mean "reject all host keys" and at the same time provide at least

func InsecureSkipVerifyHostKey() HostKeyChecker
func FixedHostKey(key []byte) HostKeyChecker

and maybe also

func HostKeysFile(file string) HostKeyChecker

Thanks to Phil Pennock for pointing out this problem.

@rsc rsc added this to the Unreleased milestone Mar 29, 2017

@bradfitz bradfitz added the Security label Mar 29, 2017

@hanwen

This comment has been minimized.

Contributor

hanwen commented Mar 29, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Mar 29, 2017

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

@philpennock

This comment has been minimized.

philpennock commented Apr 3, 2017

For reference, this is now: CVE-2017-3204

@muratsplat

This comment has been minimized.

muratsplat commented Apr 13, 2017

Hello,

This issue not occurred on "go1.8.1 darwin/amd64". I think that "go1.8.1 darwin/amd64" and " go1.8.1 linux/amd64" is not same in ssh package. Can "go1.8.1 darwin/amd64" commits be behind to " go1.8.1 linux/amd64". It is possible @rsc

@muratsplat

This comment has been minimized.

muratsplat commented Apr 13, 2017

Hello again.

I forget do "go get -u github.com/pkg/sftp" with "-u" flag. Sorry for attention :(

brikis98 added a commit to gruntwork-io/terratest that referenced this issue Apr 16, 2017

Add new required SSH param HostKeyCallback
A security vulnerability was found in Go
(golang/go#19767) where its SSH library did
not do host key checking by default. This was fixed
(https://go-review.googlesource.com/c/38701/) and the fix is (by
design) backwards incompatible in that you are now required to pass a
HostKeyCallback param.

xiu added a commit to xiu/sftp that referenced this issue Apr 26, 2017

bucklander pushed a commit to bucklander/go-netconf that referenced this issue May 11, 2017

@CameronGo

This comment has been minimized.

CameronGo commented May 15, 2017

I had a question about the approach used to solve this. I understand the reason for the breaking change, but I'm wondering if there is a way to implement this so that the "break" occurs during build as opposed to at run time. That would lessen the impact and the surprises that come up after a build was successful and then the first run in production fails for some error that you've never seen before.

Thoughts?

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 15, 2017

@CameronGo, had there been a nice solution with that property (failing at compile time instead of run time), we would've used it. But e.g. renaming renaming ssh.ClientConfig to ssh.ClientConfig2 would've been super gross.

@CameronGo

This comment has been minimized.

CameronGo commented May 15, 2017

Understood - (and agreed on the latter point for sure).

astj added a commit to mackerelio/go-check-plugins that referenced this issue May 16, 2017

mislavn pushed a commit to sartura/go-netconf that referenced this issue May 19, 2017

tg123 added a commit to tg123/sshpiper that referenced this issue Jun 20, 2017

ssh: require host key checking in the ClientConfig
This change breaks existing behavior.

Before, a missing ClientConfig.HostKeyCallback would cause host key
checking to be disabled. In this configuration, establishing a
connection to any host just works, so today, most SSH client code in
the wild does not perform any host key checks.

This makes it easy to perform a MITM attack:

* SSH installations that use keyboard-interactive or password
authentication can be attacked with MITM, thereby stealing
passwords.

* Clients that use public-key authentication with agent forwarding are
also vulnerable: the MITM server could allow the login to succeed, and
then immediately ask the agent to authenticate the login to the real
server.

* Clients that use public-key authentication without agent forwarding
are harder to attack unnoticedly: an attacker cannot authenticate the
login to the real server, so it cannot in general present a convincing
server to the victim.

Now, a missing HostKeyCallback will cause the handshake to fail. This
change also provides InsecureIgnoreHostKey() and FixedHostKey(key) as
ready made host checkers.

A simplistic parser for OpenSSH's known_hosts file is given as an
example.  This change does not provide a full-fledged parser, as it
has complexity (wildcards, revocation, hashed addresses) that will
need further consideration.

When introduced, the host checking feature maintained backward
compatibility at the expense of security. We have decided this is not
the right tradeoff for the SSH library.

Fixes golang/go#19767

Change-Id: I45fc7ba9bd1ea29c31ec23f115cdbab99913e814
Reviewed-on: https://go-review.googlesource.com/38701
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

ipcjk added a commit to ipcjk/mlxsh that referenced this issue Jul 15, 2017

sfreiberg added a commit to sfreiberg/simplessh that referenced this issue Jul 19, 2017

mylxsw added a commit to mylxsw/remote-tail that referenced this issue Aug 30, 2017

doodles526 added a commit to doodles526/wormhole that referenced this issue Oct 4, 2017

local/ssh_handler: fix bad ssh config
Go had a security bug with verifying host keys. For now we are simply
wildcarding to remain compatible with go 1.8+. But in the future we should confirm these
golang/go#19767

@doodles526 doodles526 referenced this issue Oct 4, 2017

Closed

Fix ssh #16

0 of 3 tasks complete

doodles526 added a commit to doodles526/wormhole that referenced this issue Oct 4, 2017

local/ssh: Fix ssh bug
In accordance to golang/go#19767 we are having
wildcard host to fix the issue. We will change to proper host
verification in the future

doodles526 added a commit to doodles526/wormhole that referenced this issue Oct 4, 2017

local/ssh: Fix ssh bug
In accordance to golang/go#19767 we are having
wildcard host to fix the issue. We will change to proper host
verification in the future

@doodles526 doodles526 referenced this issue Oct 4, 2017

Merged

Fix ssh #17

3 of 3 tasks complete

mbyczkowski added a commit to superfly/wormhole that referenced this issue Oct 4, 2017

Fix ssh (#17)
* local/ssh: Fix ssh bug

In accordance to golang/go#19767 we are having
wildcard host to fix the issue. We will change to proper host verification in the future.
@rite2nikhil

This comment has been minimized.

rite2nikhil commented Nov 29, 2017

How can agent forwarding be supported this this case, the mux ssh server
client -> mux -> remote

@Ajibola Ajibola referenced this issue Nov 30, 2017

Merged

Cleanup imports #1

robermorales added a commit to robermorales/sshmux that referenced this issue Feb 6, 2018

Adding default on HostKeyCallback
Since this issue golang/go#19767 the default behaviour of HostKeyCallback changed, so the old behaviour can be achieved with this change.

kennylevinsen added a commit to kennylevinsen/sshmux that referenced this issue Feb 6, 2018

Adding default on HostKeyCallback
Since this issue golang/go#19767 the default behaviour of HostKeyCallback changed, so the old behaviour can be achieved with this change.

omunroe-com pushed a commit to omunroe-com/runnr.glab that referenced this issue Oct 30, 2018

@golang golang locked and limited conversation to collaborators Nov 29, 2018

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