From bb27d7c429afb655ee8b7449bca17458500d6a82 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Mon, 9 Dec 2024 11:49:22 +0100 Subject: [PATCH 1/6] Limit recursion depth for unknown field detection --- x/tx/decode/unknown.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/x/tx/decode/unknown.go b/x/tx/decode/unknown.go index cd0ed0ba7aa1..907f2d58c531 100644 --- a/x/tx/decode/unknown.go +++ b/x/tx/decode/unknown.go @@ -33,9 +33,22 @@ func RejectUnknownFieldsStrict(bz []byte, msg protoreflect.MessageDescriptor, re // This function traverses inside of messages nested via google.protobuf.Any. It does not do any deserialization of the proto.Message. // An AnyResolver must be provided for traversing inside google.protobuf.Any's. func RejectUnknownFields(bz []byte, desc protoreflect.MessageDescriptor, allowUnknownNonCriticals bool, resolver protodesc.Resolver) (hasUnknownNonCriticals bool, err error) { + return doRejectUnknownFields(bz, desc, allowUnknownNonCriticals, resolver, 1_000) +} + +func doRejectUnknownFields( + bz []byte, + desc protoreflect.MessageDescriptor, + allowUnknownNonCriticals bool, + resolver protodesc.Resolver, + recursionLimit int, +) (hasUnknownNonCriticals bool, err error) { if len(bz) == 0 { return hasUnknownNonCriticals, nil } + if recursionLimit == 0 { + return false, errors.New("recursion limit reached") + } fields := desc.Fields() @@ -111,7 +124,7 @@ func RejectUnknownFields(bz []byte, desc protoreflect.MessageDescriptor, allowUn if fieldMessage.FullName() == anyFullName { // Firstly typecheck types.Any to ensure nothing snuck in. - hasUnknownNonCriticalsChild, err := RejectUnknownFields(fieldBytes, anyDesc, allowUnknownNonCriticals, resolver) + hasUnknownNonCriticalsChild, err := doRejectUnknownFields(fieldBytes, anyDesc, allowUnknownNonCriticals, resolver, recursionLimit-1) hasUnknownNonCriticals = hasUnknownNonCriticals || hasUnknownNonCriticalsChild if err != nil { return hasUnknownNonCriticals, err @@ -131,7 +144,7 @@ func RejectUnknownFields(bz []byte, desc protoreflect.MessageDescriptor, allowUn fieldBytes = a.Value } - hasUnknownNonCriticalsChild, err := RejectUnknownFields(fieldBytes, fieldMessage, allowUnknownNonCriticals, resolver) + hasUnknownNonCriticalsChild, err := doRejectUnknownFields(fieldBytes, fieldMessage, allowUnknownNonCriticals, resolver, recursionLimit-1) hasUnknownNonCriticals = hasUnknownNonCriticals || hasUnknownNonCriticalsChild if err != nil { return hasUnknownNonCriticals, err From 60c69bbbd63ec89b3e0ea17622d9eb75bf612297 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Tue, 10 Dec 2024 10:58:37 +0100 Subject: [PATCH 2/6] Limit unpack any --- codec/types/interface_registry.go | 56 +++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/codec/types/interface_registry.go b/codec/types/interface_registry.go index 04c94c4394a9..34d59bd33a46 100644 --- a/codec/types/interface_registry.go +++ b/codec/types/interface_registry.go @@ -15,6 +15,17 @@ import ( "cosmossdk.io/x/tx/signing" ) +var ( + + // MaxUnpackAnySubCalls extension point that defines the maximum number of sub-calls allowed during the unpacking + // process of protobuf Any messages. + MaxUnpackAnySubCalls = 100 + + // MaxUnpackAnyRecursionDepth extension point that defines the maximum allowed recursion depth during protobuf Any + // message unpacking. + MaxUnpackAnyRecursionDepth = 10 +) + // UnpackInterfaces is a convenience function that calls UnpackInterfaces // on x if x implements UnpackInterfacesMessage func UnpackInterfaces(x interface{}, unpacker gogoprotoany.AnyUnpacker) error { @@ -230,6 +241,45 @@ func (registry *interfaceRegistry) ListImplementations(ifaceName string) []strin } func (registry *interfaceRegistry) UnpackAny(any *Any, iface interface{}) error { + unpacker := &statefulUnpacker{ + registry: registry, + maxDepth: MaxUnpackAnyRecursionDepth, + maxCalls: &sharedCounter{count: MaxUnpackAnySubCalls}, + } + return unpacker.UnpackAny(any, iface) +} + +// sharedCounter is a type that encapsulates a counter value +type sharedCounter struct { + count int +} + +// statefulUnpacker is a struct that helps in deserializing and unpacking +// protobuf Any messages while maintaining certain stateful constraints. +type statefulUnpacker struct { + registry *interfaceRegistry + maxDepth int + maxCalls *sharedCounter +} + +// cloneForRecursion returns a new statefulUnpacker instance with maxDepth reduced by one, preserving the registry and maxCalls. +func (r statefulUnpacker) cloneForRecursion() *statefulUnpacker { + return &statefulUnpacker{ + registry: r.registry, + maxDepth: r.maxDepth - 1, + maxCalls: r.maxCalls, + } +} + +// UnpackAny deserializes a protobuf Any message into the provided interface, ensuring the interface is a pointer. +// It applies stateful constraints such as max depth and call limits, and unpacks interfaces if required. +func (r *statefulUnpacker) UnpackAny(any *Any, iface interface{}) error { + if r.maxDepth == 0 { + return errors.New("max depth exceeded") + } + if r.maxCalls.count == 0 { + return errors.New("call limit exceeded") + } // here we gracefully handle the case in which `any` itself is `nil`, which may occur in message decoding if any == nil { return nil @@ -240,6 +290,8 @@ func (registry *interfaceRegistry) UnpackAny(any *Any, iface interface{}) error return nil } + r.maxCalls.count-- + rv := reflect.ValueOf(iface) if rv.Kind() != reflect.Ptr { return errors.New("UnpackAny expects a pointer") @@ -255,7 +307,7 @@ func (registry *interfaceRegistry) UnpackAny(any *Any, iface interface{}) error } } - imap, found := registry.interfaceImpls[rt] + imap, found := r.registry.interfaceImpls[rt] if !found { return fmt.Errorf("no registered implementations of type %+v", rt) } @@ -277,7 +329,7 @@ func (registry *interfaceRegistry) UnpackAny(any *Any, iface interface{}) error return err } - err = UnpackInterfaces(msg, registry) + err = UnpackInterfaces(msg, r.cloneForRecursion()) if err != nil { return err } From 0f441f8d9fc9b00762da26f6e75333fc2d91e8a5 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Fri, 13 Dec 2024 11:02:06 +0100 Subject: [PATCH 3/6] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09bef4b4a38e..de3104378c6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,7 +55,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i * [#22826](https://github.com/cosmos/cosmos-sdk/pull/22826) Simplify testing frameworks by removing `testutil/cmdtest`. ### Bug Fixes - +* Fix [ABS-0043/ ABS-0044](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm) Limit recursion depth for unknown field detection and unpack any * (sims) [#21906](https://github.com/cosmos/cosmos-sdk/pull/21906) Skip sims test when running dry on validators * (cli) [#21919](https://github.com/cosmos/cosmos-sdk/pull/21919) Query address-by-acc-num by account_id instead of id. * (cli) [#22656](https://github.com/cosmos/cosmos-sdk/pull/22656) Prune cmd should disable async pruning. From 26bbd4bf707866346a9055b1e3d92d66e9765434 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Fri, 13 Dec 2024 12:43:37 +0100 Subject: [PATCH 4/6] Another limit recursion depth for unknown field detection --- codec/unknownproto/unknown_fields.go | 18 ++++++++++++++++-- x/tx/decode/unknown.go | 3 ++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/codec/unknownproto/unknown_fields.go b/codec/unknownproto/unknown_fields.go index 07759456e296..17b8f7e424ee 100644 --- a/codec/unknownproto/unknown_fields.go +++ b/codec/unknownproto/unknown_fields.go @@ -40,9 +40,23 @@ func RejectUnknownFieldsStrict(bz []byte, msg proto.Message, resolver jsonpb.Any // This function traverses inside of messages nested via google.protobuf.Any. It does not do any deserialization of the proto.Message. // An AnyResolver must be provided for traversing inside google.protobuf.Any's. func RejectUnknownFields(bz []byte, msg proto.Message, allowUnknownNonCriticals bool, resolver jsonpb.AnyResolver) (hasUnknownNonCriticals bool, err error) { + // recursion limit with same default as https://github.com/protocolbuffers/protobuf-go/blob/v1.35.2/encoding/protowire/wire.go#L28 + return doRejectUnknownFields(bz, msg, allowUnknownNonCriticals, resolver, 10_000) +} + +func doRejectUnknownFields( + bz []byte, + msg proto.Message, + allowUnknownNonCriticals bool, + resolver jsonpb.AnyResolver, + recursionLimit int, +) (hasUnknownNonCriticals bool, err error) { if len(bz) == 0 { return hasUnknownNonCriticals, nil } + if recursionLimit == 0 { + return false, errors.New("recursion limit reached") + } fieldDescProtoFromTagNum, _, err := getDescriptorInfo(msg) if err != nil { @@ -125,7 +139,7 @@ func RejectUnknownFields(bz []byte, msg proto.Message, allowUnknownNonCriticals if protoMessageName == ".google.protobuf.Any" { // Firstly typecheck types.Any to ensure nothing snuck in. - hasUnknownNonCriticalsChild, err := RejectUnknownFields(fieldBytes, (*types.Any)(nil), allowUnknownNonCriticals, resolver) + hasUnknownNonCriticalsChild, err := doRejectUnknownFields(fieldBytes, (*types.Any)(nil), allowUnknownNonCriticals, resolver, recursionLimit-1) hasUnknownNonCriticals = hasUnknownNonCriticals || hasUnknownNonCriticalsChild if err != nil { return hasUnknownNonCriticals, err @@ -148,7 +162,7 @@ func RejectUnknownFields(bz []byte, msg proto.Message, allowUnknownNonCriticals } } - hasUnknownNonCriticalsChild, err := RejectUnknownFields(fieldBytes, msg, allowUnknownNonCriticals, resolver) + hasUnknownNonCriticalsChild, err := doRejectUnknownFields(fieldBytes, msg, allowUnknownNonCriticals, resolver, recursionLimit-1) hasUnknownNonCriticals = hasUnknownNonCriticals || hasUnknownNonCriticalsChild if err != nil { return hasUnknownNonCriticals, err diff --git a/x/tx/decode/unknown.go b/x/tx/decode/unknown.go index 907f2d58c531..3a30ef898776 100644 --- a/x/tx/decode/unknown.go +++ b/x/tx/decode/unknown.go @@ -33,7 +33,8 @@ func RejectUnknownFieldsStrict(bz []byte, msg protoreflect.MessageDescriptor, re // This function traverses inside of messages nested via google.protobuf.Any. It does not do any deserialization of the proto.Message. // An AnyResolver must be provided for traversing inside google.protobuf.Any's. func RejectUnknownFields(bz []byte, desc protoreflect.MessageDescriptor, allowUnknownNonCriticals bool, resolver protodesc.Resolver) (hasUnknownNonCriticals bool, err error) { - return doRejectUnknownFields(bz, desc, allowUnknownNonCriticals, resolver, 1_000) + // recursion limit with same default as https://github.com/protocolbuffers/protobuf-go/blob/v1.35.2/encoding/protowire/wire.go#L28 + return doRejectUnknownFields(bz, desc, allowUnknownNonCriticals, resolver, 10_000) } func doRejectUnknownFields( From fc8d8e1a46b5475a09a98a01b6d72c55c936f75f Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Fri, 13 Dec 2024 12:50:52 +0100 Subject: [PATCH 5/6] Update changelog --- x/tx/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index edec82e4efcf..41accb5d2e11 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -33,10 +33,11 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos- ## [Unreleased] -## [v1.0.0-alpha.3](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v1.0.0-alpha.3) - 2024-12-12 +## [v1.0.0-alpha.3](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v1.0.0-alpha.3) - 2024-12-16 ### Bug Fixes +* Fix [ABS-0043](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm) Limit recursion depth for unknown field detection * [#22852](https://github.com/cosmos/cosmos-sdk/pull/22852) Fallback to injected resolver for placeholder descriptors. ## [v1.0.0-alpha.2](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v1.0.0-alpha.2) - 2024-11-01 From 0cc4eb35562c2ef37301f851a9fb22e9e9c89b13 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Mon, 16 Dec 2024 17:45:01 +0100 Subject: [PATCH 6/6] Revert changelog --- CHANGELOG.md | 1 - x/tx/CHANGELOG.md | 1 - 2 files changed, 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de3104378c6d..22f663ee1d68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,7 +55,6 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i * [#22826](https://github.com/cosmos/cosmos-sdk/pull/22826) Simplify testing frameworks by removing `testutil/cmdtest`. ### Bug Fixes -* Fix [ABS-0043/ ABS-0044](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm) Limit recursion depth for unknown field detection and unpack any * (sims) [#21906](https://github.com/cosmos/cosmos-sdk/pull/21906) Skip sims test when running dry on validators * (cli) [#21919](https://github.com/cosmos/cosmos-sdk/pull/21919) Query address-by-acc-num by account_id instead of id. * (cli) [#22656](https://github.com/cosmos/cosmos-sdk/pull/22656) Prune cmd should disable async pruning. diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index 41accb5d2e11..958cd9ac19b4 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -37,7 +37,6 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos- ### Bug Fixes -* Fix [ABS-0043](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm) Limit recursion depth for unknown field detection * [#22852](https://github.com/cosmos/cosmos-sdk/pull/22852) Fallback to injected resolver for placeholder descriptors. ## [v1.0.0-alpha.2](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v1.0.0-alpha.2) - 2024-11-01