Hello, *Question* : I am unsure how the fix commit provided fixes the vulnerability in question. Details below, if some one can please explain the actual fix for the vuln, would be great.
Reaching out to talk about the potential fix done for vulnerability https://github.com/advisories/GHSA-4gg5-vx3j-xwc7 or https://github.com/advisories/GHSA-g5ww-5jh7-63cx in version 3.16.3 , as part of the fix commit https://github.com/protocolbuffers/protobuf/commit/db7c17803320525722f45c1d26fc08bc41d1bf48 The vulnerability speaks about - " *A parsing issue similar to CVE-2022-3171 <https://github.com/advisories/GHSA-h4h5-3hr4-j3g2>, but with Message-Type Extensions 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.* " It speaks mostly about, how if fields are marked as immutable, messages would need to be cloned, leading to more object creation and thereby long garbage pauses. We can see the impact of it here <https://github.com/protocolbuffers/protobuf/blob/v3.16.3/java/core/src/main/java/com/google/protobuf/DynamicMessage.java#L659> As per the message in the fix commit , " Change the Lite runtime to prefer merging from the wireformat into mutable messages *rather than building *up a new immutable object before merging. This way results in fewer allocations and copy operations." *Doubt:* I tried generating a message structure that can potentially reproduce the vulnerability, however based on my debugging, fields are always mutable in fixed version 3.16.3 , as well as lower versions. The problem i see is, the fields are always remaining mutable, meaning there is no flipping between mutable and immutable forms as mentioned in the vulnerability. I am not sure if its my test thats flawed, hence reaching out for help. Attaching test case, and test data structure. Thank you and Regards, Somak -- **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 [email protected]. To view this discussion visit https://groups.google.com/d/msgid/protobuf/74d07507-8712-42d0-ac4f-2756bf5f2ab9n%40googlegroups.com.
google_person.proto
Description: Binary data
package com.google.protobuf;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
public class NewSizeTest {
private static void writeTagAndLengthDelimited(ByteArrayOutputStream out, int fieldNumber, String value) throws Exception {
int tag = (fieldNumber << 3) | 2; // wire type 2 = length-delimited
writeVarint(out, tag);
byte[] data = value.getBytes("UTF-8");
writeVarint(out, data.length);
out.write(data);
}
private static void writeVarint(ByteArrayOutputStream out, int value) {
while ((value & 0xFFFFFF80) != 0) {
out.write((value & 0x7F) | 0x80);
value >>>= 7;
}
out.write(value);
}
public static byte[][] getNameTags() throws Exception {
ByteArrayOutputStream nameBytes1 = new ByteArrayOutputStream();
writeTagAndLengthDelimited(nameBytes1, 2, "Alice"); // first_name = 2
byte[] nameMsg1 = nameBytes1.toByteArray();
int count = 2;
byte[][] data = new byte[count][];
for(int i =0; i< count;i++){
ByteArrayOutputStream nameBytes2 = new ByteArrayOutputStream();
writeTagAndLengthDelimited(nameBytes2, 3, "Smith"); // last_name = 3
writeTagAndLengthDelimited(nameBytes2, 200, "suspicious_tag_1"); // unknown
byte[] nameMsg2 = nameBytes2.toByteArray();
data[i] = nameMsg2;
}
return data;
}
public static void main(String[] args) throws Exception{
System.out.println("Starting test...");
ExtensionRegistry registry = ExtensionRegistry.newInstance();
GooglePersonProto.registerAllExtensions(registry);
Descriptors.Descriptor personDesc = GooglePersonProto.Person.getDescriptor();
byte[][] data = getNameTags();
ByteArrayOutputStream personBytes = new ByteArrayOutputStream();
// i am adding repeated fields as per vuln record available in https://github.com/advisories/GHSA-4gg5-vx3j-xwc7 and https://github.com/advisories/GHSA-g5ww-5jh7-63cx
for (byte[] nameMsg : data) {
writeVarint(personBytes, (1 << 3) | 2); // tag = 1, wire type = 2
writeVarint(personBytes, nameMsg.length);
personBytes.write(nameMsg);
}
// i am adding extension fields as per vuln record available in https://github.com/advisories/GHSA-4gg5-vx3j-xwc7
writeTagAndLengthDelimited(personBytes, 101, "extension_data");
writeTagAndLengthDelimited(personBytes, 102, "ext1");
writeTagAndLengthDelimited(personBytes, 102, "ext2");
byte[] finalPayload = personBytes.toByteArray();
System.out.println("before parsing.........................................................");
DynamicMessage msg = DynamicMessage.parseFrom(personDesc, finalPayload, registry);
}
}
