From 9f17bc77aff66286286b992c87fdf5be8ae8dc21 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Tue, 3 Nov 2020 10:35:22 -0800 Subject: [PATCH] baseapp, client: reject gRPC connections with out-of-range/nefarious x-cosmos-block-height values (#7663) * baseapp, client: reject gRPC connections with out-of-range/nefarious x-cosmos-block-height values Rejects gRPC connections that send out-of-range x-cosmos-block-height values that previously weren't checked for. We now reject any negative values and any value greater than max(int64) aka >9223372036854775807. Also added an enforcement for returning an error if any negative heights are passed into (*BaseApp).createQueryContext. Fixes #7662 * baseapp, client: reject gRPC connections with out-of-range/nefarious x-cosmos-block-height values Rejects gRPC connections that send out-of-range x-cosmos-block-height values that previously weren't checked for. We now reject any negative values and any value greater than max(int64) aka >9223372036854775807. Also added an enforcement for returning an error if any negative heights are passed into (*BaseApp).createQueryContext. Fixes #7662 * Address Robert's feedback to extract negative height checker * Fix tests Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- baseapp/abci.go | 15 +++++++++++++++ baseapp/abci_test.go | 23 +++++++++++++++++++++++ baseapp/grpcserver.go | 6 ++++++ client/grpc_query.go | 6 ++++++ server/grpc/server_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 88 insertions(+) diff --git a/baseapp/abci.go b/baseapp/abci.go index 5b16156b72..a8f442b8d3 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -537,9 +537,24 @@ func gRPCErrorToSDKError(err error) error { } } +func checkNegativeHeight(height int64) error { + if height < 0 { + // Reject invalid heights. + return sdkerrors.Wrap( + sdkerrors.ErrInvalidRequest, + "cannot query with height < 0; please provide a valid height", + ) + } + return nil +} + // createQueryContext creates a new sdk.Context for a query, taking as args // the block height and whether the query needs a proof or not. func (app *BaseApp) createQueryContext(height int64, prove bool) (sdk.Context, error) { + if err := checkNegativeHeight(height); err != nil { + return sdk.Context{}, err + } + // when a client did not provide a query height, manually inject the latest if height == 0 { height = app.LastBlockHeight() diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 33735140e2..8a61a0aebf 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1,6 +1,7 @@ package baseapp import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -116,3 +117,25 @@ func TestGetBlockRentionHeight(t *testing.T) { }) } } + +// Test and ensure that negative heights always cause errors. +// See issue https://github.com/cosmos/cosmos-sdk/issues/7662. +func TestBaseAppCreateQueryContextRejectsNegativeHeights(t *testing.T) { + t.Parallel() + + logger := defaultLogger() + db := dbm.NewMemDB() + name := t.Name() + app := NewBaseApp(name, logger, db, nil) + + proves := []bool{ + false, true, + } + for _, prove := range proves { + t.Run(fmt.Sprintf("prove=%t", prove), func(t *testing.T) { + sctx, err := app.createQueryContext(-10, true) + require.Error(t, err) + require.Equal(t, sctx, sdk.Context{}) + }) + } +} diff --git a/baseapp/grpcserver.go b/baseapp/grpcserver.go index ab27685b16..74e8b8a21c 100644 --- a/baseapp/grpcserver.go +++ b/baseapp/grpcserver.go @@ -11,6 +11,7 @@ import ( "google.golang.org/grpc/status" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" grpctypes "github.com/cosmos/cosmos-sdk/types/grpc" ) @@ -33,6 +34,11 @@ func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) { if heightHeaders := md.Get(grpctypes.GRPCBlockHeightHeader); len(heightHeaders) > 0 { height, err = strconv.ParseInt(heightHeaders[0], 10, 64) if err != nil { + return nil, sdkerrors.Wrapf( + sdkerrors.ErrInvalidRequest, + "Baseapp.RegisterGRPCServer: invalid height header %q: %v", grpctypes.GRPCBlockHeightHeader, err) + } + if err := checkNegativeHeight(height); err != nil { return nil, err } } diff --git a/client/grpc_query.go b/client/grpc_query.go index 7f1fe6a394..979333f641 100644 --- a/client/grpc_query.go +++ b/client/grpc_query.go @@ -15,6 +15,7 @@ import ( grpctypes "github.com/cosmos/cosmos-sdk/types/grpc" "github.com/cosmos/cosmos-sdk/codec/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) var _ gogogrpc.ClientConn = Context{} @@ -35,6 +36,11 @@ func (ctx Context) Invoke(grpcCtx gocontext.Context, method string, args, reply if err != nil { return err } + if height < 0 { + return sdkerrors.Wrapf( + sdkerrors.ErrInvalidRequest, + "client.Context.Invoke: height (%d) from %q must be >= 0", height, grpctypes.GRPCBlockHeightHeader) + } ctx = ctx.WithHeight(height) } diff --git a/server/grpc/server_test.go b/server/grpc/server_test.go index f5db3c7fc2..bfa39d8185 100644 --- a/server/grpc/server_test.go +++ b/server/grpc/server_test.go @@ -7,6 +7,7 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "google.golang.org/grpc" "google.golang.org/grpc/metadata" @@ -48,6 +49,7 @@ func (s *IntegrationTestSuite) TestGRPCServer() { grpc.WithInsecure(), // Or else we get "no transport security set" ) s.Require().NoError(err) + defer conn.Close() // gRPC query to test service should work testClient := testdata.NewQueryClient(conn) @@ -99,6 +101,42 @@ func (s *IntegrationTestSuite) TestGRPCServer() { s.Require().True(servicesMap["cosmos.bank.v1beta1.Query"]) } +// Test and enforce that we upfront reject any connections to baseapp containing +// invalid initial x-cosmos-block-height that aren't positive and in the range [0, max(int64)] +// See issue https://github.com/cosmos/cosmos-sdk/issues/7662. +func (s *IntegrationTestSuite) TestGRPCServerInvalidHeaderHeights() { + t := s.T() + val0 := s.network.Validators[0] + + // We should reject connections with invalid block heights off the bat. + invalidHeightStrs := []struct { + value string + wantErr string + }{ + {"-1", "height < 0"}, + {"9223372036854775808", "value out of range"}, // > max(int64) by 1 + {"-10", "height < 0"}, + {"18446744073709551615", "value out of range"}, // max uint64, which is > max(int64) + {"-9223372036854775809", "value out of range"}, // Out of the range of for negative int64 + } + for _, tt := range invalidHeightStrs { + t.Run(tt.value, func(t *testing.T) { + conn, err := grpc.Dial( + val0.AppConfig.GRPC.Address, + grpc.WithInsecure(), // Or else we get "no transport security set" + ) + defer conn.Close() + + testClient := testdata.NewQueryClient(conn) + ctx := metadata.AppendToOutgoingContext(context.Background(), grpctypes.GRPCBlockHeightHeader, tt.value) + testRes, err := testClient.Echo(ctx, &testdata.EchoRequest{Message: "hello"}) + require.Error(t, err) + require.Nil(t, testRes) + require.Contains(t, err.Error(), tt.wantErr) + }) + } +} + func TestIntegrationTestSuite(t *testing.T) { suite.Run(t, new(IntegrationTestSuite)) }