From 97217e8a68b58ccab8d8bbd787e7e4ee3f20570e Mon Sep 17 00:00:00 2001 From: Emman Date: Mon, 11 Apr 2022 17:33:40 +0800 Subject: [PATCH] crypto/x509: only disable SHA-1 verification for certificates --- internal/godebug/godebug.go | 34 ++++++++++++ smx509/x509.go | 21 +++++-- smx509/x509_test.go | 106 ++++++++++++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 internal/godebug/godebug.go diff --git a/internal/godebug/godebug.go b/internal/godebug/godebug.go new file mode 100644 index 0000000..ac434e5 --- /dev/null +++ b/internal/godebug/godebug.go @@ -0,0 +1,34 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package godebug parses the GODEBUG environment variable. +package godebug + +import "os" + +// Get returns the value for the provided GODEBUG key. +func Get(key string) string { + return get(os.Getenv("GODEBUG"), key) +} + +// get returns the value part of key=value in s (a GODEBUG value). +func get(s, key string) string { + for i := 0; i < len(s)-len(key)-1; i++ { + if i > 0 && s[i-1] != ',' { + continue + } + afterKey := s[i+len(key):] + if afterKey[0] != '=' || s[i:i+len(key)] != key { + continue + } + val := afterKey[1:] + for i, b := range val { + if b == ',' { + return val[:i] + } + } + return val + } + return "" +} diff --git a/smx509/x509.go b/smx509/x509.go index 2af3b9b..947d9dd 100644 --- a/smx509/x509.go +++ b/smx509/x509.go @@ -24,6 +24,7 @@ import ( "golang.org/x/crypto/cryptobyte" cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1" + "github.com/emmansun/gmsm/internal/godebug" "github.com/emmansun/gmsm/sm2" ) @@ -544,6 +545,9 @@ func oidFromExtKeyUsage(eku ExtKeyUsage) (oid asn1.ObjectIdentifier, ok bool) { return } +// debugAllowSHA1 allows SHA-1 signatures. See issue 41682. +var debugAllowSHA1 = godebug.Get("x509sha1") == "1" + // A Certificate represents an X.509 certificate. type Certificate x509.Certificate @@ -585,13 +589,13 @@ func (c *Certificate) CheckSignatureFrom(parent *Certificate) error { // TODO(agl): don't ignore the path length constraint. - return parent.CheckSignature(c.SignatureAlgorithm, c.RawTBSCertificate, c.Signature) + return checkSignature(c.SignatureAlgorithm, c.RawTBSCertificate, c.Signature, parent.PublicKey, debugAllowSHA1) } // CheckSignature verifies that signature is a valid signature over signed from // c's public key. func (c *Certificate) CheckSignature(algo SignatureAlgorithm, signed, signature []byte) error { - return checkSignature(algo, signed, signature, c.PublicKey) + return checkSignature(algo, signed, signature, c.PublicKey, true) } func (c *Certificate) hasNameConstraints() bool { @@ -613,7 +617,7 @@ func signaturePublicKeyAlgoMismatchError(expectedPubKeyAlgo PublicKeyAlgorithm, // checkSignature verifies that signature is a valid signature over signed from // a crypto.PublicKey. -func checkSignature(algo SignatureAlgorithm, signed, signature []byte, publicKey crypto.PublicKey) (err error) { +func checkSignature(algo SignatureAlgorithm, signed, signature []byte, publicKey crypto.PublicKey, allowSHA1 bool) (err error) { var hashType crypto.Hash var pubKeyAlgo PublicKeyAlgorithm @@ -632,6 +636,11 @@ func checkSignature(algo SignatureAlgorithm, signed, signature []byte, publicKey } case crypto.MD5: return x509.InsecureAlgorithmError(algo) + case crypto.SHA1: + if !allowSHA1 { + return x509.InsecureAlgorithmError(algo) + } + fallthrough default: if !hashType.Available() { return x509.ErrUnsupportedAlgorithm @@ -1380,11 +1389,11 @@ func CreateCertificate(rand io.Reader, template, parent *x509.Certificate, pub, // Check the signature to ensure the crypto.Signer behaved correctly. sigAlg := getSignatureAlgorithmFromAI(signatureAlgorithm) switch sigAlg { - case MD5WithRSA, SHA1WithRSA, ECDSAWithSHA1: + case MD5WithRSA: // We skip the check if the signature algorithm is only supported for // signing, not verification. default: - if err := checkSignature(sigAlg, c.Raw, signature, key.Public()); err != nil { + if err := checkSignature(sigAlg, c.Raw, signature, key.Public(), true); err != nil { return nil, fmt.Errorf("x509: signature over certificate returned by signer is invalid: %w", err) } } @@ -1830,7 +1839,7 @@ func parseCertificateRequest(in *certificateRequest) (*CertificateRequest, error // CheckSignature reports whether the signature on c is valid. func (c *CertificateRequest) CheckSignature() error { - return checkSignature(c.SignatureAlgorithm, c.RawTBSCertificateRequest, c.Signature, c.PublicKey) + return checkSignature(c.SignatureAlgorithm, c.RawTBSCertificateRequest, c.Signature, c.PublicKey, true) } // CreateRevocationList creates a new X.509 v2 Certificate Revocation List, diff --git a/smx509/x509_test.go b/smx509/x509_test.go index 4da10b4..31753a8 100644 --- a/smx509/x509_test.go +++ b/smx509/x509_test.go @@ -187,6 +187,36 @@ func init() { } } +func bigFromString(s string) *big.Int { + ret := new(big.Int) + ret.SetString(s, 10) + return ret +} + +func fromBase10(base10 string) *big.Int { + i := new(big.Int) + i.SetString(base10, 10) + return i +} + +func bigFromHexString(s string) *big.Int { + ret := new(big.Int) + ret.SetString(s, 16) + return ret +} + +var rsaPrivateKey = &rsa.PrivateKey{ + PublicKey: rsa.PublicKey{ + N: bigFromString("124737666279038955318614287965056875799409043964547386061640914307192830334599556034328900586693254156136128122194531292927142396093148164407300419162827624945636708870992355233833321488652786796134504707628792159725681555822420087112284637501705261187690946267527866880072856272532711620639179596808018872997"), + E: 65537, + }, + D: bigFromString("69322600686866301945688231018559005300304807960033948687567105312977055197015197977971637657636780793670599180105424702854759606794705928621125408040473426339714144598640466128488132656829419518221592374964225347786430566310906679585739468938549035854760501049443920822523780156843263434219450229353270690889"), + Primes: []*big.Int{ + bigFromString("11405025354575369741595561190164746858706645478381139288033759331174478411254205003127028642766986913445391069745480057674348716675323735886284176682955723"), + bigFromString("10937079261204603443118731009201819560867324167189758120988909645641782263430128449826989846631183550578761324239709121189827307416350485191350050332642639"), + }, +} + func getPublicKey(pemContent []byte) (interface{}, error) { block, _ := pem.Decode(pemContent) if block == nil { @@ -2323,3 +2353,79 @@ func TestParseUniqueID(t *testing.T) { t.Fatalf("unexpected number of extensions (probably because the extension section was not parsed): got %d, want 7", len(cert.Extensions)) } } + +func TestCreateCertificateLegacy(t *testing.T) { + sigAlg := MD5WithRSA + template := &Certificate{ + SerialNumber: big.NewInt(10), + DNSNames: []string{"example.com"}, + SignatureAlgorithm: sigAlg, + } + _, err := CreateCertificate(rand.Reader, template.asX509(), template.asX509(), testPrivateKey.Public(), &brokenSigner{testPrivateKey.Public()}) + if err != nil { + t.Fatalf("CreateCertificate failed when SignatureAlgorithm = %v: %s", sigAlg, err) + } +} + +func TestDisableSHA1ForCertOnly(t *testing.T) { + defer func(old bool) { debugAllowSHA1 = old }(debugAllowSHA1) + debugAllowSHA1 = false + + tmpl := &Certificate{ + SerialNumber: big.NewInt(1), + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + SignatureAlgorithm: SHA1WithRSA, + BasicConstraintsValid: true, + IsCA: true, + KeyUsage: KeyUsageCertSign | KeyUsageCRLSign, + } + certDER, err := CreateCertificate(rand.Reader, tmpl.asX509(), tmpl.asX509(), rsaPrivateKey.Public(), rsaPrivateKey) + if err != nil { + t.Fatalf("failed to generate test cert: %s", err) + } + cert, err := ParseCertificate(certDER) + if err != nil { + t.Fatalf("failed to parse test cert: %s", err) + } + + err = cert.CheckSignatureFrom(cert) + if err == nil { + t.Error("expected CheckSignatureFrom to fail") + } else if _, ok := err.(x509.InsecureAlgorithmError); !ok { + t.Errorf("expected InsecureAlgorithmError error, got %T", err) + } + + crlDER, err := CreateRevocationList(rand.Reader, &x509.RevocationList{ + SignatureAlgorithm: SHA1WithRSA, + Number: big.NewInt(1), + ThisUpdate: time.Now().Add(-time.Hour), + NextUpdate: time.Now().Add(time.Hour), + }, cert, rsaPrivateKey) + if err != nil { + t.Fatalf("failed to generate test CRL: %s", err) + } + // TODO(rolandshoemaker): this should be ParseRevocationList once it lands + crl, err := ParseCRL(crlDER) + if err != nil { + t.Fatalf("failed to parse test CRL: %s", err) + } + + if err = cert.CheckCRLSignature(crl); err != nil { + t.Errorf("unexpected error: %s", err) + } + + // This is an unrelated OCSP response, which will fail signature verification + // but shouldn't return a InsecureAlgorithmError, since SHA1 should be allowed + // for OCSP. + ocspTBSHex := "30819fa2160414884451ff502a695e2d88f421bad90cf2cecbea7c180f32303133303631383037323434335a30743072304a300906052b0e03021a0500041448b60d38238df8456e4ee5843ea394111802979f0414884451ff502a695e2d88f421bad90cf2cecbea7c021100f78b13b946fc9635d8ab49de9d2148218000180f32303133303631383037323434335aa011180f32303133303632323037323434335a" + ocspTBS, err := hex.DecodeString(ocspTBSHex) + if err != nil { + t.Fatalf("failed to decode OCSP response TBS hex: %s", err) + } + + err = cert.CheckSignature(SHA1WithRSA, ocspTBS, nil) + if err != rsa.ErrVerification { + t.Errorf("unexpected error: %s", err) + } +}