Weaken vulnerable gencode poison pills to warning by default.

esrauchg · web-flow · commit 320eafa0b7ab · 2025-05-09T16:34:49.000-04:00
Weaken vulnerable gencode poison pills to warning by default. Merge pull request #21688 from esrauchg/25.x
diff --git a/java/core/BUILD.bazel b/java/core/BUILD.bazel
@@ -369,6 +369,7 @@ junit_tests(
             "src/test/java/com/google/protobuf/TestUtil.java",
             "src/test/java/com/google/protobuf/TestUtilLite.java",
             "src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java",
+            "src/test/java/com/google/protobuf/GeneratedMessagePre22ErrorTest.java",
         ],
     ),
     data = ["//src/google/protobuf:testdata"],
@@ -473,6 +474,7 @@ LITE_TEST_EXCLUSIONS = [
     "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/GeneratedMessagePre22ErrorTest.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",
@@ -545,6 +547,24 @@ java_test(
     ],
 )
 
+java_test(
+    name = "GeneratedMessagePre22ErrorTest",
+    size = "small",
+    srcs = [
+        "src/test/java/com/google/protobuf/GeneratedMessagePre22ErrorTest.java",
+    ],
+    jvm_flags = ["-Dcom.google.protobuf.error_on_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
@@ -25,6 +25,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.logging.Logger;
 
 /**
  * All generated protocol message classes extend this class. This class implements most of the
@@ -35,6 +36,7 @@
  */
 public abstract class GeneratedMessage extends AbstractMessage implements Serializable {
   private static final long serialVersionUID = 1L;
+  private static final Logger logger = Logger.getLogger(GeneratedMessage.class.getName());
 
   /**
    * For testing. Allows a test to disable the optimization that avoids using field builders for
@@ -310,22 +312,33 @@ public int getSerializedSize() {
     return memoizedSize;
   }
 
-  static final String PRE22_GENCODE_ACKNOWLEGE_PROPERTY =
+  static final String PRE22_GENCODE_SILENCE_PROPERTY =
       "com.google.protobuf.use_unsafe_pre22_gencode";
+  static final String PRE22_GENCODE_ERROR_PROPERTY =
+      "com.google.protobuf.error_on_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:"
+          + " `-Dcom.google.protobuf.use_unsafe_pre22_gencode` on the command line to silence this"
+          + " warning. You also can set"
+          + " `-Dcom.google.protobuf.error_on_unsafe_pre22_gencode` to throw an error instead. 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);
+    if (System.getProperty(PRE22_GENCODE_SILENCE_PROPERTY) != null) {
+      return;
+    }
+    UnsupportedOperationException exception =
+        new UnsupportedOperationException(PRE22_GENCODE_VULNERABILITY_MESSAGE);
+    if (System.getProperty(PRE22_GENCODE_ERROR_PROPERTY) != null) {
+      throw exception;
     }
+    logger.warning(exception.toString());
   }
 
   /** Used by parsing constructors in generated classes. */
diff --git a/java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22ErrorTest.java b/java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22ErrorTest.java
@@ -0,0 +1,53 @@
+package com.google.protobuf;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
+
+import protobuf_unittest.UnittestProto.TestAllExtensions;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class GeneratedMessagePre22ErrorTest {
+  @Test
+  public void generatedMessage_makeExtensionsImmutableShouldError() {
+    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;
+          }
+        };
+    Throwable e = assertThrows(UnsupportedOperationException.class, () -> msg.makeExtensionsImmutable());
+    assertThat(e).hasMessageThat().contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE);
+  }
+
+  @Test
+  public void extendableMessage_makeExtensionsImmutableShouldError() {
+    GeneratedMessageV3.ExtendableMessage<TestAllExtensions> msg =
+        TestAllExtensions.newBuilder().build();
+    Throwable e = assertThrows(UnsupportedOperationException.class, () -> msg.makeExtensionsImmutable());
+    assertThat(e).hasMessageThat().contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE);
+  }
+}
diff --git a/java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java b/java/core/src/test/java/com/google/protobuf/GeneratedMessagePre22WarningDisabledTest.java
@@ -1,14 +1,27 @@
 package com.google.protobuf;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import protobuf_unittest.UnittestProto.TestAllExtensions;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
 public class GeneratedMessagePre22WarningDisabledTest {
+  private TestUtil.TestLogHandler setupLogger() {
+    TestUtil.TestLogHandler logHandler = new TestUtil.TestLogHandler();
+    Logger logger = Logger.getLogger(GeneratedMessage.class.getName());
+    logger.addHandler(logHandler);
+    logHandler.setLevel(Level.ALL);
+    return logHandler;
+  }
+
   @Test
-  public void generatedMessage_makeExtensionsImmutableShouldNotThrow() {
+  public void generatedMessage_makeExtensionsImmutableShouldNotLog() {
+    TestUtil.TestLogHandler logHandler = setupLogger();
     GeneratedMessageV3 msg =
         new GeneratedMessageV3() {
           @Override
@@ -37,13 +50,16 @@ public Message getDefaultInstanceForType() {
           }
         };
     msg.makeExtensionsImmutable();
+    assertThat(logHandler.getStoredLogRecords()).isEmpty();
   }
 
   @Test
-  public void extendableMessage_makeExtensionsImmutableShouldNotThrow() {
+  public void extendableMessage_makeExtensionsImmutableShouldNotLog() {
+    TestUtil.TestLogHandler logHandler = setupLogger();
     GeneratedMessageV3.ExtendableMessage<TestAllExtensions> msg =
         TestAllExtensions.newBuilder().build();
     msg.makeExtensionsImmutable();
+    assertThat(logHandler.getStoredLogRecords()).isEmpty();
   }
 }
 
diff --git a/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java b/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java
@@ -50,6 +50,9 @@
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
 import org.junit.After;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -1999,9 +2002,19 @@ public void extendableBuilder_mergeFrom_repeatedField_doesNotInvalidateExistingB
     assertThat(builder.getRepeatedField(REPEATED_NESTED_MESSAGE_EXTENSION, 0))
         .isEqualTo(NestedMessage.newBuilder().setBb(100).build());
   }
+
+  private TestUtil.TestLogHandler setupLogger() {
+    TestUtil.TestLogHandler logHandler = new TestUtil.TestLogHandler();
+    Logger logger = Logger.getLogger(GeneratedMessage.class.getName());
+    logger.addHandler(logHandler);
+    logHandler.setLevel(Level.ALL);
+    return logHandler;
+  }
+
   
   @Test
-  public void generatedMessage_makeExtensionsImmutableShouldThrow() {
+  public void generatedMessage_makeExtensionsImmutableShouldLog() {
+    TestUtil.TestLogHandler logHandler = setupLogger();
     GeneratedMessageV3 msg =
         new GeneratedMessageV3() {
           @Override
@@ -2029,27 +2042,24 @@ 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);
-    }
+    msg.makeExtensionsImmutable();
+    List<LogRecord> logs = logHandler.getStoredLogRecords();
+    assertThat(logs).hasSize(1);
+    String message = logs.get(0).getMessage();
+    assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE);
+    assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_SILENCE_PROPERTY);
   }
 
   @Test
   public void extendableMessage_makeExtensionsImmutableShouldThrow() {
+    TestUtil.TestLogHandler logHandler = setupLogger();
     GeneratedMessageV3.ExtendableMessage<TestAllExtensions> 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);
-    }
+    msg.makeExtensionsImmutable();
+    List<LogRecord> logs = logHandler.getStoredLogRecords();
+    assertThat(logs).hasSize(1);
+    String message = logs.get(0).getMessage();
+    assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE);
+    assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_SILENCE_PROPERTY);
   }
 }
