From 3f1df78931afb489ef21d9c4b91305b30ef7b14d Mon Sep 17 00:00:00 2001 From: Jerry Berg <107155935+googleberg@users.noreply.github.com> Date: Thu, 23 Jan 2025 16:21:15 +0000 Subject: [PATCH] Add poison pill to makeExtensionsImmutable method (#20084) --- java/core/BUILD.bazel | 21 ++++++++ .../com/google/protobuf/GeneratedMessage.java | 21 +++++++- .../google/protobuf/GeneratedMessageV3.java | 2 + ...eratedMessagePre22WarningDisabledTest.java | 49 +++++++++++++++++ .../google/protobuf/GeneratedMessageTest.java | 53 +++++++++++++++++++ 5 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java diff --git a/java/core/BUILD.bazel b/java/core/BUILD.bazel index b896c426b96cd..32df3e02ebfe0 100644 --- a/java/core/BUILD.bazel +++ b/java/core/BUILD.bazel @@ -368,6 +368,7 @@ junit_tests( "src/test/java/com/google/protobuf/IsValidUtf8Test.java", "src/test/java/com/google/protobuf/TestUtil.java", "src/test/java/com/google/protobuf/TestUtilLite.java", + "src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java", ], ), data = ["//src/google/protobuf:testdata"], @@ -471,6 +472,7 @@ LITE_TEST_EXCLUSIONS = [ "src/test/java/com/google/protobuf/ExtensionRegistryFactoryTest.java", "src/test/java/com/google/protobuf/FieldPresenceTest.java", "src/test/java/com/google/protobuf/ForceFieldBuildersPreRun.java", + "src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java", "src/test/java/com/google/protobuf/GeneratedMessageTest.java", "src/test/java/com/google/protobuf/LazyFieldTest.java", "src/test/java/com/google/protobuf/LazyStringEndToEndTest.java", @@ -524,6 +526,25 @@ junit_tests( ], ) + +java_test( + name = "GeneratedMessagePre22WarningDisabledTest", + size = "small", + srcs = [ + "src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java", + ], + jvm_flags = ["-Dcom.google.protobuf.use_unsafe_pre22_gencode"], + deps = [ + ":core", + ":generic_test_protos_java_proto", + ":java_test_protos_java_proto", + ":lite_test_protos_java_proto", + ":test_util", + "@maven//:com_google_truth_truth", + "@maven//:junit_junit", + ], +) + pkg_files( name = "dist_files", srcs = glob([ diff --git a/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java b/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java index d3ff341dc008d..d3368085f98bb 100644 --- a/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java +++ b/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java @@ -310,9 +310,27 @@ public int getSerializedSize() { return memoizedSize; } + static final String PRE22_GENCODE_ACKNOWLEGE_PROPERTY = + "com.google.protobuf.use_unsafe_pre22_gencode"; + static final String PRE22_GENCODE_VULNERABILITY_MESSAGE = + "As of 2022/09/29 (release 21.7) makeExtensionsImmutable should not be called from protobuf" + + " gencode. If you are seeing this message, your gencode is vulnerable to a denial of" + + " service attack. You should regenerate your code using protobuf 25.6 or later. Use the" + + " latest version that meets your needs. However, if you understand the risks and wish" + + " to continue with vulnerable gencode, you can set the system property" + + " `-Dcom.google.protobuf.use_unsafe_pre22_gencode` on the command line. See security" + + " vulnerability:" + + " https://github.com/protocolbuffers/protobuf/security/advisories/GHSA-h4h5-3hr4-j3g2"; + + static void warnPre22Gencode() { + if (System.getProperty(PRE22_GENCODE_ACKNOWLEGE_PROPERTY) == null) { + throw new UnsupportedOperationException(PRE22_GENCODE_VULNERABILITY_MESSAGE); + } + } + /** Used by parsing constructors in generated classes. */ protected void makeExtensionsImmutable() { - // Noop for messages without extensions. + warnPre22Gencode(); } /** @@ -902,6 +920,7 @@ protected boolean parseUnknownField( /** Used by parsing constructors in generated classes. */ @Override protected void makeExtensionsImmutable() { + warnPre22Gencode(); extensions.makeImmutable(); } diff --git a/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java b/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java index 9c4980fcbe225..cd5f697b0b7a0 100644 --- a/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java +++ b/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java @@ -528,6 +528,7 @@ protected Object newInstance(UnusedPrivateParameter unused) { */ protected void makeExtensionsImmutable() { // Noop for messages without extensions. + GeneratedMessage.warnPre22Gencode(); } /** @@ -1275,6 +1276,7 @@ protected boolean parseUnknownFieldProto3( */ @Override protected void makeExtensionsImmutable() { + GeneratedMessage.warnPre22Gencode(); extensions.makeImmutable(); } diff --git a/java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java b/java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java new file mode 100644 index 0000000000000..c33cdc4db5353 --- /dev/null +++ b/java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java @@ -0,0 +1,49 @@ +package com.google.protobuf; + +import protobuf_unittest.UnittestProto.TestAllExtensions; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class GeneratedMessagePre22WarningDisabledTest { + @Test + public void generatedMessage_makeExtensionsImmutableShouldNotThrow() { + GeneratedMessageV3 msg = + new GeneratedMessageV3() { + @Override + protected FieldAccessorTable internalGetFieldAccessorTable() { + return null; + } + + @Override + protected Message.Builder newBuilderForType(BuilderParent parent) { + return null; + } + + @Override + public Message.Builder newBuilderForType() { + return null; + } + + @Override + public Message.Builder toBuilder() { + return null; + } + + @Override + public Message getDefaultInstanceForType() { + return null; + } + }; + msg.makeExtensionsImmutable(); + } + + @Test + public void extendableMessage_makeExtensionsImmutableShouldNotThrow() { + GeneratedMessageV3.ExtendableMessage msg = + TestAllExtensions.newBuilder().build(); + msg.makeExtensionsImmutable(); + } +} + diff --git a/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java b/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java index dbe3072fb7de3..6a2020a92a595 100644 --- a/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java +++ b/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java @@ -1999,4 +1999,57 @@ public void extendableBuilder_mergeFrom_repeatedField_doesNotInvalidateExistingB assertThat(builder.getRepeatedField(REPEATED_NESTED_MESSAGE_EXTENSION, 0)) .isEqualTo(NestedMessage.newBuilder().setBb(100).build()); } + + @Test + public void generatedMessage_makeExtensionsImmutableShouldThrow() { + GeneratedMessageV3 msg = + new GeneratedMessageV3() { + @Override + protected FieldAccessorTable internalGetFieldAccessorTable() { + return null; + } + + @Override + protected Message.Builder newBuilderForType(BuilderParent parent) { + return null; + } + + @Override + public Message.Builder newBuilderForType() { + return null; + } + + @Override + public Message.Builder toBuilder() { + return null; + } + + @Override + public Message getDefaultInstanceForType() { + return null; + } + }; + try { + msg.makeExtensionsImmutable(); + assertWithMessage("Expected UnsupportedOperationException").fail(); + } catch (UnsupportedOperationException e) { + // Expected + assertThat(e).hasMessageThat().contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE); + assertThat(e).hasMessageThat().contains(GeneratedMessage.PRE22_GENCODE_ACKNOWLEGE_PROPERTY); + } + } + + @Test + public void extendableMessage_makeExtensionsImmutableShouldThrow() { + GeneratedMessageV3.ExtendableMessage msg = + TestAllExtensions.getDefaultInstance(); + try { + msg.makeExtensionsImmutable(); + assertWithMessage("Expected UnsupportedOperationException").fail(); + } catch (UnsupportedOperationException e) { + // Expected + assertThat(e).hasMessageThat().contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE); + assertThat(e).hasMessageThat().contains(GeneratedMessage.PRE22_GENCODE_ACKNOWLEGE_PROPERTY); + } + } }