From eb4f295cb31f7fb5d52810411604a2638c9b19a2 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Sun, 27 Mar 2022 15:47:18 +0200 Subject: [PATCH 1/3] internal/wycheproof: add ECDH tests, including point decompression Fixes golang/go#38936 Change-Id: I231d30fcc683abd9efb36b6fd9cc05f599078ade Reviewed-on: https://go-review.googlesource.com/c/crypto/+/396174 Run-TryBot: Filippo Valsorda TryBot-Result: Gopher Robot Reviewed-by: Filippo Valsorda Auto-Submit: Filippo Valsorda Reviewed-by: Roland Shoemaker --- internal/wycheproof/ecdh_test.go | 163 +++++++++++++++++++++++ internal/wycheproof/ecdsa_compat_test.go | 34 ----- internal/wycheproof/ecdsa_go115_test.go | 16 --- internal/wycheproof/ecdsa_test.go | 133 ++++-------------- 4 files changed, 190 insertions(+), 156 deletions(-) create mode 100644 internal/wycheproof/ecdh_test.go delete mode 100644 internal/wycheproof/ecdsa_compat_test.go delete mode 100644 internal/wycheproof/ecdsa_go115_test.go diff --git a/internal/wycheproof/ecdh_test.go b/internal/wycheproof/ecdh_test.go new file mode 100644 index 0000000000..a3918ba62f --- /dev/null +++ b/internal/wycheproof/ecdh_test.go @@ -0,0 +1,163 @@ +// Copyright 2019 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 wycheproof + +import ( + "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/x509" + "encoding/asn1" + "errors" + "fmt" + "testing" + + "golang.org/x/crypto/cryptobyte" + casn1 "golang.org/x/crypto/cryptobyte/asn1" +) + +func TestECDH(t *testing.T) { + type ECDHTestVector struct { + // A brief description of the test case + Comment string `json:"comment,omitempty"` + // A list of flags + Flags []string `json:"flags,omitempty"` + // the private key + Private string `json:"private,omitempty"` + // Encoded public key + Public string `json:"public,omitempty"` + // Test result + Result string `json:"result,omitempty"` + // The shared secret key + Shared string `json:"shared,omitempty"` + // Identifier of the test case + TcID int `json:"tcId,omitempty"` + } + + type ECDHTestGroup struct { + Curve string `json:"curve,omitempty"` + Tests []*ECDHTestVector `json:"tests,omitempty"` + } + + type Root struct { + TestGroups []*ECDHTestGroup `json:"testGroups,omitempty"` + } + + flagsShouldPass := map[string]bool{ + // ParsePKIXPublicKey doesn't support compressed points, but we test + // them against UnmarshalCompressed anyway. + "CompressedPoint": true, + // We don't support decoding custom curves. + "UnnamedCurve": false, + // WrongOrder and UnusedParam are only found with UnnamedCurve. + "WrongOrder": false, + "UnusedParam": false, + } + + // supportedCurves is a map of all elliptic curves supported + // by crypto/elliptic, which can subsequently be parsed and tested. + supportedCurves := map[string]bool{ + "secp224r1": true, + "secp256r1": true, + "secp384r1": true, + "secp521r1": true, + } + + var root Root + readTestVector(t, "ecdh_test.json", &root) + for _, tg := range root.TestGroups { + if !supportedCurves[tg.Curve] { + continue + } + for _, tt := range tg.Tests { + tg, tt := tg, tt + t.Run(fmt.Sprintf("%s/%d", tg.Curve, tt.TcID), func(t *testing.T) { + t.Logf("Type: %v", tt.Result) + t.Logf("Flags: %q", tt.Flags) + t.Log(tt.Comment) + + shouldPass := shouldPass(tt.Result, tt.Flags, flagsShouldPass) + + p := decodeHex(tt.Public) + pp, err := x509.ParsePKIXPublicKey(p) + if err != nil { + pp, err = decodeCompressedPKIX(p) + } + if err != nil { + if shouldPass { + t.Errorf("unexpected parsing error: %s", err) + } + return + } + pub := pp.(*ecdsa.PublicKey) + + priv := decodeHex(tt.Private) + shared := decodeHex(tt.Shared) + + x, _ := pub.Curve.ScalarMult(pub.X, pub.Y, priv) + xBytes := make([]byte, (pub.Curve.Params().BitSize+7)/8) + got := bytes.Equal(shared, x.FillBytes(xBytes)) + + if want := shouldPass; got != want { + t.Errorf("wanted success %v, got %v", want, got) + } + }) + } + } +} + +func decodeCompressedPKIX(der []byte) (interface{}, error) { + s := cryptobyte.String(der) + var s1, s2 cryptobyte.String + var algoOID, namedCurveOID asn1.ObjectIdentifier + var pointDER []byte + if !s.ReadASN1(&s1, casn1.SEQUENCE) || !s.Empty() || + !s1.ReadASN1(&s2, casn1.SEQUENCE) || + !s2.ReadASN1ObjectIdentifier(&algoOID) || + !s2.ReadASN1ObjectIdentifier(&namedCurveOID) || !s2.Empty() || + !s1.ReadASN1BitStringAsBytes(&pointDER) || !s1.Empty() { + return nil, errors.New("failed to parse PKIX structure") + } + + if !algoOID.Equal(oidPublicKeyECDSA) { + return nil, errors.New("wrong algorithm OID") + } + namedCurve := namedCurveFromOID(namedCurveOID) + if namedCurve == nil { + return nil, errors.New("unsupported elliptic curve") + } + x, y := elliptic.UnmarshalCompressed(namedCurve, pointDER) + if x == nil { + return nil, errors.New("failed to unmarshal elliptic curve point") + } + pub := &ecdsa.PublicKey{ + Curve: namedCurve, + X: x, + Y: y, + } + return pub, nil +} + +var ( + oidPublicKeyECDSA = asn1.ObjectIdentifier{1, 2, 840, 10045, 2, 1} + oidNamedCurveP224 = asn1.ObjectIdentifier{1, 3, 132, 0, 33} + oidNamedCurveP256 = asn1.ObjectIdentifier{1, 2, 840, 10045, 3, 1, 7} + oidNamedCurveP384 = asn1.ObjectIdentifier{1, 3, 132, 0, 34} + oidNamedCurveP521 = asn1.ObjectIdentifier{1, 3, 132, 0, 35} +) + +func namedCurveFromOID(oid asn1.ObjectIdentifier) elliptic.Curve { + switch { + case oid.Equal(oidNamedCurveP224): + return elliptic.P224() + case oid.Equal(oidNamedCurveP256): + return elliptic.P256() + case oid.Equal(oidNamedCurveP384): + return elliptic.P384() + case oid.Equal(oidNamedCurveP521): + return elliptic.P521() + } + return nil +} diff --git a/internal/wycheproof/ecdsa_compat_test.go b/internal/wycheproof/ecdsa_compat_test.go deleted file mode 100644 index 5880fb367e..0000000000 --- a/internal/wycheproof/ecdsa_compat_test.go +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright 2020 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. - -//go:build !go1.15 -// +build !go1.15 - -// ecdsa.VerifyASN1 was added in Go 1.15. - -package wycheproof - -import ( - "crypto/ecdsa" - "math/big" - - "golang.org/x/crypto/cryptobyte" - "golang.org/x/crypto/cryptobyte/asn1" -) - -func verifyASN1(pub *ecdsa.PublicKey, hash, sig []byte) bool { - var ( - r, s = &big.Int{}, &big.Int{} - inner cryptobyte.String - ) - input := cryptobyte.String(sig) - if !input.ReadASN1(&inner, asn1.SEQUENCE) || - !input.Empty() || - !inner.ReadASN1Integer(r) || - !inner.ReadASN1Integer(s) || - !inner.Empty() { - return false - } - return ecdsa.Verify(pub, hash, r, s) -} diff --git a/internal/wycheproof/ecdsa_go115_test.go b/internal/wycheproof/ecdsa_go115_test.go deleted file mode 100644 index e13e709087..0000000000 --- a/internal/wycheproof/ecdsa_go115_test.go +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright 2020 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. - -//go:build go1.15 -// +build go1.15 - -package wycheproof - -import ( - "crypto/ecdsa" -) - -func verifyASN1(pub *ecdsa.PublicKey, hash, sig []byte) bool { - return ecdsa.VerifyASN1(pub, hash, sig) -} diff --git a/internal/wycheproof/ecdsa_test.go b/internal/wycheproof/ecdsa_test.go index 81731f707b..42f3285fc4 100644 --- a/internal/wycheproof/ecdsa_test.go +++ b/internal/wycheproof/ecdsa_test.go @@ -9,126 +9,47 @@ import ( "testing" ) -func TestEcdsa(t *testing.T) { - // AsnSignatureTestVector - type AsnSignatureTestVector struct { - +func TestECDSA(t *testing.T) { + type ASNSignatureTestVector struct { // A brief description of the test case - Comment string `json:"comment,omitempty"` - + Comment string `json:"comment"` // A list of flags - Flags []string `json:"flags,omitempty"` - + Flags []string `json:"flags"` // The message to sign - Msg string `json:"msg,omitempty"` - + Msg string `json:"msg"` // Test result - Result string `json:"result,omitempty"` - - // An ASN encoded signature for msg - Sig string `json:"sig,omitempty"` - + Result string `json:"result"` + // An ASN.1 encoded signature for msg + Sig string `json:"sig"` // Identifier of the test case - TcId int `json:"tcId,omitempty"` + TcID int `json:"tcId"` } - // EcPublicKey - type EcPublicKey struct { - - // the EC group used by this public key - Curve interface{} `json:"curve,omitempty"` - - // the key size in bits - KeySize int `json:"keySize,omitempty"` - - // the key type - Type string `json:"type,omitempty"` - - // encoded public key point - Uncompressed string `json:"uncompressed,omitempty"` - - // the x-coordinate of the public key point - Wx string `json:"wx,omitempty"` - - // the y-coordinate of the public key point - Wy string `json:"wy,omitempty"` + type ECPublicKey struct { + // The EC group used by this public key + Curve interface{} `json:"curve"` } - // EcUnnamedGroup - type EcUnnamedGroup struct { - - // coefficient a of the elliptic curve equation - A string `json:"a,omitempty"` - - // coefficient b of the elliptic curve equation - B string `json:"b,omitempty"` - - // the x-coordinate of the generator - Gx string `json:"gx,omitempty"` - - // the y-coordinate of the generator - Gy string `json:"gy,omitempty"` - - // the cofactor - H int `json:"h,omitempty"` - - // the order of the generator - N string `json:"n,omitempty"` - - // the order of the underlying field - P string `json:"p,omitempty"` - - // an unnamed EC group over a prime field in Weierstrass form - Type string `json:"type,omitempty"` - } - - // EcdsaTestGroup - type EcdsaTestGroup struct { - - // unenocded EC public key - Key *EcPublicKey `json:"key,omitempty"` - + type ECDSATestGroup struct { + // Unencoded EC public key + Key *ECPublicKey `json:"key"` // DER encoded public key - KeyDer string `json:"keyDer,omitempty"` - - // Pem encoded public key - KeyPem string `json:"keyPem,omitempty"` - + KeyDER string `json:"keyDer"` // the hash function used for ECDSA - Sha string `json:"sha,omitempty"` - Tests []*AsnSignatureTestVector `json:"tests,omitempty"` - Type interface{} `json:"type,omitempty"` - } - - // Notes a description of the labels used in the test vectors - type Notes struct { + SHA string `json:"sha"` + Tests []*ASNSignatureTestVector `json:"tests"` } - // Root type Root struct { - - // the primitive tested in the test file - Algorithm string `json:"algorithm,omitempty"` - - // the version of the test vectors. - GeneratorVersion string `json:"generatorVersion,omitempty"` - - // additional documentation - Header []string `json:"header,omitempty"` - - // a description of the labels used in the test vectors - Notes *Notes `json:"notes,omitempty"` - - // the number of test vectors in this test - NumberOfTests int `json:"numberOfTests,omitempty"` - Schema interface{} `json:"schema,omitempty"` - TestGroups []*EcdsaTestGroup `json:"testGroups,omitempty"` + TestGroups []*ECDSATestGroup `json:"testGroups"` } flagsShouldPass := map[string]bool{ - // An encoded ASN.1 integer missing a leading zero is invalid, but accepted by some implementations. + // An encoded ASN.1 integer missing a leading zero is invalid, but + // accepted by some implementations. "MissingZero": false, - // A signature using a weaker hash than the EC params is not a security risk, as long as the hash is secure. + // A signature using a weaker hash than the EC params is not a security + // risk, as long as the hash is secure. // https://www.imperialviolet.org/2014/05/25/strengthmatching.html "WeakHash": true, } @@ -149,15 +70,15 @@ func TestEcdsa(t *testing.T) { if !supportedCurves[curve] { continue } - pub := decodePublicKey(tg.KeyDer).(*ecdsa.PublicKey) - h := parseHash(tg.Sha).New() + pub := decodePublicKey(tg.KeyDER).(*ecdsa.PublicKey) + h := parseHash(tg.SHA).New() for _, sig := range tg.Tests { h.Reset() h.Write(decodeHex(sig.Msg)) hashed := h.Sum(nil) - got := verifyASN1(pub, hashed, decodeHex(sig.Sig)) + got := ecdsa.VerifyASN1(pub, hashed, decodeHex(sig.Sig)) if want := shouldPass(sig.Result, sig.Flags, flagsShouldPass); got != want { - t.Errorf("tcid: %d, type: %s, comment: %q, wanted success: %t", sig.TcId, sig.Result, sig.Comment, want) + t.Errorf("tcid: %d, type: %s, comment: %q, wanted success: %t", sig.TcID, sig.Result, sig.Comment, want) } } } From 2cf3adece1227c48e1673f1c37d70357e1a6b9d3 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Fri, 6 May 2022 11:46:21 -0700 Subject: [PATCH 2/3] internal/wycheproof: skip truncated SHA-512 RSAPSS tests for boring On the boringcrypto builder, skip the RSAPSS tests that use the truncated SHA-512 hashes, since boringcrypto does not support them. Fixes #52670 Change-Id: I8caecd0f34eb6d2740372db2b641563e3965ac7c Reviewed-on: https://go-review.googlesource.com/c/crypto/+/404654 Run-TryBot: Roland Shoemaker Auto-Submit: Roland Shoemaker TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills --- internal/wycheproof/boring.go | 9 +++++++++ internal/wycheproof/notboring.go | 9 +++++++++ internal/wycheproof/rsa_pss_test.go | 21 +++++++++++++-------- 3 files changed, 31 insertions(+), 8 deletions(-) create mode 100644 internal/wycheproof/boring.go create mode 100644 internal/wycheproof/notboring.go diff --git a/internal/wycheproof/boring.go b/internal/wycheproof/boring.go new file mode 100644 index 0000000000..aefa3ab30d --- /dev/null +++ b/internal/wycheproof/boring.go @@ -0,0 +1,9 @@ +// Copyright 2022 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. + +//go:build boringcrypto + +package wycheproof + +const boringcryptoEnabled = true diff --git a/internal/wycheproof/notboring.go b/internal/wycheproof/notboring.go new file mode 100644 index 0000000000..746af130f1 --- /dev/null +++ b/internal/wycheproof/notboring.go @@ -0,0 +1,9 @@ +// Copyright 2022 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. + +//go:build !boringcrypto + +package wycheproof + +const boringcryptoEnabled = false diff --git a/internal/wycheproof/rsa_pss_test.go b/internal/wycheproof/rsa_pss_test.go index 365ca9229e..2ad9a4313b 100644 --- a/internal/wycheproof/rsa_pss_test.go +++ b/internal/wycheproof/rsa_pss_test.go @@ -112,17 +112,22 @@ func TestRsaPss(t *testing.T) { // works deterministically to auto-detect the length when // verifying, so these tests actually pass as they should. filesOverrideToPassZeroSLen := map[string][]int{ - "rsa_pss_2048_sha1_mgf1_20_test.json": []int{46, 47}, - "rsa_pss_2048_sha256_mgf1_0_test.json": []int{67, 68}, - "rsa_pss_2048_sha256_mgf1_32_test.json": []int{67, 68}, - "rsa_pss_2048_sha512_256_mgf1_28_test.json": []int{13, 14, 15}, - "rsa_pss_2048_sha512_256_mgf1_32_test.json": []int{13, 14}, - "rsa_pss_3072_sha256_mgf1_32_test.json": []int{67, 68}, - "rsa_pss_4096_sha256_mgf1_32_test.json": []int{67, 68}, - "rsa_pss_4096_sha512_mgf1_32_test.json": []int{136, 137}, + "rsa_pss_2048_sha1_mgf1_20_test.json": []int{46, 47}, + "rsa_pss_2048_sha256_mgf1_0_test.json": []int{67, 68}, + "rsa_pss_2048_sha256_mgf1_32_test.json": []int{67, 68}, + "rsa_pss_3072_sha256_mgf1_32_test.json": []int{67, 68}, + "rsa_pss_4096_sha256_mgf1_32_test.json": []int{67, 68}, + "rsa_pss_4096_sha512_mgf1_32_test.json": []int{136, 137}, // "rsa_pss_misc_test.json": nil, // TODO: This ones seems to be broken right now, but can enable later on. } + if !boringcryptoEnabled { + // boringcrypto doesn't support the truncated SHA-512 hashes, so only + // test them if boringcrypto isn't enabled. + filesOverrideToPassZeroSLen["rsa_pss_2048_sha512_256_mgf1_28_test.json"] = []int{13, 14, 15} + filesOverrideToPassZeroSLen["rsa_pss_2048_sha512_256_mgf1_32_test.json"] = []int{13, 14} + } + for f := range filesOverrideToPassZeroSLen { var root Root readTestVector(t, f, &root) From c6db032c6c884ccc62755e1abf1214e99b8cea5f Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Wed, 11 May 2022 12:14:18 -0700 Subject: [PATCH 3/3] acme/autocert/internal/acmetest: don't validate in goroutine In the test server, rather than spawning a goroutine to validate challenges, block on the validation before responding to the client. This prevents a test race, where testing.T.Logf is called after the test is completed. While this has a slight behavioral difference to some production ACME server implementations (although is behavior allowed in the spec), the change has little material impact on what we are testing, since previously the validation would happen so quickly that it would be indistinguishable from the new blocking behavior (i.e. we would not be sending multiple requests during polling previously.) Fixes golang/go#52170 Change-Id: I75e3b2da69ddc2302be25a99f1b1151ed0f4af9b Reviewed-on: https://go-review.googlesource.com/c/crypto/+/405548 Run-TryBot: Roland Shoemaker TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills --- acme/autocert/internal/acmetest/ca.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/autocert/internal/acmetest/ca.go b/acme/autocert/internal/acmetest/ca.go index fa33987913..0a5ebe7ab7 100644 --- a/acme/autocert/internal/acmetest/ca.go +++ b/acme/autocert/internal/acmetest/ca.go @@ -380,7 +380,7 @@ func (ca *CAServer) handle(w http.ResponseWriter, r *http.Request) { ca.httpErrorf(w, http.StatusBadRequest, "challenge accept: %v", err) return } - go ca.validateChallenge(a, typ) + ca.validateChallenge(a, typ) w.Write([]byte("{}")) // Get authorization status requests.