crypto/x509: improve error when PKCS1, PKCS8, EC keys are mixed up

Improve error messages if ParsePKCS8PrivateKey/ParseECPrivateKey
/ParsePKCS1PrivateKey or ParsePKIXPublicKey/ParsePKCS1PublicKey
are called erroneously instead of one another.

Fixes #30094

Change-Id: Ia419c5f320167791aa82e174b4e9ce0f3275ec63
Reviewed-on: https://go-review.googlesource.com/c/161557
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/crypto/x509/pkcs1.go b/src/crypto/x509/pkcs1.go
index 82502cfe58129e6c237b87c0536f8d11ac605aca..5857c17a45500ec37810598299a13a3c776c9563 100644 (file)
--- a/src/crypto/x509/pkcs1.go
+++ b/src/crypto/x509/pkcs1.go
@@ -49,6 +49,12 @@ func ParsePKCS1PrivateKey(der []byte) (*rsa.PrivateKey, error) {
                return nil, asn1.SyntaxError{Msg: "trailing data"}
        }
        if err != nil {
+               if _, err := asn1.Unmarshal(der, &ecPrivateKey{}); err == nil {
+                       return nil, errors.New("x509: failed to parse private key (use ParseECPrivateKey instead for this key format)")
+               }
+               if _, err := asn1.Unmarshal(der, &pkcs8{}); err == nil {
+                       return nil, errors.New("x509: failed to parse private key (use ParsePKCS8PrivateKey instead for this key format)")
+               }
                return nil, err
        }
 
