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: CertChecker incorrectly includes port number in hostname as part of checked principal #20273
Comments
I think this patch would fix it: diff --git a/ssh/certs.go b/ssh/certs.go
index 2fc8af1..33af6d0 100644
--- a/ssh/certs.go
+++ b/ssh/certs.go
@@ -299,7 +299,12 @@ func (c *CertChecker) CheckHostKey(addr string, remote net.Addr, key PublicKey)
return fmt.Errorf("ssh: certificate presented as a host key has type %d", cert.CertType)
}
- return c.CheckCert(addr, cert)
+ host, _, err := net.SplitHostPort(addr)
+ if err != nil {
+ return err
+ }
+
+ return c.CheckCert(host, cert)
}
// Authenticate checks a user certificate. Authenticate can be used as |
as I mentioned on the openssh-dev list, i'm pretty sure openssh is being more permissive than it should be, or at least more permissive than the manpage suggests it should be.
eg, if I have a known_hosts file with
and I run
I see the following
it does that no matter the order of specific/non-specific keys in /etc/ssh/ssh_known_hosts. that seems wrong. |
should be fixed by https://go-review.googlesource.com/c/43551/ |
@pmoody- , thanks for that. Some comments on that patch:
Would it be better to move the (and then also move the |
maybe long term such a refactor might make sense, but I don't think it makes sense to fix this particular issue (which that diff shows is fixed in ~20 lines, including tests). |
Hi Peter, the refactoring I mean is pretty minor - it moves the host specific cert checking to the See below for my suggested refactor (though noting any of these are still breaking changes if clients are using this). diff --git a/ssh/certs.go b/ssh/certs.go
index 2fc8af1..c30e716 100644
--- a/ssh/certs.go
+++ b/ssh/certs.go
@@ -298,8 +298,17 @@ func (c *CertChecker) CheckHostKey(addr string, remote net.Addr, key PublicKey)
if cert.CertType != HostCert {
return fmt.Errorf("ssh: certificate presented as a host key has type %d", cert.CertType)
}
+ if !c.IsHostAuthority(cert.SignatureKey, addr) {
+ return fmt.Errorf("ssh: no authorities for hostname: %v", addr)
+ }
+
+ host, _, err := net.SplitHostPort(addr)
+ if err != nil {
+ return err
+ }
- return c.CheckCert(addr, cert)
+ // Pass host as principal for host certificates
+ return c.CheckCert(host, cert)
}
// Authenticate checks a user certificate. Authenticate can be used as
@@ -316,6 +325,9 @@ func (c *CertChecker) Authenticate(conn ConnMetadata, pubKey PublicKey) (*Permis
if cert.CertType != UserCert {
return nil, fmt.Errorf("ssh: cert has type %d", cert.CertType)
}
+ if !c.IsUserAuthority(cert.SignatureKey) {
+ return nil, fmt.Errorf("ssh: certificate signed by unrecognized authority")
+ }
if err := c.CheckCert(conn.User(), cert); err != nil {
return nil, err
@@ -364,16 +376,6 @@ func (c *CertChecker) CheckCert(principal string, cert *Certificate) error {
}
}
- // if this is a host cert, principal is the remote hostname as passed
- // to CheckHostCert.
- if cert.CertType == HostCert && !c.IsHostAuthority(cert.SignatureKey, principal) {
- return fmt.Errorf("ssh: no authorities for hostname: %v", principal)
- }
-
- if cert.CertType == UserCert && !c.IsUserAuthority(cert.SignatureKey) {
- return fmt.Errorf("ssh: certificate signed by unrecognized authority")
- }
-
clock := c.Clock
if clock == nil {
clock = time.Now |
¯\_(ツ)_/¯ i'm not sure I see the benefit over 43551 but if you feel strongly you can sign the cla and send a diff. |
Hi Peter, thanks and here's my first foray into Gerrit: |
For reference,
I think that makes it clear that the certificate principal evaluation should be done on based on the hostname only, and I think the reflects the current OpenSSH behavior, so I'll update the title of this issue to reflect. @pmoody-, I don't think that has any bearing on what |
my objection from the beginning was that your proposed patch broke the recently added known hosts restrictions. i created a diff that addressed this issue which didn't break the know hosts validations, then you did too. cool, I'm happy. so i'm not sure what you need at this point, adam. |
I'm happy too - I just wanted to take the opportunity to link to the actual protocol spec, as I hadn't come across it earlier, and I thought it was worth being explicit that Now we just need the patch to land, and I can see the review is underway, so I think we are all happy. |
glad you were able to refactor my patch. |
Looks like the patch landed in golang/crypto@7e91053. Closing this issue. Thanks for your help Peter. |
What version of Go are you using (
go version
)?go version go1.7.4 darwin/amd64
Updated to latest version of
x/crypto/ssh
today.What did you do?
When using
ssh.Dial()
the address is of the formserver:port
.If the client is configured with a
HostKeyCallback
of typessh.CertChecker
, the SSH handshake fails due to the principal being checked being the address passed tossh.Dial()
, inclusive of the port, whereas I believe the normal usage for a host certificate is to include only the hostname.e.g. the docs for OpenSSH suggest that the principals are user or host names. There is no mention there, nor have I seen in the wild, any examples that include the port as part of the hostname:
https://www.freebsd.org/cgi/man.cgi?query=ssh-keygen&sektion=1&manpath=OpenBSD#CERTIFICATES
To see an end to end example that demonstrates the bug, see this code:
https://github.com/continusec/certcheckerbug/blob/master/cmd/certcheckerbug/main-certcheckerbug.go
What did you expect to see?
What did you see instead?
The text was updated successfully, but these errors were encountered: