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

x/crypto/ssh: CertChecker incorrectly includes port number in hostname as part of checked principal #20273

Closed
aeijdenberg opened this issue May 7, 2017 · 13 comments
Milestone

Comments

@aeijdenberg
Copy link

@aeijdenberg aeijdenberg commented May 7, 2017

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 form server:port.

If the client is configured with a HostKeyCallback of type ssh.CertChecker, the SSH handshake fails due to the principal being checked being the address passed to ssh.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?

Server handshake successful
Client handshake successful

What did you see instead?

ssh: handshake failed: ssh: principal "localhost:2022" not in the set of valid principals for given certificate: ["localhost"]
@gopherbot gopherbot added this to the Unreleased milestone May 7, 2017
@aeijdenberg
Copy link
Author

@aeijdenberg aeijdenberg commented May 7, 2017

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

@pmoody- pmoody- commented May 15, 2017

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.

SSH_KNOWN_HOSTS FILE FORMAT :
  ...

  A hostname or address may optionally be enclosed within `[' and `]'
brackets then followed by `:' and a non-standard port number.

eg, if I have a known_hosts file with

@cert-authority [foo.bar.com]:1234 ssh-ed25519 <key not presented by foo.bar.com:1234>
@cert-authority foo.bar.com ssh-ed25519 <key presented by foo.bar.com:1234>

and I run

ssh -v foo.bar.com -p 1234

I see the following

  debug1: Host 'foo.bar.com' is known and matches the ED25519-CERT
host certificate.
  debug1: Found CA key in /etc/ssh/ssh_known_hosts:2
  debug1: found matching key w/out port

it does that no matter the order of specific/non-specific keys in /etc/ssh/ssh_known_hosts. that seems wrong.

@pmoody-
Copy link

@pmoody- pmoody- commented May 16, 2017

@aeijdenberg
Copy link
Author

@aeijdenberg aeijdenberg commented May 16, 2017

@pmoody- , thanks for that. Some comments on that patch:

  1. As far as I can tell, this equally effects SSH when running on the standard port, as ssh.Dial still wants a host:port string, and thus will pass host:port for the principal evaluation (meaning that a server will need a host:22 principal in their certificate). This doesn't affect the patch other than its description.

  2. The previous principal code checking has been in this codebase for at least 3 years (since import into this repo) - perhaps we should consider matching certificates with either principal for compatibility? (host or host:port?) I'm very surprised that no one else has reported this issue, so perhaps it simply hasn't been used?

  3. In terms of the code structure, it doesn't feel right to me that an argument named principal is treated as one that would normally be named addr in the first part of the function, and as a principal in the second. I realise other solutions would make a slightly bigger patch, but as it stands, it feels like it violates the principal of least surprise.

Would it be better to move the IsHostAuthority() check to CheckHostKey() (which already takes an addr as an argument), and then extract the hostname to pass as the principal to CheckCert()? (and consider calling CheckCert() a second time with the port included in the principal if the first call fails?)

(and then also move the IsUserAuthority() to Authenticate()) - that would mean that CheckCert() would be back to not having any host or user specific code in it)

@pmoody-
Copy link

@pmoody- pmoody- commented May 16, 2017

  1. it depends on what you pass into certchecker.CheckCert. IsHostAuthority and ssh/knownhosts are brand new so dealing with host certs has historically meant a lot of hand holding & much less golang ssh library automation.

  2. again, prior to the knownhosts package, dealing with host certs was very manual, but I can promise it's been used (I've been using it for nearly 2 years)

  3. i'm not sure I understand issue about the argument name principal. The field is called ValidPrincipals ..

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).

@aeijdenberg
Copy link
Author

@aeijdenberg aeijdenberg commented May 16, 2017

Hi Peter, the refactoring I mean is pretty minor - it moves the host specific cert checking to the CheckHostKey() method. What I meant re names, is that the argument to CheckCert() is named principal - we shouldn't be passing it a host:port (addr) to begin with.

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

@pmoody- pmoody- commented May 16, 2017

¯\_(ツ)_/¯

i'm not sure I see the benefit over 43551 but if you feel strongly you can sign the cla and send a diff.

@aeijdenberg
Copy link
Author

@aeijdenberg aeijdenberg commented May 16, 2017

Hi Peter, thanks and here's my first foray into Gerrit:
https://go-review.googlesource.com/c/43475/

@aeijdenberg
Copy link
Author

@aeijdenberg aeijdenberg commented May 16, 2017

For reference, PROTOCOL.certkeys states the following:

"valid principals" is a string containing zero or more principals as
strings packed inside it. These principals list the names for which this
certificate is valid; hostnames for SSH_CERT_TYPE_HOST certificates and
usernames for SSH_CERT_TYPE_USER certificates. As a special case, a
zero-length "valid principals" field means the certificate is valid for
any principal of the specified type.

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 IsHostAuthority() should do or should not do in the context of processing a known_hosts file - that's not addressed by that document.

@aeijdenberg aeijdenberg changed the title x/crypto/ssh: CertChecker incorrectly(?) includes port number in hostname as part of checked principal x/crypto/ssh: CertChecker incorrectly includes port number in hostname as part of checked principal May 16, 2017
@pmoody-
Copy link

@pmoody- pmoody- commented May 16, 2017

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.

@aeijdenberg
Copy link
Author

@aeijdenberg aeijdenberg commented May 16, 2017

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 known_host matching is different from principal evaluation (since that also caused confusion on the openssh list).

Now we just need the patch to land, and I can see the review is underway, so I think we are all happy.

@pmoody-
Copy link

@pmoody- pmoody- commented May 16, 2017

glad you were able to refactor my patch.

@aeijdenberg
Copy link
Author

@aeijdenberg aeijdenberg commented May 23, 2017

Looks like the patch landed in golang/crypto@7e91053.

Closing this issue. Thanks for your help Peter.

@golang golang locked and limited conversation to collaborators May 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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