chore: Avoid out-of-bounds reads in go-lang code

Replace panics with friendly error messages.
Agoric · Dec 19, 2023 · 7f3b821 · 7f3b821
1 parent 7d63ca3
commit 7f3b821
Show file tree

Hide file tree

Showing 6 changed files with 62 additions and 46 deletions.
diff --git a/golang/cosmos/x/swingset/client/cli/tx.go b/golang/cosmos/x/swingset/client/cli/tx.go
@@ -2,6 +2,7 @@ package cli
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"strings"
 
@@ -56,20 +57,18 @@ func GetCmdDeliver() *cobra.Command {
 			}
 
 			jsonIn := args[0]
-			if jsonIn[0] == '@' {
-				fname := args[0][1:]
+			if strings.HasPrefix(jsonIn, "@") {
+				var jsonBytes []byte
+				fname := jsonIn[1:]
 				if fname == "-" {
-					// Reading from stdin.
-					if _, err := fmt.Scanln(&jsonIn); err != nil {
-						return err
-					}
+					jsonBytes, err = io.ReadAll(os.Stdin)
 				} else {
-					jsonBytes, err := os.ReadFile(fname)
-					if err != nil {
-						return err
-					}
-					jsonIn = string(jsonBytes)
+					jsonBytes, err = os.ReadFile(fname)
 				}
+				if err != nil {
+					return err
+				}
+				jsonIn = string(jsonBytes)
 			}
 			msgs, err := types.UnmarshalMessagesJSON(jsonIn)
 			if err != nil {
@@ -102,20 +101,18 @@ func GetCmdInstallBundle() *cobra.Command {
 			}
 
 			jsonIn := args[0]
-			if jsonIn[0] == '@' {
-				fname := args[0][1:]
+			if strings.HasPrefix(jsonIn, "@") {
+				var jsonBytes []byte
+				fname := jsonIn[1:]
 				if fname == "-" {
-					// Reading from stdin.
-					if _, err := fmt.Scanln(&jsonIn); err != nil {
-						return err
-					}
+					jsonBytes, err = io.ReadAll(os.Stdin)
 				} else {
-					jsonBytes, err := os.ReadFile(fname)
-					if err != nil {
-						return err
-					}
-					jsonIn = string(jsonBytes)
+					jsonBytes, err = os.ReadFile(fname)
 				}
+				if err != nil {
+					return err
+				}
+				jsonIn = string(jsonBytes)
 			}
 
 			msg := types.NewMsgInstallBundle(jsonIn, cctx.GetFromAddress())
@@ -160,6 +157,8 @@ func GetCmdProvisionOne() *cobra.Command {
 				return err
 			}
 
+			nickname := args[0]
+
 			addr, err := sdk.AccAddressFromBech32(args[1])
 			if err != nil {
 				return err
@@ -170,7 +169,7 @@ func GetCmdProvisionOne() *cobra.Command {
 				powerFlags = strings.Split(args[2], ",")
 			}
 
-			msg := types.NewMsgProvision(args[0], addr, powerFlags, cctx.GetFromAddress())
+			msg := types.NewMsgProvision(nickname, addr, powerFlags, cctx.GetFromAddress())
 			if err := msg.ValidateBasic(); err != nil {
 				return err
 			}

diff --git a/golang/cosmos/x/swingset/keeper/msg_server.go b/golang/cosmos/x/swingset/keeper/msg_server.go
@@ -45,13 +45,11 @@ func (keeper msgServer) routeAction(ctx sdk.Context, msg vm.ControllerAdmissionM
 func (keeper msgServer) DeliverInbound(goCtx context.Context, msg *types.MsgDeliverInbound) (*types.MsgDeliverInboundResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
+	// msg.Nums and msg.Messages must be zipped into an array of [num, message] pairs.
 	messages := make([][]interface{}, len(msg.Messages))
 	for i, message := range msg.Messages {
-		messages[i] = make([]interface{}, 2)
-		messages[i][0] = msg.Nums[i]
-		messages[i][1] = message
+		messages[i] = []interface{}{msg.Nums[i], message}
 	}
-
 	action := &deliverInboundAction{
 		Type:        "DELIVER_INBOUND",
 		Peer:        msg.Submitter.String(),

diff --git a/golang/cosmos/x/swingset/keeper/querier.go b/golang/cosmos/x/swingset/keeper/querier.go
@@ -24,11 +24,21 @@ const (
 // NewQuerier is the module level router for state queries
 func NewQuerier(keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) sdk.Querier {
 	return func(ctx sdk.Context, path []string, req abci.RequestQuery) (res []byte, err error) {
-		switch path[0] {
+		var queryType string
+		if len(path) > 0 {
+			queryType = path[0]
+		}
+		switch queryType {
 		case QueryEgress:
+			if len(path) < 2 || path[1] == "" {
+				return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "missing egress address")
+			}
 			return queryEgress(ctx, path[1], req, keeper, legacyQuerierCdc)
 		case QueryMailbox:
-			return queryMailbox(ctx, path[1:], req, keeper, legacyQuerierCdc)
+			if len(path) < 2 || path[1] == "" {
+				return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "missing mailbox peer")
+			}
+			return queryMailbox(ctx, path[1], req, keeper, legacyQuerierCdc)
 		case LegacyQueryStorage:
 			return legacyQueryStorage(ctx, strings.Join(path[1:], "/"), req, keeper, legacyQuerierCdc)
 		case LegacyQueryKeys:
@@ -60,9 +70,7 @@ func queryEgress(ctx sdk.Context, bech32 string, req abci.RequestQuery, keeper K
 }
 
 // nolint: unparam
-func queryMailbox(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) (res []byte, err error) {
-	peer := path[0]
-
+func queryMailbox(ctx sdk.Context, peer string, req abci.RequestQuery, keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) (res []byte, err error) {
 	value := keeper.GetMailbox(ctx, peer)
 
 	if value == "" {

diff --git a/golang/cosmos/x/swingset/types/types.go b/golang/cosmos/x/swingset/types/types.go
@@ -45,16 +45,18 @@ type Messages struct {
 	Ack      uint64
 }
 
-func UnmarshalMessagesJSON(jsonString string) (*Messages, error) {
-	// [message[], ack]
-	// message [num, body]
-	packet := make([]interface{}, 2)
-	err := json.Unmarshal([]byte(jsonString), &packet)
+// UnmarshalMessagesJSON decodes Messages from JSON text.
+// Input must represent an array in which the first element is an array of
+// [messageNum: integer, messageBody: string] pairs and the second element is
+// an "Ack" integer.
+func UnmarshalMessagesJSON(jsonString string) (ret *Messages, err error) {
+	packet := [2]interface{}{}
+	err = json.Unmarshal([]byte(jsonString), &packet)
 	if err != nil {
 		return nil, err
 	}
 
-	ret := &Messages{}
+	ret = &Messages{}
 
 	ackFloat, ok := packet[1].(float64)
 	if !ok {
@@ -72,24 +74,25 @@ func UnmarshalMessagesJSON(jsonString string) (*Messages, error) {
 
 	ret.Messages = make([]string, len(msgs))
 	ret.Nums = make([]uint64, len(msgs))
-	for i, nummsgi := range msgs {
-		nummsg, ok := nummsgi.([]interface{})
-		if !ok || len(nummsg) != 2 {
+	for i, rawMsg := range msgs {
+		arrMsg, ok := rawMsg.([]interface{})
+		if !ok || len(arrMsg) != 2 {
 			return nil, errors.New("Message is not a pair")
 		}
-		numFloat, ok := nummsg[0].(float64)
+
+		numFloat, ok := arrMsg[0].(float64)
 		if !ok {
 			return nil, errors.New("Message Num is not an integer")
 		}
 		ret.Nums[i], err = Nat(numFloat)
 		if err != nil {
 			return nil, errors.New("Message num is not a Nat")
 		}
-		msg, ok := nummsg[1].(string)
+
+		ret.Messages[i], ok = arrMsg[1].(string)
 		if !ok {
 			return nil, errors.New("Message is not a string")
 		}
-		ret.Messages[i] = msg
 	}
 
 	return ret, nil

diff --git a/golang/cosmos/x/vbank/keeper/querier.go b/golang/cosmos/x/vbank/keeper/querier.go
@@ -11,7 +11,11 @@ import (
 
 func NewQuerier(k Keeper, legacyQuerierCdc *codec.LegacyAmino) sdk.Querier {
 	return func(ctx sdk.Context, path []string, req abci.RequestQuery) ([]byte, error) {
-		switch path[0] {
+		var queryType string
+		if len(path) > 0 {
+			queryType = path[0]
+		}
+		switch queryType {
 		case types.QueryParams:
 			return queryParams(ctx, path[1:], req, k, legacyQuerierCdc)
 

diff --git a/golang/cosmos/x/vstorage/keeper/querier.go b/golang/cosmos/x/vstorage/keeper/querier.go
@@ -33,7 +33,11 @@ func getVstorageEntryPath(urlPathSegments []string) (string, error) {
 // be used to extend it to a vstorage path such as "foo.bar.baz").
 func NewQuerier(keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) sdk.Querier {
 	return func(ctx sdk.Context, urlPathSegments []string, req abci.RequestQuery) (res []byte, err error) {
-		switch urlPathSegments[0] {
+		var queryType string
+		if len(urlPathSegments) > 0 {
+			queryType = urlPathSegments[0]
+		}
+		switch queryType {
 		case QueryData:
 			entryPath, entryPathErr := getVstorageEntryPath(urlPathSegments[1:])
 			if entryPathErr != nil {