diff --git a/CHANGELOG-3.6.md b/CHANGELOG-3.6.md index 2560e0578..f098fb893 100644 --- a/CHANGELOG-3.6.md +++ b/CHANGELOG-3.6.md @@ -42,6 +42,7 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.5.0...v3.6.0). - Fix [assertion failed due to tx closed when recovering v3 backend from a snapshot db](https://github.com/etcd-io/etcd/pull/13500) - Fix [A client can panic etcd by passing invalid utf-8 in the client-api-version header](https://github.com/etcd-io/etcd/pull/13560) - Fix [etcd gateway doesn't format the endpoint of IPv6 address correctly](https://github.com/etcd-io/etcd/pull/13551) +- Fix [A client can cause a nil dereference in etcd by passing an invalid SortTarget](https://github.com/etcd-io/etcd/pull/13555) ### tools/benchmark diff --git a/api/v3rpc/rpctypes/error.go b/api/v3rpc/rpctypes/error.go index 088c03435..a00832c86 100644 --- a/api/v3rpc/rpctypes/error.go +++ b/api/v3rpc/rpctypes/error.go @@ -28,6 +28,7 @@ var ( ErrGRPCTooManyOps = status.New(codes.InvalidArgument, "etcdserver: too many operations in txn request").Err() ErrGRPCDuplicateKey = status.New(codes.InvalidArgument, "etcdserver: duplicate key given in txn request").Err() ErrGRPCInvalidClientAPIVersion = status.New(codes.InvalidArgument, "etcdserver: invalid client api version").Err() + ErrGRPCInvalidSortOption = status.New(codes.InvalidArgument, "etcdserver: invalid sort option").Err() ErrGRPCCompacted = status.New(codes.OutOfRange, "etcdserver: mvcc: required revision has been compacted").Err() ErrGRPCFutureRev = status.New(codes.OutOfRange, "etcdserver: mvcc: required revision is a future revision").Err() ErrGRPCNoSpace = status.New(codes.ResourceExhausted, "etcdserver: mvcc: database space exceeded").Err() @@ -96,11 +97,12 @@ var ( ErrorDesc(ErrGRPCValueProvided): ErrGRPCValueProvided, ErrorDesc(ErrGRPCLeaseProvided): ErrGRPCLeaseProvided, - ErrorDesc(ErrGRPCTooManyOps): ErrGRPCTooManyOps, - ErrorDesc(ErrGRPCDuplicateKey): ErrGRPCDuplicateKey, - ErrorDesc(ErrGRPCCompacted): ErrGRPCCompacted, - ErrorDesc(ErrGRPCFutureRev): ErrGRPCFutureRev, - ErrorDesc(ErrGRPCNoSpace): ErrGRPCNoSpace, + ErrorDesc(ErrGRPCTooManyOps): ErrGRPCTooManyOps, + ErrorDesc(ErrGRPCDuplicateKey): ErrGRPCDuplicateKey, + ErrorDesc(ErrGRPCInvalidSortOption): ErrGRPCInvalidSortOption, + ErrorDesc(ErrGRPCCompacted): ErrGRPCCompacted, + ErrorDesc(ErrGRPCFutureRev): ErrGRPCFutureRev, + ErrorDesc(ErrGRPCNoSpace): ErrGRPCNoSpace, ErrorDesc(ErrGRPCLeaseNotFound): ErrGRPCLeaseNotFound, ErrorDesc(ErrGRPCLeaseExist): ErrGRPCLeaseExist, @@ -158,15 +160,16 @@ var ( // client-side error var ( - ErrEmptyKey = Error(ErrGRPCEmptyKey) - ErrKeyNotFound = Error(ErrGRPCKeyNotFound) - ErrValueProvided = Error(ErrGRPCValueProvided) - ErrLeaseProvided = Error(ErrGRPCLeaseProvided) - ErrTooManyOps = Error(ErrGRPCTooManyOps) - ErrDuplicateKey = Error(ErrGRPCDuplicateKey) - ErrCompacted = Error(ErrGRPCCompacted) - ErrFutureRev = Error(ErrGRPCFutureRev) - ErrNoSpace = Error(ErrGRPCNoSpace) + ErrEmptyKey = Error(ErrGRPCEmptyKey) + ErrKeyNotFound = Error(ErrGRPCKeyNotFound) + ErrValueProvided = Error(ErrGRPCValueProvided) + ErrLeaseProvided = Error(ErrGRPCLeaseProvided) + ErrTooManyOps = Error(ErrGRPCTooManyOps) + ErrDuplicateKey = Error(ErrGRPCDuplicateKey) + ErrInvalidSortOption = Error(ErrGRPCInvalidSortOption) + ErrCompacted = Error(ErrGRPCCompacted) + ErrFutureRev = Error(ErrGRPCFutureRev) + ErrNoSpace = Error(ErrGRPCNoSpace) ErrLeaseNotFound = Error(ErrGRPCLeaseNotFound) ErrLeaseExist = Error(ErrGRPCLeaseExist) diff --git a/client/v3/kv.go b/client/v3/kv.go index 5e9fb7d45..80cd80af1 100644 --- a/client/v3/kv.go +++ b/client/v3/kv.go @@ -16,6 +16,7 @@ package clientv3 import ( "context" + "go.etcd.io/etcd/api/v3/v3rpc/rpctypes" pb "go.etcd.io/etcd/api/v3/etcdserverpb" @@ -145,10 +146,14 @@ func (kv *kv) Do(ctx context.Context, op Op) (OpResponse, error) { var err error switch op.t { case tRange: - var resp *pb.RangeResponse - resp, err = kv.remote.Range(ctx, op.toRangeRequest(), kv.callOpts...) - if err == nil { - return OpResponse{get: (*GetResponse)(resp)}, nil + if op.IsSortOptionValid() { + var resp *pb.RangeResponse + resp, err = kv.remote.Range(ctx, op.toRangeRequest(), kv.callOpts...) + if err == nil { + return OpResponse{get: (*GetResponse)(resp)}, nil + } + } else { + err = rpctypes.ErrInvalidSortOption } case tPut: var resp *pb.PutResponse diff --git a/client/v3/namespace/kv.go b/client/v3/namespace/kv.go index f745225ca..9a428fa58 100644 --- a/client/v3/namespace/kv.go +++ b/client/v3/namespace/kv.go @@ -51,7 +51,11 @@ func (kv *kvPrefix) Get(ctx context.Context, key string, opts ...clientv3.OpOpti if len(key) == 0 && !(clientv3.IsOptsWithFromKey(opts) || clientv3.IsOptsWithPrefix(opts)) { return nil, rpctypes.ErrEmptyKey } - r, err := kv.KV.Do(ctx, kv.prefixOp(clientv3.OpGet(key, opts...))) + getOp := clientv3.OpGet(key, opts...) + if !getOp.IsSortOptionValid() { + return nil, rpctypes.ErrInvalidSortOption + } + r, err := kv.KV.Do(ctx, kv.prefixOp(getOp)) if err != nil { return nil, err } diff --git a/client/v3/op.go b/client/v3/op.go index e8c0c1e08..6492fbdff 100644 --- a/client/v3/op.go +++ b/client/v3/op.go @@ -581,3 +581,19 @@ func IsOptsWithFromKey(opts []OpOption) bool { return ret.isOptsWithFromKey } + +func (op Op) IsSortOptionValid() bool { + if op.sort != nil { + sortOrder := int32(op.sort.Order) + sortTarget := int32(op.sort.Target) + + if _, ok := pb.RangeRequest_SortOrder_name[sortOrder]; !ok { + return false + } + + if _, ok := pb.RangeRequest_SortTarget_name[sortTarget]; !ok { + return false + } + } + return true +} diff --git a/client/v3/op_test.go b/client/v3/op_test.go index 762044fc5..7cc189c78 100644 --- a/client/v3/op_test.go +++ b/client/v3/op_test.go @@ -36,3 +36,42 @@ func TestOpWithSort(t *testing.T) { t.Fatalf("expected %+v, got %+v", wreq, req) } } + +func TestIsSortOptionValid(t *testing.T) { + rangeReqs := []struct { + sortOrder pb.RangeRequest_SortOrder + sortTarget pb.RangeRequest_SortTarget + expectedValid bool + }{ + { + sortOrder: pb.RangeRequest_ASCEND, + sortTarget: pb.RangeRequest_CREATE, + expectedValid: true, + }, + { + sortOrder: pb.RangeRequest_ASCEND, + sortTarget: 100, + expectedValid: false, + }, + { + sortOrder: 200, + sortTarget: pb.RangeRequest_MOD, + expectedValid: false, + }, + } + + for _, req := range rangeReqs { + getOp := Op{ + sort: &SortOption{ + Order: SortOrder(req.sortOrder), + Target: SortTarget(req.sortTarget), + }, + } + + actualRet := getOp.IsSortOptionValid() + if actualRet != req.expectedValid { + t.Errorf("expected sortOrder (%d) and sortTarget (%d) to be %t, but got %t", + req.sortOrder, req.sortTarget, req.expectedValid, actualRet) + } + } +} diff --git a/server/etcdserver/api/v3rpc/key.go b/server/etcdserver/api/v3rpc/key.go index d1a7ee633..2c1de2a90 100644 --- a/server/etcdserver/api/v3rpc/key.go +++ b/server/etcdserver/api/v3rpc/key.go @@ -115,6 +115,15 @@ func checkRangeRequest(r *pb.RangeRequest) error { if len(r.Key) == 0 { return rpctypes.ErrGRPCEmptyKey } + + if _, ok := pb.RangeRequest_SortOrder_name[int32(r.SortOrder)]; !ok { + return rpctypes.ErrGRPCInvalidSortOption + } + + if _, ok := pb.RangeRequest_SortTarget_name[int32(r.SortTarget)]; !ok { + return rpctypes.ErrGRPCInvalidSortOption + } + return nil } diff --git a/server/etcdserver/api/v3rpc/key_test.go b/server/etcdserver/api/v3rpc/key_test.go new file mode 100644 index 000000000..87e025fe0 --- /dev/null +++ b/server/etcdserver/api/v3rpc/key_test.go @@ -0,0 +1,67 @@ +// Copyright 2021 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package v3rpc + +import ( + pb "go.etcd.io/etcd/api/v3/etcdserverpb" + "go.etcd.io/etcd/api/v3/v3rpc/rpctypes" + "testing" +) + +func TestCheckRangeRequest(t *testing.T) { + rangeReqs := []struct { + sortOrder pb.RangeRequest_SortOrder + sortTarget pb.RangeRequest_SortTarget + expectedError error + }{ + { + sortOrder: pb.RangeRequest_ASCEND, + sortTarget: pb.RangeRequest_CREATE, + expectedError: nil, + }, + { + sortOrder: pb.RangeRequest_ASCEND, + sortTarget: 100, + expectedError: rpctypes.ErrGRPCInvalidSortOption, + }, + { + sortOrder: 200, + sortTarget: pb.RangeRequest_MOD, + expectedError: rpctypes.ErrGRPCInvalidSortOption, + }, + } + + for _, req := range rangeReqs { + rangeReq := pb.RangeRequest{ + Key: []byte{1, 2, 3}, + SortOrder: req.sortOrder, + SortTarget: req.sortTarget, + } + + actualRet := checkRangeRequest(&rangeReq) + if getError(actualRet) != getError(req.expectedError) { + t.Errorf("expected sortOrder (%d) and sortTarget (%d) to be %q, but got %q", + req.sortOrder, req.sortTarget, getError(req.expectedError), getError(actualRet)) + } + } +} + +func getError(err error) string { + if err == nil { + return "" + } + + return err.Error() +} diff --git a/server/etcdserver/apply.go b/server/etcdserver/apply.go index c8ff51674..ca7930696 100644 --- a/server/etcdserver/apply.go +++ b/server/etcdserver/apply.go @@ -337,6 +337,8 @@ func (a *applierV3backend) Range(ctx context.Context, txn mvcc.TxnRead, r *pb.Ra resp := &pb.RangeResponse{} resp.Header = &pb.ResponseHeader{} + lg := a.s.Logger() + if txn == nil { txn = a.s.kv.Read(mvcc.ConcurrentReadTxMode, trace) defer txn.End() @@ -407,6 +409,8 @@ func (a *applierV3backend) Range(ctx context.Context, txn mvcc.TxnRead, r *pb.Ra sorter = &kvSortByMod{&kvSort{rr.KVs}} case r.SortTarget == pb.RangeRequest_VALUE: sorter = &kvSortByValue{&kvSort{rr.KVs}} + default: + lg.Panic("unexpected sort target", zap.Int32("sort-target", int32(r.SortTarget))) } switch { case sortOrder == pb.RangeRequest_ASCEND: