From 8883d7161ac7a006caf5780566e520b7ce884c0c Mon Sep 17 00:00:00 2001 From: Matthew Sykes Date: Mon, 8 Feb 2021 18:15:16 -0500 Subject: [PATCH] Add test to exercise signature validation change The Go team has tightened up ECDSA signature validation in the standard library. If a CRL contains a signature that used to validate but no longer does, identities revoked by the CRL would no longer be considered invalid. This commit adds a test that asserts on the go 1.14 behavior and fails on go 1.15. Signed-off-by: Matthew Sykes --- msp/configbuilder.go | 9 ++-- msp/revocation_test.go | 120 ++++++++++++++++++++++++++++++++--------- 2 files changed, 101 insertions(+), 28 deletions(-) diff --git a/msp/configbuilder.go b/msp/configbuilder.go index 2579732ff92..f670d4073fb 100644 --- a/msp/configbuilder.go +++ b/msp/configbuilder.go @@ -354,11 +354,12 @@ func getMspConfig(dir string, ID string, sigid *msp.SigningIdentityInfo) (*msp.M FabricNodeOus: nodeOUs, } - fmpsjs, _ := proto.Marshal(fmspconf) - - mspconf := &msp.MSPConfig{Config: fmpsjs, Type: int32(FABRIC)} + fmpsjs, err := proto.Marshal(fmspconf) + if err != nil { + return nil, err + } - return mspconf, nil + return &msp.MSPConfig{Config: fmpsjs, Type: int32(FABRIC)}, nil } func loadCertificateAt(dir, certificatePath string, ouType string) []byte { diff --git a/msp/revocation_test.go b/msp/revocation_test.go index 198495b781d..20c8f521e64 100644 --- a/msp/revocation_test.go +++ b/msp/revocation_test.go @@ -17,39 +17,111 @@ limitations under the License. package msp import ( + "crypto/x509" + "encoding/asn1" + "encoding/pem" + "math/big" "path/filepath" "testing" + "github.com/golang/protobuf/proto" "github.com/hyperledger/fabric-protos-go/msp" "github.com/hyperledger/fabric/bccsp/sw" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRevocation(t *testing.T) { - // testdata/revocation - // 1) a key and a signcert (used to populate the default signing identity); - // 2) cacert is the CA that signed the intermediate; - // 3) a revocation list that revokes signcert - thisMSP := getLocalMSP(t, "testdata/revocation") - - id, err := thisMSP.GetDefaultSigningIdentity() - assert.NoError(t, err) - - // the certificate associated to this id is revoked and so validation should fail! - err = id.Validate() - assert.Error(t, err) - - // This MSP is identical to the previous one, with only 1 difference: - // the signature on the CRL is invalid - thisMSP = getLocalMSP(t, "testdata/revocation2") - - id, err = thisMSP.GetDefaultSigningIdentity() - assert.NoError(t, err) - - // the certificate associated to this id is revoked but the signature on the CRL is invalid - // so validation should succeed - err = id.Validate() - assert.NoError(t, err, "Identity found revoked although the signature over the CRL is invalid") + t.Run("ValidCRLSignature", func(t *testing.T) { + // testdata/revocation + // 1) a key and a signcert (used to populate the default signing identity); + // 2) cacert is the CA that signed the intermediate; + // 3) a revocation list that revokes signcert + thisMSP := getLocalMSP(t, "testdata/revocation") + + id, err := thisMSP.GetDefaultSigningIdentity() + require.NoError(t, err) + + // the certificate associated to this id is revoked and so validation should fail! + err = id.Validate() + require.Error(t, err) + }) + + t.Run("MalformedCRLSignature", func(t *testing.T) { + // This test appends an extra int to the CRL signature. This extra data is + // ignored in go 1.14, the signature is considered valid, and the identity + // is treated as revoked. + // + // In go 1.15 the CheckCRLSignature implementation is more strict and the + // CRL signature is treated as invalid and the identity is not treated as + // revoked. + // + // This behavior change needs to be mitigated between the two versions. + conf, err := GetLocalMspConfig("testdata/revocation", nil, "SampleOrg") + require.NoError(t, err) + + // Unmarshal the config + var mspConfig msp.FabricMSPConfig + err = proto.Unmarshal(conf.Config, &mspConfig) + require.NoError(t, err) + require.Len(t, mspConfig.RevocationList, 1) + crl, err := x509.ParseCRL(mspConfig.RevocationList[0]) + require.NoError(t, err) + + // Decode the CRL signature + var sig struct{ R, S *big.Int } + _, err = asn1.Unmarshal(crl.SignatureValue.Bytes, &sig) + require.NoError(t, err) + + // Extend the signature with another value + extendedSig := struct{ R, S, T *big.Int }{sig.R, sig.S, big.NewInt(100)} + longSigBytes, err := asn1.Marshal(extendedSig) + require.NoError(t, err) + + // Use the extended signature in the CRL + crl.SignatureValue.Bytes = longSigBytes + crl.SignatureValue.BitLength = 8 * len(longSigBytes) + crlBytes, err := asn1.Marshal(*crl) + require.NoError(t, err) + mspConfig.RevocationList[0] = pem.EncodeToMemory(&pem.Block{Type: "X509 CRL", Bytes: crlBytes}) + + // Remarshal the configuration + conf.Config, err = proto.Marshal(&mspConfig) + require.NoError(t, err) + + ks, err := sw.NewFileBasedKeyStore(nil, filepath.Join("testdata/revocation", "keystore"), true) + require.NoError(t, err) + cryptoProvider, err := sw.NewDefaultSecurityLevelWithKeystore(ks) + require.NoError(t, err) + + thisMSP, err := NewBccspMspWithKeyStore(MSPv1_0, ks, cryptoProvider) + require.NoError(t, err) + + err = thisMSP.Setup(conf) + require.NoError(t, err) + + id, err := thisMSP.GetDefaultSigningIdentity() + require.NoError(t, err) + + // the cert associated with this id is revoked and the extra info on the + // signature is ignored so validation should fail! + err = id.Validate() + require.EqualError(t, err, "could not validate identity against certification chain: The certificate has been revoked") + }) + + t.Run("InvalidCRLSignature", func(t *testing.T) { + // This MSP is identical to the previous one, with only 1 difference: + // the signature on the CRL is invalid + thisMSP := getLocalMSP(t, "testdata/revocation2") + + id, err := thisMSP.GetDefaultSigningIdentity() + require.NoError(t, err) + + // the certificate associated to this id is revoked but the signature on the CRL is invalid + // so validation should succeed + err = id.Validate() + require.NoError(t, err, "Identity found revoked although the signature over the CRL is invalid") + }) } func TestIdentityPolicyPrincipalAgainstRevokedIdentity(t *testing.T) {