Seeking clarification on vulnerability fixes done for DOS https://github.com/advisories/GHSA-g5ww-5jh7-63cx

[Somak Dutta]

Hello,

Would appreciate any help for the below two question.

I am working on retrofiting/ cherry picking fixes for the below 2 vulnerabilities in version 2.5.0

https://github.com/advisories/GHSA-g5ww-5jh7-63cx

https://github.com/advisories/GHSA-4gg5-vx3j-xwc7

The vulnerability https://github.com/advisories/GHSA-g5ww-5jh7-63cx speaks about 

"in protobuf-java core and lite versions prior to 3.21.7, 3.20.3, 3.19.6 and 3.16.3 can lead to a denial of service attack. Inputs containing multiple instances of non-repeated embedded messages with repeated or unknown fields causes objects to be converted back-n-forth between mutable and immutable forms, resulting in potentially long garbage collection pauses. We recommend updating to the versions mentioned above."

There is a potential fix commit linked to the issue https://github.com/protocolbuffers/protobuf/commit/a3888f53317a8018e7a439bac4abeb8f3425d5e9

Seeking clarification on the fixes done above and how does it tackle the vulnerability in question

1. We can see there are changes in setField and addRepeatedField, where if field value is  instance of MessageLite.Builder, buildPartial() method is invoked prior to setting/adding the field. However given the changes are in MessageReflection, what would be the scenario where lite protocol is parsed via Message Reflection.  As per https://protobuf.dev/reference/java/api-docs/com/google/protobuf/MessageLite.html message lite does not carry features for using descriptors or reflection

2. There are changes done on mergeMessage for message/group type where if getFieldBuilder returns value, the value is just read instead of attempting a merge operation  here https://github.com/protocolbuffers/protobuf/commit/db7c17803320525722f45c1d26fc08bc41d1bf48#diff-eca77d4401894dba76fe6ed92f5ba8e6d0a7b17815b4df0d9ac41e26376223c3R567

However the method getFieldBuilder always returns null, and this check is never invoked.

Regards,

Somak

[Em Rauch]

>  As per https://protobuf.dev/reference/java/api-docs/com/google/protobuf/MessageLite.html message lite does not carry features for using descriptors or reflection

It may be that the behavior you identified is only because Message.Builder extends MessageLite.Builder anyway, and not because you could actually use it with Lite codegen, and that naming MessageLite does not actually give any more or less 'power' / generality in practice.

Note that full-vs-lite codegen is a slightly different topic from full-vs-lite runtime; you can generally use Lite gencode on Full runtime, and even mix Lite and Full codegen as long as you are using the Full runtime,. In general mixing could only work if a Lite gencode message parents a Full one and not the other way around, because we hold the invariant that if you can reflect on a given message you can also reflect on all of its transitive children, which is a property that is maintained when "Lite parents Full" but not when "Full parents Lite".

> 2. There are changes done on mergeMessage for message/group type where if getFieldBuilder returns value, the value is just read instead of attempting a merge operation  here https://github.com/protocolbuffers/protobuf/commit/db7c17803320525722f45c1d26fc08bc41d1bf48#diff-eca77d4401894dba76fe6ed92f5ba8e6d0a7b17815b4df0d9ac41e26376223c3R567

However the method getFieldBuilder always returns null, and this check is never invoked.

It appears that it won't return null in the case of BuilderAdapter has an arbitrary Message.Builder which will define getFieldBuilder() (L400 in your linked commit)

https://github.com/protocolbuffers/protobuf/commit/db7c17803320525722f45c1d26fc08bc41d1bf48#diff-eca77d4401894dba76fe6ed92f5ba8e6d0a7b17815b4df0d9ac41e26376223c3R400

Confidentiality Notice: This email and any attachments are confidential and intended solely for the use of the individual or entity to whom they are addressed. If you have received this email in error, please notify the sender immediately and delete it from your system. Unauthorized use, disclosure, or copying of this email or its contents is strictly prohibited.

--
You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+u...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/protobuf/d9e7fc2e-ca0c-49ec-95c7-a52112dedb74n%40googlegroups.com.

[Somak Dutta]

Thank you Em, appreciate your quick responses always.

Regarding, 

It appears that it won't return null in the case of BuilderAdapter has an arbitrary Message.Builder which will define getFieldBuilder() (L400 in your linked commit)

True, in version 3.16.3 usage of BuilderAdapter does enter the fast path of reading message and return.

protobuf version 2.5.0 has no corresponding BuilderAdapterclass, which would mean the fast path fix from https://github.com/protocolbuffers/protobuf/commit/db7c17803320525722f45c1d26fc08bc41d1bf48#diff-eca77d4401894dba76fe6ed92f5ba8e6d0a7b17815b4df0d9ac41e26376223c3R400 would not actually bring any benefits.

I can create an adapter per my own if i could want, but its not part of the library

I am beginning to wonder if the fix for the vulnerability would actually work if retrofitted in protobuf 2.5 versions.

public static class BuilderAdapter {

private final Message.Builder builder;

public BuilderAdapter(Message.Builder builder) {

this.builder = builder;

}

public void mergeMessage(CodedInputStream input, ExtensionRegistry extensionRegistry,

Descriptors.FieldDescriptor field, UnknownFieldSet.Builder unknownFields) throws IOException {

int tag = WireFormat.makeTag(field.getNumber(), WireFormat.WIRETYPE_LENGTH_DELIMITED);

AbstractMessage.Builder.mergeFieldFrom(

input,

unknownFields != null ? unknownFields : UnknownFieldSet.newBuilder(),

extensionRegistry,

builder.getDescriptorForType(),

builder,

null, // extensions - not needed for regular fields

tag

);

}

}

Regards,

Somak