@@ -125,6 +131,9 @@ func ParsePKCS1PublicKey(der []byte) (*rsa.PublicKey, error) {
        var pub pkcs1PublicKey
        rest, err := asn1.Unmarshal(der, &pub)
        if err != nil {
+               if _, err := asn1.Unmarshal(der, &publicKeyInfo{}); err == nil {
+                       return nil, errors.New("x509: failed to parse public key (use ParsePKIXPublicKey instead for this key format)")
+               }
                return nil, err
        }
        if len(rest) > 0 {

diff --git a/src/crypto/x509/pkcs8.go b/src/crypto/x509/pkcs8.go
index fb1340c6df7b23739785014e3cbb8116ffb70bf4..bf3bd9e565bf778aedd3c0145bfbada0a6874594 100644 (file)
--- a/src/crypto/x509/pkcs8.go
+++ b/src/crypto/x509/pkcs8.go
@@ -28,6 +28,12 @@ type pkcs8 struct {
 func ParsePKCS8PrivateKey(der []byte) (key interface{}, err error) {
        var privKey pkcs8
        if _, err := asn1.Unmarshal(der, &privKey); err != nil {
+               if _, err := asn1.Unmarshal(der, &ecPrivateKey{}); err == nil {
+                       return nil, errors.New("x509: failed to parse private key (use ParseECPrivateKey instead for this key format)")
+               }
+               if _, err := asn1.Unmarshal(der, &pkcs1PrivateKey{}); err == nil {
+                       return nil, errors.New("x509: failed to parse private key (use ParsePKCS1PrivateKey instead for this key format)")
+               }
                return nil, err
        }
        switch {

diff --git a/src/crypto/x509/pkcs8_test.go b/src/crypto/x509/pkcs8_test.go
index c8f11e64d12ebf809eceee259850fdb9d5ec923b..4a72cc0c5e606a466e356bbc9adda9614da6175c 100644 (file)
--- a/src/crypto/x509/pkcs8_test.go
+++ b/src/crypto/x509/pkcs8_test.go
@@ -11,6 +11,7 @@ import (
        "crypto/rsa"
        "encoding/hex"
        "reflect"
+       "strings"
        "testing"
 )
 
@@ -107,3 +108,24 @@ func TestPKCS8(t *testing.T) {
                }
        }
 }
+
+const hexPKCS8TestPKCS1Key = "3082025c02010002818100b1a1e0945b9289c4d3f1329f8a982c4a2dcd59bfd372fb8085a9c517554607ebd2f7990eef216ac9f4605f71a03b04f42a5255b158cf8e0844191f5119348baa44c35056e20609bcf9510f30ead4b481c81d7865fb27b8e0090e112b717f3ee08cdfc4012da1f1f7cf2a1bc34c73a54a12b06372d09714742dd7895eadde4aa5020301000102818062b7fa1db93e993e40237de4d89b7591cc1ea1d04fed4904c643f17ae4334557b4295270d0491c161cb02a9af557978b32b20b59c267a721c4e6c956c2d147046e9ae5f2da36db0106d70021fa9343455f8f973a4b355a26fd19e6b39dee0405ea2b32deddf0f4817759ef705d02b34faab9ca93c6766e9f722290f119f34449024100d9c29a4a013a90e35fd1be14a3f747c589fac613a695282d61812a711906b8a0876c6181f0333ca1066596f57bff47e7cfcabf19c0fc69d9cd76df743038b3cb024100d0d3546fecf879b5551f2bd2c05e6385f2718a08a6face3d2aecc9d7e03645a480a46c81662c12ad6bd6901e3bd4f38029462de7290859567cdf371c79088d4f024100c254150657e460ea58573fcf01a82a4791e3d6223135c8bdfed69afe84fbe7857274f8eb5165180507455f9b4105c6b08b51fe8a481bb986a202245576b713530240045700003b7a867d0041df9547ae2e7f50248febd21c9040b12dae9c2feab0d3d4609668b208e4727a3541557f84d372ac68eaf74ce1018a4c9a0ef92682c8fd02405769731480bb3a4570abf422527c5f34bf732fa6c1e08cc322753c511ce055fac20fc770025663ad3165324314df907f1f1942f0448a7e9cdbf87ecd98b92156"
+const hexPKCS8TestECKey = "3081a40201010430bdb9839c08ee793d1157886a7a758a3c8b2a17a4df48f17ace57c72c56b4723cf21dcda21d4e1ad57ff034f19fcfd98ea00706052b81040022a16403620004feea808b5ee2429cfcce13c32160e1c960990bd050bb0fdf7222f3decd0a55008e32a6aa3c9062051c4cba92a7a3b178b24567412d43cdd2f882fa5addddd726fe3e208d2c26d733a773a597abb749714df7256ead5105fa6e7b3650de236b50"
+
+var pkcs8MismatchKeyTests = []struct {
+       hexKey        string
+       errorContains string
+}{
+       {hexKey: hexPKCS8TestECKey, errorContains: "use ParseECPrivateKey instead"},
+       {hexKey: hexPKCS8TestPKCS1Key, errorContains: "use ParsePKCS1PrivateKey instead"},
+}
+
+func TestPKCS8MismatchKeyFormat(t *testing.T) {
+       for i, test := range pkcs8MismatchKeyTests {
+               derBytes, _ := hex.DecodeString(test.hexKey)
+               _, err := ParsePKCS8PrivateKey(derBytes)
+               if !strings.Contains(err.Error(), test.errorContains) {
+                       t.Errorf("#%d: expected error containing %q, got %s", i, test.errorContains, err)
+               }
+       }
+}

diff --git a/src/crypto/x509/sec1.go b/src/crypto/x509/sec1.go
index 3008d0df773fcc34814d7913846acac798c6632b..faba9dbe5dd11d0c08d4cf6a66440ced1cff6e2f 100644 (file)
--- a/src/crypto/x509/sec1.go
+++ b/src/crypto/x509/sec1.go
@@ -65,6 +65,12 @@ func marshalECPrivateKeyWithOID(key *ecdsa.PrivateKey, oid asn1.ObjectIdentifier
 func parseECPrivateKey(namedCurveOID *asn1.ObjectIdentifier, der []byte) (key *ecdsa.PrivateKey, err error) {
        var privKey ecPrivateKey
        if _, err := asn1.Unmarshal(der, &privKey); err != nil {
+               if _, err := asn1.Unmarshal(der, &pkcs8{}); err == nil {
+                       return nil, errors.New("x509: failed to parse private key (use ParsePKCS8PrivateKey instead for this key format)")
+               }
+               if _, err := asn1.Unmarshal(der, &pkcs1PrivateKey{}); err == nil {
+                       return nil, errors.New("x509: failed to parse private key (use ParsePKCS1PrivateKey instead for this key format)")
+               }
                return nil, errors.New("x509: failed to parse EC private key: " + err.Error())
        }
        if privKey.Version != ecPrivKeyVersion {

diff --git a/src/crypto/x509/sec1_test.go b/src/crypto/x509/sec1_test.go
index 573c937cafdf8b8fe5e202a86ef8da6b2b20189a..9ac251896bd995ecebcd70c2831b476750f9619c 100644 (file)
--- a/src/crypto/x509/sec1_test.go
+++ b/src/crypto/x509/sec1_test.go
@@ -7,6 +7,7 @@ package x509
 import (
        "bytes"
        "encoding/hex"
+       "strings"
        "testing"
 )
 
@@ -42,3 +43,24 @@ func TestParseECPrivateKey(t *testing.T) {
                }
        }
 }
+
+const hexECTestPKCS1Key = "3082025c02010002818100b1a1e0945b9289c4d3f1329f8a982c4a2dcd59bfd372fb8085a9c517554607ebd2f7990eef216ac9f4605f71a03b04f42a5255b158cf8e0844191f5119348baa44c35056e20609bcf9510f30ead4b481c81d7865fb27b8e0090e112b717f3ee08cdfc4012da1f1f7cf2a1bc34c73a54a12b06372d09714742dd7895eadde4aa5020301000102818062b7fa1db93e993e40237de4d89b7591cc1ea1d04fed4904c643f17ae4334557b4295270d0491c161cb02a9af557978b32b20b59c267a721c4e6c956c2d147046e9ae5f2da36db0106d70021fa9343455f8f973a4b355a26fd19e6b39dee0405ea2b32deddf0f4817759ef705d02b34faab9ca93c6766e9f722290f119f34449024100d9c29a4a013a90e35fd1be14a3f747c589fac613a695282d61812a711906b8a0876c6181f0333ca1066596f57bff47e7cfcabf19c0fc69d9cd76df743038b3cb024100d0d3546fecf879b5551f2bd2c05e6385f2718a08a6face3d2aecc9d7e03645a480a46c81662c12ad6bd6901e3bd4f38029462de7290859567cdf371c79088d4f024100c254150657e460ea58573fcf01a82a4791e3d6223135c8bdfed69afe84fbe7857274f8eb5165180507455f9b4105c6b08b51fe8a481bb986a202245576b713530240045700003b7a867d0041df9547ae2e7f50248febd21c9040b12dae9c2feab0d3d4609668b208e4727a3541557f84d372ac68eaf74ce1018a4c9a0ef92682c8fd02405769731480bb3a4570abf422527c5f34bf732fa6c1e08cc322753c511ce055fac20fc770025663ad3165324314df907f1f1942f0448a7e9cdbf87ecd98b92156"
+const hexECTestPKCS8Key = "30820278020100300d06092a864886f70d0101010500048202623082025e02010002818100cfb1b5bf9685ffa97b4f99df4ff122b70e59ac9b992f3bc2b3dde17d53c1a34928719b02e8fd17839499bfbd515bd6ef99c7a1c47a239718fe36bfd824c0d96060084b5f67f0273443007a24dfaf5634f7772c9346e10eb294c2306671a5a5e719ae24b4de467291bc571014b0e02dec04534d66a9bb171d644b66b091780e8d020301000102818100b595778383c4afdbab95d2bfed12b3f93bb0a73a7ad952f44d7185fd9ec6c34de8f03a48770f2009c8580bcd275e9632714e9a5e3f32f29dc55474b2329ff0ebc08b3ffcb35bc96e6516b483df80a4a59cceb71918cbabf91564e64a39d7e35dce21cb3031824fdbc845dba6458852ec16af5dddf51a8397a8797ae0337b1439024100ea0eb1b914158c70db39031dd8904d6f18f408c85fbbc592d7d20dee7986969efbda081fdf8bc40e1b1336d6b638110c836bfdc3f314560d2e49cd4fbde1e20b024100e32a4e793b574c9c4a94c8803db5152141e72d03de64e54ef2c8ed104988ca780cd11397bc359630d01b97ebd87067c5451ba777cf045ca23f5912f1031308c702406dfcdbbd5a57c9f85abc4edf9e9e29153507b07ce0a7ef6f52e60dcfebe1b8341babd8b789a837485da6c8d55b29bbb142ace3c24a1f5b54b454d01b51e2ad03024100bd6a2b60dee01e1b3bfcef6a2f09ed027c273cdbbaf6ba55a80f6dcc64e4509ee560f84b4f3e076bd03b11e42fe71a3fdd2dffe7e0902c8584f8cad877cdc945024100aa512fa4ada69881f1d8bb8ad6614f192b83200aef5edf4811313d5ef30a86cbd0a90f7b025c71ea06ec6b34db6306c86b1040670fd8654ad7291d066d06d031"
+
+var ecMismatchKeyTests = []struct {
+       hexKey        string
+       errorContains string
+}{
+       {hexKey: hexECTestPKCS8Key, errorContains: "use ParsePKCS8PrivateKey instead"},
+       {hexKey: hexECTestPKCS1Key, errorContains: "use ParsePKCS1PrivateKey instead"},
+}
+
+func TestECMismatchKeyFormat(t *testing.T) {
+       for i, test := range ecMismatchKeyTests {
+               derBytes, _ := hex.DecodeString(test.hexKey)
+               _, err := ParseECPrivateKey(derBytes)
+               if !strings.Contains(err.Error(), test.errorContains) {
+                       t.Errorf("#%d: expected error containing %q, got %s", i, test.errorContains, err)
+               }
+       }
+}

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 58098adc2d5a2c53b0ea5757cd86047481185e7f..4f9b305e7c3357329d38fb1db7eba14d3ea7da03 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -54,6 +54,9 @@ type pkixPublicKey struct {
 func ParsePKIXPublicKey(derBytes []byte) (pub interface{}, err error) {
        var pki publicKeyInfo
        if rest, err := asn1.Unmarshal(derBytes, &pki); err != nil {
+               if _, err := asn1.Unmarshal(derBytes, &pkcs1PublicKey{}); err == nil {
+                       return nil, errors.New("x509: failed to parse public key (use ParsePKCS1PublicKey instead for this key format)")
+               }
                return nil, err
        } else if len(rest) != 0 {
                return nil, errors.New("x509: trailing data after ASN.1 of public-key")

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 388156e209726c37839fadb633a74a315183c815..f5851f1f11b79fe03dbb8e5e038e2a3fd6f05d83 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -54,6 +54,17 @@ func TestParsePKCS1PrivateKey(t *testing.T) {
        }
 }
 
+func TestPKCS1MismatchPublicKeyFormat(t *testing.T) {
+
+       const pkixPublicKey = "30820122300d06092a864886f70d01010105000382010f003082010a0282010100dd5a0f37d3ca5232852ccc0e81eebec270e2f2c6c44c6231d852971a0aad00aa7399e9b9de444611083c59ea919a9d76c20a7be131a99045ec19a7bb452d647a72429e66b87e28be9e8187ed1d2a2a01ef3eb2360706bd873b07f2d1f1a72337aab5ec94e983e39107f52c480d404915e84d75a3db2cfd601726a128cb1d7f11492d4bdb53272e652276667220795c709b8a9b4af6489cbf48bb8173b8fb607c834a71b6e8bf2d6aab82af3c8ad7ce16d8dcf58373a6edc427f7484d09744d4c08f4e19ed07adbf6cb31243bc5d0d1145e77a08a6fc5efd208eca67d6abf2d6f38f58b6fdd7c28774fb0cc03fc4935c6e074842d2e1479d3d8787249258719f90203010001"
+       const errorContains = "use ParsePKIXPublicKey instead"
+       derBytes, _ := hex.DecodeString(pkixPublicKey)
+       _, err := ParsePKCS1PublicKey(derBytes)
+       if !strings.Contains(err.Error(), errorContains) {
+               t.Errorf("expected error containing %q, got %s", errorContains, err)
+       }
+}
+
 func TestParsePKIXPublicKey(t *testing.T) {
        block, _ := pem.Decode([]byte(pemPublicKey))
        pub, err := ParsePKIXPublicKey(block.Bytes)
@@ -106,6 +117,17 @@ wg/HcAJWY60xZTJDFN+Qfx8ZQvBEin6c2/h+zZi5IVY=
 -----END RSA PRIVATE KEY-----
 `
 
+func TestPKIXMismatchPublicKeyFormat(t *testing.T) {
+
+       const pkcs1PublicKey = "308201080282010100817cfed98bcaa2e2a57087451c7674e0c675686dc33ff1268b0c2a6ee0202dec710858ee1c31bdf5e7783582e8ca800be45f3275c6576adc35d98e26e95bb88ca5beb186f853b8745d88bc9102c5f38753bcda519fb05948d5c77ac429255ff8aaf27d9f45d1586e95e2e9ba8a7cb771b8a09dd8c8fed3f933fd9b439bc9f30c475953418ef25f71a2b6496f53d94d39ce850aa0cc75d445b5f5b4f4ee4db78ab197a9a8d8a852f44529a007ac0ac23d895928d60ba538b16b0b087a7f903ed29770e215019b77eaecc360f35f7ab11b6d735978795b2c4a74e5bdea4dc6594cd67ed752a108e666729a753ab36d6c4f606f8760f507e1765be8cd744007e629020103"
+       const errorContains = "use ParsePKCS1PublicKey instead"
+       derBytes, _ := hex.DecodeString(pkcs1PublicKey)
+       _, err := ParsePKIXPublicKey(derBytes)
+       if !strings.Contains(err.Error(), errorContains) {
+               t.Errorf("expected error containing %q, got %s", errorContains, err)
+       }
+}
+
 var testPrivateKey *rsa.PrivateKey
 
 func init() {
@@ -2017,3 +2039,24 @@ func TestMultipleURLsInCRLDP(t *testing.T) {
                t.Errorf("CRL distribution points = %#v, want #%v", got, want)
        }
 }
+
+const hexPKCS1TestPKCS8Key = "30820278020100300d06092a864886f70d0101010500048202623082025e02010002818100cfb1b5bf9685ffa97b4f99df4ff122b70e59ac9b992f3bc2b3dde17d53c1a34928719b02e8fd17839499bfbd515bd6ef99c7a1c47a239718fe36bfd824c0d96060084b5f67f0273443007a24dfaf5634f7772c9346e10eb294c2306671a5a5e719ae24b4de467291bc571014b0e02dec04534d66a9bb171d644b66b091780e8d020301000102818100b595778383c4afdbab95d2bfed12b3f93bb0a73a7ad952f44d7185fd9ec6c34de8f03a48770f2009c8580bcd275e9632714e9a5e3f32f29dc55474b2329ff0ebc08b3ffcb35bc96e6516b483df80a4a59cceb71918cbabf91564e64a39d7e35dce21cb3031824fdbc845dba6458852ec16af5dddf51a8397a8797ae0337b1439024100ea0eb1b914158c70db39031dd8904d6f18f408c85fbbc592d7d20dee7986969efbda081fdf8bc40e1b1336d6b638110c836bfdc3f314560d2e49cd4fbde1e20b024100e32a4e793b574c9c4a94c8803db5152141e72d03de64e54ef2c8ed104988ca780cd11397bc359630d01b97ebd87067c5451ba777cf045ca23f5912f1031308c702406dfcdbbd5a57c9f85abc4edf9e9e29153507b07ce0a7ef6f52e60dcfebe1b8341babd8b789a837485da6c8d55b29bbb142ace3c24a1f5b54b454d01b51e2ad03024100bd6a2b60dee01e1b3bfcef6a2f09ed027c273cdbbaf6ba55a80f6dcc64e4509ee560f84b4f3e076bd03b11e42fe71a3fdd2dffe7e0902c8584f8cad877cdc945024100aa512fa4ada69881f1d8bb8ad6614f192b83200aef5edf4811313d5ef30a86cbd0a90f7b025c71ea06ec6b34db6306c86b1040670fd8654ad7291d066d06d031"
+const hexPKCS1TestECKey = "3081a40201010430bdb9839c08ee793d1157886a7a758a3c8b2a17a4df48f17ace57c72c56b4723cf21dcda21d4e1ad57ff034f19fcfd98ea00706052b81040022a16403620004feea808b5ee2429cfcce13c32160e1c960990bd050bb0fdf7222f3decd0a55008e32a6aa3c9062051c4cba92a7a3b178b24567412d43cdd2f882fa5addddd726fe3e208d2c26d733a773a597abb749714df7256ead5105fa6e7b3650de236b50"
+
+var pkcs1MismatchKeyTests = []struct {
+       hexKey        string
+       errorContains string
+}{
+       {hexKey: hexPKCS1TestPKCS8Key, errorContains: "use ParsePKCS8PrivateKey instead"},
+       {hexKey: hexPKCS1TestECKey, errorContains: "use ParseECPrivateKey instead"},
+}
+
+func TestPKCS1MismatchKeyFormat(t *testing.T) {
+       for i, test := range pkcs1MismatchKeyTests {
+               derBytes, _ := hex.DecodeString(test.hexKey)
+               _, err := ParsePKCS1PrivateKey(derBytes)
+               if !strings.Contains(err.Error(), test.errorContains) {
+                       t.Errorf("#%d: expected error containing %q, got %s", i, test.errorContains, err)
+               }
+       }
+}