From 694ee61277300a4c24d570e75c1b63fff8dea468 Mon Sep 17 00:00:00 2001 From: Arash Bina Date: Wed, 6 Feb 2019 21:41:55 -0500 Subject: [PATCH] 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 Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot --- src/crypto/x509/pkcs1.go | 9 ++++++++ src/crypto/x509/pkcs8.go | 6 +++++ src/crypto/x509/pkcs8_test.go | 22 ++++++++++++++++++ src/crypto/x509/sec1.go | 6 +++++ src/crypto/x509/sec1_test.go | 22 ++++++++++++++++++ src/crypto/x509/x509.go | 3 +++ src/crypto/x509/x509_test.go | 43 +++++++++++++++++++++++++++++++++++ 7 files changed, 111 insertions(+) diff --git a/src/crypto/x509/pkcs1.go b/src/crypto/x509/pkcs1.go index 82502cfe581..5857c17a455 100644 --- 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 fb1340c6df7..bf3bd9e565b 100644 --- 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 c8f11e64d12..4a72cc0c5e6 100644 --- 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 3008d0df773..faba9dbe5dd 100644 --- 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 573c937cafd..9ac251896bd 100644 --- 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 58098adc2d5..4f9b305e7c3 100644 --- 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 388156e2097..f5851f1f11b 100644 --- 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) + } + } +}