From 15d8abf5c4f384ff6370c85655f17abac6f84716 Mon Sep 17 00:00:00 2001 From: JP Sugarbroad Date: Wed, 9 Oct 2013 12:56:09 -0400 Subject: [PATCH] go.crypto/ssh: fix certificate parsing/marshaling. The change to add the PublicKey interface accidentally caused certificate handling to expect an extra copy of the private key algorithm name in the binary representation. This change adapts a suitable parsing API and adds a test to ensure that cert handling isn't easily broken in the future. R=agl, hanwen, jmpittman CC=golang-dev https://golang.org/cl/13272055 --- ssh/agent.go | 2 +- ssh/certs.go | 22 +++++++++++++++++----- ssh/common.go | 2 +- ssh/keys.go | 29 +++++++++++++++-------------- ssh/keys_test.go | 29 +++++++++++++++++++++++++++++ ssh/server.go | 2 +- 6 files changed, 64 insertions(+), 22 deletions(-) diff --git a/ssh/agent.go b/ssh/agent.go index 924a58a..a91da34 100644 --- a/ssh/agent.go +++ b/ssh/agent.go @@ -100,7 +100,7 @@ func (ak *AgentKey) String() string { // Key returns an agent's public key as one of the supported key or certificate types. func (ak *AgentKey) Key() (PublicKey, error) { - if key, _, ok := parsePubKey(ak.blob); ok { + if key, _, ok := ParsePublicKey(ak.blob); ok { return key, nil } return nil, errors.New("ssh: failed to parse key blob") diff --git a/ssh/certs.go b/ssh/certs.go index 53bc8fd..0772289 100644 --- a/ssh/certs.go +++ b/ssh/certs.go @@ -60,6 +60,17 @@ var certAlgoNames = map[string]string{ KeyAlgoECDSA521: CertAlgoECDSA521v01, } +// certToPrivAlgo returns the underlying algorithm for a certificate algorithm. +// Panics if a non-certificate algorithm is passed. +func certToPrivAlgo(algo string) string { + for privAlgo, pubAlgo := range certAlgoNames { + if pubAlgo == algo { + return privAlgo + } + } + panic("unknown cert algorithm") +} + func (c *OpenSSHCertV01) PublicKeyAlgo() string { algo, ok := certAlgoNames[c.Key.PublicKeyAlgo()] if !ok { @@ -83,12 +94,14 @@ func parseOpenSSHCertV01(in []byte, algo string) (out *OpenSSHCertV01, rest []by return } - cert.Key, in, ok = ParsePublicKey(in) + privAlgo := certToPrivAlgo(algo) + cert.Key, in, ok = parsePubKey(in, privAlgo) if !ok { return } - if cert.Key.PrivateKeyAlgo() != algo { + // We test PublicKeyAlgo to make sure we don't use some weird sub-cert. + if cert.Key.PublicKeyAlgo() != privAlgo { ok = false return } @@ -139,7 +152,7 @@ func parseOpenSSHCertV01(in []byte, algo string) (out *OpenSSHCertV01, rest []by if !ok { return } - if cert.SignatureKey, _, ok = parsePubKey(sigKey); !ok { + if cert.SignatureKey, _, ok = ParsePublicKey(sigKey); !ok { return } @@ -152,8 +165,7 @@ func parseOpenSSHCertV01(in []byte, algo string) (out *OpenSSHCertV01, rest []by } func (cert *OpenSSHCertV01) Marshal() []byte { - pubKey := MarshalPublicKey(cert.Key) - + pubKey := cert.Key.Marshal() sigKey := MarshalPublicKey(cert.SignatureKey) length := stringLength(len(cert.Nonce)) diff --git a/ssh/common.go b/ssh/common.go index 2c68278..44f15a3 100644 --- a/ssh/common.go +++ b/ssh/common.go @@ -201,7 +201,7 @@ func serializeSignature(name string, sig []byte) []byte { // generating an authorized_keys or host_keys file. func MarshalPublicKey(key PublicKey) []byte { // See also RFC 4253 6.6. - algoname := key.PrivateKeyAlgo() + algoname := key.PublicKeyAlgo() blob := key.Marshal() length := stringLength(len(algoname)) diff --git a/ssh/keys.go b/ssh/keys.go index 0c6c6b3..fa1e236 100644 --- a/ssh/keys.go +++ b/ssh/keys.go @@ -31,14 +31,10 @@ const ( KeyAlgoECDSA521 = "ecdsa-sha2-nistp521" ) -// parsePubKey parses a public key according to RFC 4253, section 6.6. -func parsePubKey(in []byte) (pubKey PublicKey, rest []byte, ok bool) { - algo, in, ok := parseString(in) - if !ok { - return - } - - switch string(algo) { +// parsePubKey parses a public key of the given algorithm. +// Use ParsePublicKey for keys with prepended algorithm. +func parsePubKey(in []byte, algo string) (pubKey PublicKey, rest []byte, ok bool) { + switch algo { case KeyAlgoRSA: return parseRSA(in) case KeyAlgoDSA: @@ -46,7 +42,7 @@ func parsePubKey(in []byte) (pubKey PublicKey, rest []byte, ok bool) { case KeyAlgoECDSA256, KeyAlgoECDSA384, KeyAlgoECDSA521: return parseECDSA(in) case CertAlgoRSAv01, CertAlgoDSAv01, CertAlgoECDSA256v01, CertAlgoECDSA384v01, CertAlgoECDSA521v01: - return parseOpenSSHCertV01(in, string(algo)) + return parseOpenSSHCertV01(in, algo) } return nil, nil, false } @@ -54,7 +50,7 @@ func parsePubKey(in []byte) (pubKey PublicKey, rest []byte, ok bool) { // parseAuthorizedKey parses a public key in OpenSSH authorized_keys format // (see sshd(8) manual page) once the options and key type fields have been // removed. -func parseAuthorizedKey(in []byte) (out interface{}, comment string, ok bool) { +func parseAuthorizedKey(in []byte) (out PublicKey, comment string, ok bool) { in = bytes.TrimSpace(in) i := bytes.IndexAny(in, " \t") @@ -69,7 +65,7 @@ func parseAuthorizedKey(in []byte) (out interface{}, comment string, ok bool) { return } key = key[:n] - out, _, ok = parsePubKey(key) + out, _, ok = ParsePublicKey(key) if !ok { return nil, "", false } @@ -79,7 +75,7 @@ func parseAuthorizedKey(in []byte) (out interface{}, comment string, ok bool) { // ParseAuthorizedKeys parses a public key from an authorized_keys // file used in OpenSSH according to the sshd(8) manual page. -func ParseAuthorizedKey(in []byte) (out interface{}, comment string, options []string, rest []byte, ok bool) { +func ParseAuthorizedKey(in []byte) (out PublicKey, comment string, options []string, rest []byte, ok bool) { for len(in) > 0 { end := bytes.IndexByte(in, '\n') if end != -1 { @@ -179,9 +175,14 @@ func ParseAuthorizedKey(in []byte) (out interface{}, comment string, options []s } // ParsePublicKey parses an SSH public key formatted for use in -// the SSH wire protocol. +// the SSH wire protocol according to RFC 4253, section 6.6. func ParsePublicKey(in []byte) (out PublicKey, rest []byte, ok bool) { - return parsePubKey(in) + algo, in, ok := parseString(in) + if !ok { + return + } + + return parsePubKey(in, string(algo)) } // MarshalAuthorizedKey returns a byte stream suitable for inclusion diff --git a/ssh/keys_test.go b/ssh/keys_test.go index 595fb29..f05843f 100644 --- a/ssh/keys_test.go +++ b/ssh/keys_test.go @@ -1,11 +1,13 @@ package ssh import ( + "bytes" "crypto/dsa" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" "crypto/rsa" + "encoding/base64" "reflect" "strings" "testing" @@ -163,6 +165,33 @@ func TestParseDSA(t *testing.T) { } } +func TestParseCert(t *testing.T) { + // Cert generated by ssh-keygen 6.0p1 Debian-4. + // % ssh-keygen -s ca-key -I test user-key + b64data := "AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=" + + data, err := base64.StdEncoding.DecodeString(b64data) + if err != nil { + t.Fatal("base64.StdEncoding.DecodeString: ", err) + } + key, rest, ok := ParsePublicKey(data) + if !ok { + t.Fatalf("could not parse certificate") + } + if len(rest) > 0 { + t.Errorf("rest: got %q, want empty", rest) + } + _, ok = key.(*OpenSSHCertV01) + if !ok { + t.Fatalf("got %#v, want *OpenSSHCertV01", key) + } + + marshaled := MarshalPublicKey(key) + if !bytes.Equal(data, marshaled) { + t.Errorf("marshaled certificate does not match original: got %q, want %q", marshaled, data) + } +} + func init() { raw, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) ecdsaKey, _ = NewSignerFromKey(raw) diff --git a/ssh/server.go b/ssh/server.go index b5768b6..86f0970 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -406,7 +406,7 @@ userAuthLoop: break } signedData := buildDataSignedForAuth(H, userAuthReq, algoBytes, pubKey) - key, _, ok := parsePubKey(pubKey) + key, _, ok := ParsePublicKey(pubKey) if !ok { return ParseError{msgUserAuthRequest} }