fix: audit issues by verichain (#451)

* fix audit issues by verichain * fix: add statement check for nagqu
bnb-chain · Sep 6, 2023 · d33cb72 · d33cb72
1 parent c66aea3
commit d33cb72
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 8 deletions.
diff --git a/types/grn.go b/types/grn.go
@@ -18,8 +18,8 @@ const (
 )
 
 var (
-	validGRNRegex       = regexp.MustCompile("^grn:([b|o|g]):([^:]*):([^:]*)$")
-	validGRNRegexNoWild = regexp.MustCompile("^grn:([b|o|g]):([^:]*):([^:*]*)$")
+	validGRNRegex       = regexp.MustCompile("^grn:([bog]):([^:]*):([^:]*)$")
+	validGRNRegexNoWild = regexp.MustCompile("^grn:([bog]):([^:]*):([^:*]*)$")
 )
 
 // GRN define a standard ResourceName format, full name: GreenFieldResourceName

diff --git a/x/permission/keeper/keeper.go b/x/permission/keeper/keeper.go
@@ -15,6 +15,7 @@ import (
 	"github.com/bnb-chain/greenfield/types/resource"
 	"github.com/bnb-chain/greenfield/x/permission/types"
 	storagetypes "github.com/bnb-chain/greenfield/x/storage/types"
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
 type (
@@ -298,7 +299,7 @@ func (k Keeper) GetPolicyForGroup(ctx sdk.Context, resourceID math.Uint,
 	for _, item := range policyGroup.Items {
 		k.Logger(ctx).Info(fmt.Sprintf("GetPolicy, policyID: %s, groupID: %s", item.PolicyId.String(), item.GroupId.String()))
 		if item.GroupId.Equal(groupID) {
-			return k.MustGetPolicyByID(ctx, item.PolicyId), true
+			return k.GetPolicyByID(ctx, item.PolicyId)
 		}
 	}
 	return nil, false
@@ -325,7 +326,9 @@ func (k Keeper) DeletePolicy(ctx sdk.Context, principal *types.Principal, resour
 		if err != nil {
 			return math.ZeroUint(), err
 		}
-		bz := store.Get(types.GetPolicyForGroupKey(resourceID, resourceType))
+		policyGroupKey := types.GetPolicyForGroupKey(resourceID, resourceType)
+		bz := store.Get(policyGroupKey)
+		updated := false
 		if bz != nil {
 			policyGroup := types.PolicyGroup{}
 			k.cdc.MustUnmarshal(bz, &policyGroup)
@@ -342,11 +345,20 @@ func (k Keeper) DeletePolicy(ctx sdk.Context, principal *types.Principal, resour
 					if policy.ExpirationTime != nil {
 						store.Delete(types.PolicyPrefixQueue(policy.ExpirationTime, policy.Id.Bytes()))
 					}
+					updated = true
+					break // Only one should be deleted
 				}
 			}
-			// delete the key if value is empty
-			if len(policyGroup.Items) == 0 {
-				store.Delete(types.GetPolicyForGroupKey(resourceID, resourceType))
+			if updated {
+				if len(policyGroup.Items) == 0 {
+					// delete the key if value is empty
+					store.Delete(policyGroupKey)
+				} else {
+					if ctx.IsUpgraded(upgradetypes.Nagqu) {
+						// persist policy group after updated.
+						store.Set(policyGroupKey, k.cdc.MustMarshal(&policyGroup))
+					}
+				}
 			}
 		}
 	} else {

diff --git a/x/permission/types/types.go b/x/permission/types/types.go
@@ -180,7 +180,7 @@ func (s *Statement) ValidateBasic(resType resource.ResourceType) error {
 			var grn gnfd.GRN
 			err := grn.ParseFromString(r, true)
 			if err != nil {
-				return err
+				return ErrInvalidStatement.Wrapf("GRN parse from string failed, err: %s", err)
 			}
 		}
 
@@ -210,3 +210,31 @@ func (s *Statement) ValidateBasic(resType resource.ResourceType) error {
 	}
 	return nil
 }
+
+func (s *Statement) ValidateAfterNagqu(resType resource.ResourceType) error {
+	if s.Effect == EFFECT_UNSPECIFIED {
+		return ErrInvalidStatement.Wrap("Please specify the Effect explicitly. Not allowed set EFFECT_UNSPECIFIED")
+	}
+	switch resType {
+	case resource.RESOURCE_TYPE_UNSPECIFIED:
+		return ErrInvalidStatement.Wrap("Please specify the ResourceType explicitly. Not allowed set RESOURCE_TYPE_UNSPECIFIED")
+	case resource.RESOURCE_TYPE_BUCKET:
+		for _, r := range s.Resources {
+			_, err := regexp.Compile(r)
+			if err != nil {
+				return ErrInvalidStatement.Wrapf("The Resources regexp compile failed, err: %s", err)
+			}
+		}
+	case resource.RESOURCE_TYPE_OBJECT:
+		if s.Resources != nil {
+			return ErrInvalidStatement.Wrap("The Resources option can only be used at the bucket level. ")
+		}
+	case resource.RESOURCE_TYPE_GROUP:
+		if s.Resources != nil {
+			return ErrInvalidStatement.Wrap("The Resources option can only be used at the bucket level. ")
+		}
+	default:
+		return ErrInvalidStatement.Wrap("unknown resource type.")
+	}
+	return nil
+}
diff --git a/x/storage/keeper/msg_server.go b/x/storage/keeper/msg_server.go
@@ -16,6 +16,7 @@ import (
 	"github.com/bnb-chain/greenfield/x/storage/types"
 	storagetypes "github.com/bnb-chain/greenfield/x/storage/types"
 	virtualgroupmoduletypes "github.com/bnb-chain/greenfield/x/virtualgroup/types"
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
 type msgServer struct {
@@ -369,6 +370,12 @@ func (k msgServer) PutPolicy(goCtx context.Context, msg *types.MsgPutPolicy) (*t
 		if s.ExpirationTime != nil && s.ExpirationTime.Before(ctx.BlockTime()) {
 			return nil, permtypes.ErrPermissionExpired.Wrapf("The specified statement expiration time is less than the current block time, block time: %s", ctx.BlockTime().String())
 		}
+		if ctx.IsUpgraded(upgradetypes.Nagqu) {
+			err := s.ValidateAfterNagqu(grn.ResourceType())
+			if err != nil {
+				return nil, err
+			}
+		}
 	}
 
 	policy := &permtypes.Policy{