add protection code for Range when the sortTarget is an invalid value

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
 

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)

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

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
 	}

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
+}

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)
+		}
+	}
+}

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
 }
 

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()
+}

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: