reflect/protodesc: fix panic when working with dynamicpb

Improves on CL 642575 with a few additional checks to make sure a panic doesn't occur even under unexpected conditions. Fixes golang/protobuf#1671 Change-Id: I25bf1cfdf0b35b381cab603f9e06581b3b630d73 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/642975 Reviewed-by: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Stapelberg <[email protected]>
protocolbuffers · Jan 16, 2025 · 2005adb · 2005adb
1 parent aee8a9c
commit 2005adb
Show file tree

Hide file tree

Showing 2 changed files with 106 additions and 24 deletions.
diff --git a/reflect/protodesc/editions.go b/reflect/protodesc/editions.go
@@ -11,6 +11,7 @@ import (
 
 	"google.golang.org/protobuf/internal/editiondefaults"
 	"google.golang.org/protobuf/internal/filedesc"
+	"google.golang.org/protobuf/internal/genid"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/types/descriptorpb"
@@ -128,23 +129,39 @@ func mergeEditionFeatures(parentDesc protoreflect.Descriptor, child *descriptorp
 	// We must not use proto.GetExtension(child, gofeaturespb.E_Go)
 	// because that only works for messages we generated, but not for
 	// dynamicpb messages. See golang/protobuf#1669.
+	//
+	// Further, we harden this code against adversarial inputs: a
+	// service which accepts descriptors from a possibly malicious
+	// source shouldn't crash.
 	goFeatures := child.ProtoReflect().Get(gofeaturespb.E_Go.TypeDescriptor())
 	if !goFeatures.IsValid() {
 		return parentFS
 	}
+	gf, ok := goFeatures.Interface().(protoreflect.Message)
+	if !ok {
+		return parentFS
+	}
 	// gf.Interface() could be *dynamicpb.Message or *gofeaturespb.GoFeatures.
-	gf := goFeatures.Message()
 	fields := gf.Descriptor().Fields()
 
-	if fd := fields.ByName("legacy_unmarshal_json_enum"); gf.Has(fd) {
+	if fd := fields.ByNumber(genid.GoFeatures_LegacyUnmarshalJsonEnum_field_number); fd != nil &&
+		!fd.IsList() &&
+		fd.Kind() == protoreflect.BoolKind &&
+		gf.Has(fd) {
 		parentFS.GenerateLegacyUnmarshalJSON = gf.Get(fd).Bool()
 	}
 
-	if fd := fields.ByName("strip_enum_prefix"); gf.Has(fd) {
+	if fd := fields.ByNumber(genid.GoFeatures_StripEnumPrefix_field_number); fd != nil &&
+		!fd.IsList() &&
+		fd.Kind() == protoreflect.EnumKind &&
+		gf.Has(fd) {
 		parentFS.StripEnumPrefix = int(gf.Get(fd).Enum())
 	}
 
-	if fd := fields.ByName("api_level"); gf.Has(fd) {
+	if fd := fields.ByNumber(genid.GoFeatures_ApiLevel_field_number); fd != nil &&
+		!fd.IsList() &&
+		fd.Kind() == protoreflect.EnumKind &&
+		gf.Has(fd) {
 		parentFS.APILevel = int(gf.Get(fd).Enum())
 	}
 

diff --git a/reflect/protodesc/editions_test.go b/reflect/protodesc/editions_test.go
@@ -7,41 +7,106 @@ package protodesc
 import (
 	"testing"
 
+	"google.golang.org/protobuf/internal/genid"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/reflect/protoreflect"
+	"google.golang.org/protobuf/reflect/protoregistry"
 	"google.golang.org/protobuf/types/descriptorpb"
 	"google.golang.org/protobuf/types/dynamicpb"
 	"google.golang.org/protobuf/types/gofeaturespb"
 )
 
-func TestGoFeaturesDynamic(t *testing.T) {
+func TestGoFeatures_NotExpectedType(t *testing.T) {
 	md := (*gofeaturespb.GoFeatures)(nil).ProtoReflect().Descriptor()
 	gf := dynamicpb.NewMessage(md)
 	opaque := protoreflect.ValueOfEnum(gofeaturespb.GoFeatures_API_OPAQUE.Number())
-	gf.Set(md.Fields().ByName("api_level"), opaque)
-	featureSet := &descriptorpb.FeatureSet{}
+	gf.Set(md.Fields().ByNumber(genid.GoFeatures_ApiLevel_field_number), opaque)
 	dynamicExt := dynamicpb.NewExtensionType(gofeaturespb.E_Go.TypeDescriptor().Descriptor())
-	proto.SetExtension(featureSet, dynamicExt, gf)
 
-	fd := &descriptorpb.FileDescriptorProto{
-		Name: proto.String("a.proto"),
-		Dependency: []string{
-			"google/protobuf/go_features.proto",
-		},
-		Edition: descriptorpb.Edition_EDITION_2023.Enum(),
-		Syntax:  proto.String("editions"),
-		Options: &descriptorpb.FileOptions{
-			Features: featureSet,
+	extFile, err := NewFile(&descriptorpb.FileDescriptorProto{
+		Name:       proto.String("test.proto"),
+		Dependency: []string{"google/protobuf/descriptor.proto"},
+		Extension: []*descriptorpb.FieldDescriptorProto{
+			{
+				Name:     proto.String("ext1002"),
+				Number:   proto.Int32(int32(gofeaturespb.E_Go.TypeDescriptor().Number())),
+				Type:     descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(),
+				Label:    descriptorpb.FieldDescriptorProto_LABEL_OPTIONAL.Enum(),
+				Extendee: proto.String(".google.protobuf.FeatureSet"),
+			},
 		},
+	}, protoregistry.GlobalFiles)
+	if err != nil {
+		t.Fatal(err)
+	}
+	nonMessageExt := dynamicpb.NewExtensionType(extFile.Extensions().Get(0))
+
+	extFdProto := ToFieldDescriptorProto(gofeaturespb.E_Go.TypeDescriptor().Descriptor())
+	extMsgProto := ToDescriptorProto(gofeaturespb.E_Go.TypeDescriptor().Descriptor().Message())
+	extMsgProto.Field = extMsgProto.Field[:1] // remove all but first field from the message
+	extFile, err = NewFile(&descriptorpb.FileDescriptorProto{
+		Name:        proto.String("google/protobuf/go_features.proto"),
+		Package:     proto.String("pb"),
+		Dependency:  []string{"google/protobuf/descriptor.proto"},
+		Extension:   []*descriptorpb.FieldDescriptorProto{extFdProto},
+		MessageType: []*descriptorpb.DescriptorProto{extMsgProto},
+	}, protoregistry.GlobalFiles)
+	if err != nil {
+		t.Fatal(err)
 	}
-	fds := &descriptorpb.FileDescriptorSet{
-		File: []*descriptorpb.FileDescriptorProto{
-			ToFileDescriptorProto(descriptorpb.File_google_protobuf_descriptor_proto),
-			ToFileDescriptorProto(gofeaturespb.File_google_protobuf_go_features_proto),
-			fd,
+	missingFieldsExt := dynamicpb.NewExtensionType(extFile.Extensions().Get(0))
+	gfMissingFields := dynamicpb.NewMessage(extFile.Messages().Get(0))
+	gfPresentField := gfMissingFields.Descriptor().Fields().Get(0)
+	gfMissingFields.Set(gfPresentField, gfMissingFields.NewField(gfPresentField))
+
+	testCases := []struct {
+		name string
+		ext  protoreflect.ExtensionType
+		val  any
+	}{
+		{
+			name: "dynamic_message",
+			ext:  dynamicExt,
+			val:  gf,
+		},
+		{
+			name: "not_a_message",
+			ext:  nonMessageExt,
+			val:  "abc",
+		},
+		{
+			name: "message_missing_fields",
+			ext:  missingFieldsExt,
+			val:  gfMissingFields,
 		},
 	}
-	if _, err := NewFiles(fds); err != nil {
-		t.Fatal(err)
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			featureSet := &descriptorpb.FeatureSet{}
+			proto.SetExtension(featureSet, tc.ext, tc.val)
+
+			fd := &descriptorpb.FileDescriptorProto{
+				Name: proto.String("a.proto"),
+				Dependency: []string{
+					"google/protobuf/go_features.proto",
+				},
+				Edition: descriptorpb.Edition_EDITION_2023.Enum(),
+				Syntax:  proto.String("editions"),
+				Options: &descriptorpb.FileOptions{
+					Features: featureSet,
+				},
+			}
+			fds := &descriptorpb.FileDescriptorSet{
+				File: []*descriptorpb.FileDescriptorProto{
+					ToFileDescriptorProto(descriptorpb.File_google_protobuf_descriptor_proto),
+					ToFileDescriptorProto(gofeaturespb.File_google_protobuf_go_features_proto),
+					fd,
+				},
+			}
+			if _, err := NewFiles(fds); err != nil {
+				t.Fatal(err)
+			}
+		})
 	}
 }