[FAB-15845] Additional validation checks

- refactor chaincode name and version regexp
- user chaincode name cannot be same as system cc
- no duplicate namespace in RWSet

Change-Id: Id81133244cfc62888227bd4799f520d29211a7f7
Signed-off-by: Danny Cao <dcao@us.ibm.com>
Signed-off-by: Wenjian Qiao <wenjianq@gmail.com>

Author: caod123

core/committer/txvalidator/v14/validator_test.go

@@ -43,6 +43,8 @@ import (
 	"github.com/hyperledger/fabric/msp/mgmt"
 	msptesttools "github.com/hyperledger/fabric/msp/mgmt/testtools"
 	"github.com/hyperledger/fabric/protos/common"
+	"github.com/hyperledger/fabric/protos/ledger/rwset"
+	"github.com/hyperledger/fabric/protos/ledger/rwset/kvrwset"
 	mb "github.com/hyperledger/fabric/protos/msp"
 	"github.com/hyperledger/fabric/protos/peer"
 	pb "github.com/hyperledger/fabric/protos/peer"
@@ -403,6 +405,64 @@ func testInvokeOK(t *testing.T, l ledger.PeerLedger, v txvalidator.Validator) {
 	assertValid(b, t)
 }
 
+func TestInvokeNOKDuplicateNs(t *testing.T) {
+	t.Run("1.2Capability", func(t *testing.T) {
+		l, v, cleanup := setupLedgerAndValidatorWithV12Capabilities(t)
+		defer cleanup()
+
+		testInvokeNOKDuplicateNs(t, l, v)
+	})
+
+	t.Run("1.3Capability", func(t *testing.T) {
+		l, v, cleanup := setupLedgerAndValidatorWithV13Capabilities(t)
+		defer cleanup()
+
+		testInvokeNOKDuplicateNs(t, l, v)
+	})
+}
+
+func testInvokeNOKDuplicateNs(t *testing.T, l ledger.PeerLedger, v txvalidator.Validator) {
+	ccID := "mycc"
+
+	putCCInfo(l, ccID, signedByAnyMember([]string{"SampleOrg"}), t)
+
+	// note that this read-write set has two read-write sets for the same namespace and key
+	txrws := &rwset.TxReadWriteSet{
+		DataModel: rwset.TxReadWriteSet_KV,
+		NsRwset: []*rwset.NsReadWriteSet{
+			{
+				Namespace: "mycc",
+				Rwset: protoutil.MarshalOrPanic(&kvrwset.KVRWSet{
+					Writes: []*kvrwset.KVWrite{
+						{
+							Key:   "foo",
+							Value: []byte("bar1"),
+						},
+					},
+				}),
+			},
+			{
+				Namespace: "mycc",
+				Rwset: protoutil.MarshalOrPanic(&kvrwset.KVRWSet{
+					Writes: []*kvrwset.KVWrite{
+						{
+							Key:   "foo",
+							Value: []byte("bar2"),
+						},
+					},
+				}),
+			},
+		},
+	}
+
+	tx := getEnv(ccID, nil, protoutil.MarshalOrPanic(txrws), t)
+	b := &common.Block{Data: &common.BlockData{Data: [][]byte{protoutil.MarshalOrPanic(tx)}}, Header: &common.BlockHeader{Number: 2}}
+
+	err := v.Validate(b)
+	assert.NoError(t, err)
+	assertInvalid(b, t, peer.TxValidationCode_ILLEGAL_WRITESET)
+}
+
 func TestInvokeNoRWSet(t *testing.T) {
 	plugin := &mocks.Plugin{}
 	plugin.On("Init", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)

core/committer/txvalidator/v14/vscc_validator.go

@@ -118,7 +118,15 @@ func (v *VsccValidatorImpl) VSCCValidateTx(seq int, payload *common.Payload, env
 		}
 	}
 
+	namespaces := make(map[string]struct{})
 	for _, ns := range txRWSet.NsRwSets {
+		// check to make sure there is no duplicate namespace in txRWSet
+		if _, ok := namespaces[ns.NameSpace]; ok {
+			return errors.Errorf("duplicate namespace '%s' in txRWSet", ns.NameSpace),
+				peer.TxValidationCode_ILLEGAL_WRITESET
+		}
+		namespaces[ns.NameSpace] = struct{}{}
+
 		if !v.txWritesToNamespace(ns) {
 			continue
 		}

core/committer/txvalidator/v20/plugindispatcher/dispatcher.go

@@ -168,7 +168,16 @@ func (v *dispatcherImpl) Dispatch(seq int, payload *common.Payload, envBytes []b
 		}
 	}
 
+	namespaces := make(map[string]struct{})
 	for _, ns := range txRWSet.NsRwSets {
+		// check to make sure there is no duplicate namespace in txRWSet
+		if _, ok := namespaces[ns.NameSpace]; ok {
+			logger.Errorf("duplicate namespace '%s' in txRWSet", ns.NameSpace)
+			return errors.Errorf("duplicate namespace '%s' in txRWSet", ns.NameSpace),
+				peer.TxValidationCode_ILLEGAL_WRITESET
+		}
+		namespaces[ns.NameSpace] = struct{}{}
+
 		if v.txWritesToNamespace(ns) {
 			wrNamespace[ns.NameSpace] = true
 		}

core/committer/txvalidator/v20/validator_test.go

@@ -39,6 +39,8 @@ import (
 	"github.com/hyperledger/fabric/msp/mgmt"
 	msptesttools "github.com/hyperledger/fabric/msp/mgmt/testtools"
 	"github.com/hyperledger/fabric/protos/common"
+	"github.com/hyperledger/fabric/protos/ledger/rwset"
+	"github.com/hyperledger/fabric/protos/ledger/rwset/kvrwset"
 	protosmsp "github.com/hyperledger/fabric/protos/msp"
 	"github.com/hyperledger/fabric/protos/peer"
 	protospeer "github.com/hyperledger/fabric/protos/peer"
@@ -310,6 +312,56 @@ func TestInvokeOK(t *testing.T) {
 	assertValid(b, t)
 }
 
+func TestInvokeNOKDuplicateNs(t *testing.T) {
+	ccID := "mycc"
+
+	v, mockQE, _, _ := setupValidator()
+
+	mockQE.On("GetState", "lscc", ccID).Return(protoutil.MarshalOrPanic(&ccp.ChaincodeData{
+		Name:    ccID,
+		Version: ccVersion,
+		Vscc:    "vscc",
+		Policy:  signedByAnyMember([]string{"SampleOrg"}),
+	}), nil)
+	mockQE.On("GetStateMetadata", ccID, "key").Return(nil, nil)
+
+	// note that this read-write set has two read-write sets for the same namespace and key
+	txrws := &rwset.TxReadWriteSet{
+		DataModel: rwset.TxReadWriteSet_KV,
+		NsRwset: []*rwset.NsReadWriteSet{
+			{
+				Namespace: "mycc",
+				Rwset: protoutil.MarshalOrPanic(&kvrwset.KVRWSet{
+					Writes: []*kvrwset.KVWrite{
+						{
+							Key:   "foo",
+							Value: []byte("bar1"),
+						},
+					},
+				}),
+			},
+			{
+				Namespace: "mycc",
+				Rwset: protoutil.MarshalOrPanic(&kvrwset.KVRWSet{
+					Writes: []*kvrwset.KVWrite{
+						{
+							Key:   "foo",
+							Value: []byte("bar2"),
+						},
+					},
+				}),
+			},
+		},
+	}
+
+	tx := getEnv(ccID, nil, protoutil.MarshalOrPanic(txrws), t)
+	b := &common.Block{Data: &common.BlockData{Data: [][]byte{protoutil.MarshalOrPanic(tx)}}, Header: &common.BlockHeader{Number: 2}}
+
+	err := v.Validate(b)
+	assert.NoError(t, err)
+	assertInvalid(b, t, peer.TxValidationCode_ILLEGAL_WRITESET)
+}
+
 func TestInvokeNoRWSet(t *testing.T) {
 	ccID := "mycc"
 

core/handlers/validation/builtin/v12/validation_logic.go

@@ -35,7 +35,19 @@ import (
 	"github.com/pkg/errors"
 )
 
-var logger = flogging.MustGetLogger("vscc")
+var (
+	logger = flogging.MustGetLogger("vscc")
+
+	// currently defined system chaincode names that shouldn't
+	// be allowed as user-defined chaincode names
+	systemChaincodeNames = map[string]struct{}{
+		"cscc": {},
+		"escc": {},
+		"lscc": {},
+		"qscc": {},
+		"vscc": {},
+	}
+)
 
 const (
 	DUPLICATED_IDENTITY_ERROR = "Endorsement policy evaluation failure might be caused by duplicated identities"
@@ -524,6 +536,11 @@ func (vscc *Validator) ValidateLSCCInvocation(
 			return policyErr(fmt.Errorf("GetChaincodeDeploymentSpec error %s", err))
 		}
 
+		if cdsArgs == nil || cdsArgs.ChaincodeSpec == nil || cdsArgs.ChaincodeSpec.ChaincodeId == nil ||
+			cap.Action == nil || cap.Action.ProposalResponsePayload == nil {
+			return policyErr(fmt.Errorf("VSCC error: invocation of lscc(%s) does not have appropriate arguments", lsccFunc))
+		}
+
 		err = packaging.NewRegistry(
 			// XXX We should definitely _not_ have this external dependency in VSCC
 			// as adding a platform could cause non-determinism.  This is yet another
@@ -538,9 +555,22 @@ func (vscc *Validator) ValidateLSCCInvocation(
 			return policyErr(fmt.Errorf("failed to validate deployment spec: %s", err))
 		}
 
-		if cdsArgs == nil || cdsArgs.ChaincodeSpec == nil || cdsArgs.ChaincodeSpec.ChaincodeId == nil ||
-			cap.Action == nil || cap.Action.ProposalResponsePayload == nil {
-			return policyErr(fmt.Errorf("VSCC error: invocation of lscc(%s) does not have appropriate arguments", lsccFunc))
+		// validate chaincode name
+		ccName := cdsArgs.ChaincodeSpec.ChaincodeId.Name
+		// it must comply with the lscc.ChaincodeNameRegExp
+		if !lscc.ChaincodeNameRegExp.MatchString(ccName) {
+			return policyErr(errors.Errorf("invalid chaincode name '%s'", ccName))
+		}
+		// it can't match the name of one of the system chaincodes
+		if _, in := systemChaincodeNames[ccName]; in {
+			return policyErr(errors.Errorf("chaincode name '%s' is reserved for system chaincodes", ccName))
+		}
+
+		// validate chaincode version
+		ccVersion := cdsArgs.ChaincodeSpec.ChaincodeId.Version
+		// it must comply with the lscc.ChaincodeVersionRegExp
+		if !lscc.ChaincodeVersionRegExp.MatchString(ccVersion) {
+			return policyErr(errors.Errorf("invalid chaincode version '%s'", ccVersion))
 		}
 
 		// get the rwset

core/handlers/validation/builtin/v12/validation_logic_test.go

@@ -889,6 +889,66 @@ func TestValidateDeployNoLedger(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestValidateDeployNOKNilChaincodeSpec(t *testing.T) {
+	state := make(map[string]map[string][]byte)
+	mp := (&scc.MocksccProviderFactory{
+		Qe:                    lm.NewMockQueryExecutor(state),
+		ApplicationConfigBool: true,
+		ApplicationConfigRv:   &mc.MockApplication{CapabilitiesRv: &mc.MockApplicationCapabilities{}},
+	}).NewSystemChaincodeProvider().(*scc.MocksccProviderImpl)
+
+	v := newValidationInstance(state)
+
+	mockAclProvider := &aclmocks.MockACLProvider{}
+	lccc := lscc.New(mp, mockAclProvider, mockMSPIDGetter, &mockPolicyChecker{})
+	stublccc := shimtest.NewMockStub("lscc", lccc)
+	state["lscc"] = stublccc.State
+
+	ccname := "mycc"
+	ccver := "1"
+
+	defaultPolicy, err := getSignedByMSPAdminPolicy(mspid)
+	assert.NoError(t, err)
+	res, err := createCCDataRWset(ccname, ccname, ccver, defaultPolicy)
+	assert.NoError(t, err)
+
+	// Create a ChaincodeDeploymentSpec with nil ChaincodeSpec for negative test
+	cdsBytes, err := proto.Marshal(&peer.ChaincodeDeploymentSpec{})
+	assert.NoError(t, err)
+
+	// ChaincodeDeploymentSpec/ChaincodeSpec are derived from cdsBytes (i.e., cis.ChaincodeSpec.Input.Args[2])
+	cis := &peer.ChaincodeInvocationSpec{
+		ChaincodeSpec: &peer.ChaincodeSpec{
+			ChaincodeId: &peer.ChaincodeID{Name: "lscc"},
+			Input: &peer.ChaincodeInput{
+				Args: [][]byte{[]byte(lscc.DEPLOY), []byte("barf"), cdsBytes},
+			},
+			Type: peer.ChaincodeSpec_GOLANG,
+		},
+	}
+
+	prop, _, err := protoutil.CreateProposalFromCIS(common.HeaderType_ENDORSER_TRANSACTION, util.GetTestChainID(), cis, sid)
+	assert.NoError(t, err)
+
+	ccid := &peer.ChaincodeID{Name: ccname, Version: ccver}
+
+	presp, err := protoutil.CreateProposalResponse(prop.Header, prop.Payload, &peer.Response{Status: 200}, res, nil, ccid, nil, id)
+	assert.NoError(t, err)
+
+	env, err := protoutil.CreateSignedTx(prop, id, presp)
+	assert.NoError(t, err)
+
+	// good path: signed by the right MSP
+	policy, err := getSignedByMSPMemberPolicy(mspid)
+	if err != nil {
+		t.Fatalf("failed getting policy, err %s", err)
+	}
+
+	b := &common.Block{Data: &common.BlockData{Data: [][]byte{protoutil.MarshalOrPanic(env)}}, Header: &common.BlockHeader{}}
+	err = v.Validate(b, "lscc", 0, 0, policy)
+	assert.EqualError(t, err, "VSCC error: invocation of lscc(deploy) does not have appropriate arguments")
+}
+
 func TestValidateDeployOK(t *testing.T) {
 	state := make(map[string]map[string][]byte)
 	mp := (&scc.MocksccProviderFactory{
@@ -933,6 +993,65 @@ func TestValidateDeployOK(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestValidateDeployNOK(t *testing.T) {
+	var testCases = []struct {
+		description string
+		ccName      string
+		ccVersion   string
+		errMsg      string
+	}{
+		{description: "empty cc name", ccName: "", ccVersion: "1", errMsg: "invalid chaincode name ''"},
+		{description: "bad first character in cc name", ccName: "_badname", ccVersion: "1.2", errMsg: "invalid chaincode name '_badname'"},
+		{description: "bad character in cc name", ccName: "bad.name", ccVersion: "1-5", errMsg: "invalid chaincode name 'bad.name'"},
+		{description: "empty cc version", ccName: "1good_name", ccVersion: "", errMsg: "invalid chaincode version ''"},
+		{description: "bad cc version", ccName: "good-name", ccVersion: "$badversion", errMsg: "invalid chaincode version '$badversion'"},
+		{description: "use system cc name", ccName: "qscc", ccVersion: "2.1", errMsg: "chaincode name 'qscc' is reserved for system chaincodes"},
+	}
+
+	// create validator and policy
+	state := make(map[string]map[string][]byte)
+	mp := (&scc.MocksccProviderFactory{
+		Qe:                    lm.NewMockQueryExecutor(state),
+		ApplicationConfigBool: true,
+		ApplicationConfigRv:   &mc.MockApplication{CapabilitiesRv: &mc.MockApplicationCapabilities{}},
+	}).NewSystemChaincodeProvider().(*scc.MocksccProviderImpl)
+
+	v := newValidationInstance(state)
+
+	mockAclProvider := &aclmocks.MockACLProvider{}
+	lccc := lscc.New(mp, mockAclProvider, mockMSPIDGetter, &mockPolicyChecker{})
+	stublccc := shimtest.NewMockStub("lscc", lccc)
+	state["lscc"] = stublccc.State
+
+	policy, err := getSignedByMSPAdminPolicy(mspid)
+	assert.NoError(t, err)
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			testChaincodeDeployNOK(t, tc.ccName, tc.ccVersion, tc.errMsg, v, policy)
+		})
+	}
+}
+
+func testChaincodeDeployNOK(t *testing.T, ccName, ccVersion, errMsg string, v *Validator, policy []byte) {
+	res, err := createCCDataRWset(ccName, ccName, ccVersion, policy)
+	assert.NoError(t, err)
+
+	tx, err := createLSCCTx(ccName, ccVersion, lscc.DEPLOY, res)
+	if err != nil {
+		t.Fatalf("createTx returned err %s", err)
+	}
+
+	envBytes, err := protoutil.GetBytesEnvelope(tx)
+	if err != nil {
+		t.Fatalf("GetBytesEnvelope returned err %s", err)
+	}
+
+	b := &common.Block{Data: &common.BlockData{Data: [][]byte{envBytes}}, Header: &common.BlockHeader{}}
+	err = v.Validate(b, "lscc", 0, 0, policy)
+	assert.EqualError(t, err, errMsg)
+}
+
 func TestValidateDeployWithCollection(t *testing.T) {
 	state := make(map[string]map[string][]byte)
 	mp := (&scc.MocksccProviderFactory{