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 fd19f2d774a63..fb7622dc34f77 100644 --- a/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java +++ b/java/core/src/main/java/com/google/protobuf/GeneratedMessage.java @@ -21,9 +21,11 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.TreeMap; import java.util.logging.Logger; @@ -329,21 +331,30 @@ public int getSerializedSize() { + " security vulnerability:" + " https://github.com/protocolbuffers/protobuf/security/advisories/GHSA-h4h5-3hr4-j3g2"; - static void warnPre22Gencode() { + private static final Set loggedPre22TypeNames + = Collections.synchronizedSet(new HashSet()); + static void warnPre22Gencode(Class messageClass) { if (System.getProperty(PRE22_GENCODE_SILENCE_PROPERTY) != null) { return; } - UnsupportedOperationException exception = - new UnsupportedOperationException(PRE22_GENCODE_VULNERABILITY_MESSAGE); + String messageName = messageClass.getName(); + String vulnerabilityMessage = + "Vulnerable protobuf generated type in use: " + messageName + "\n" + + PRE22_GENCODE_VULNERABILITY_MESSAGE; + if (System.getProperty(PRE22_GENCODE_ERROR_PROPERTY) != null) { - throw exception; + throw new UnsupportedOperationException(vulnerabilityMessage); + } + + if (!loggedPre22TypeNames.add(messageName)) { + return; } - logger.warning(exception.toString()); + logger.warning(vulnerabilityMessage); } /** Used by parsing constructors in generated classes. */ protected void makeExtensionsImmutable() { - warnPre22Gencode(); + warnPre22Gencode(getClass()); } /** @@ -933,7 +944,7 @@ protected boolean parseUnknownField( /** Used by parsing constructors in generated classes. */ @Override protected void makeExtensionsImmutable() { - warnPre22Gencode(); + warnPre22Gencode(getClass()); 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 cd5f697b0b7a0..e73641cc0c428 100644 --- a/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java +++ b/java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java @@ -528,7 +528,7 @@ protected Object newInstance(UnusedPrivateParameter unused) { */ protected void makeExtensionsImmutable() { // Noop for messages without extensions. - GeneratedMessage.warnPre22Gencode(); + GeneratedMessage.warnPre22Gencode(getClass()); } /** @@ -1276,7 +1276,7 @@ protected boolean parseUnknownFieldProto3( */ @Override protected void makeExtensionsImmutable() { - GeneratedMessage.warnPre22Gencode(); + GeneratedMessage.warnPre22Gencode(getClass()); extensions.makeImmutable(); } 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 a4ae136c1c36d..8e6694ad32d0b 100644 --- a/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java +++ b/java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java @@ -2015,39 +2015,58 @@ private TestUtil.TestLogHandler setupLogger() { @Test public void generatedMessage_makeExtensionsImmutableShouldLog() { TestUtil.TestLogHandler logHandler = setupLogger(); - GeneratedMessageV3 msg = - new GeneratedMessageV3() { - @Override - protected FieldAccessorTable internalGetFieldAccessorTable() { - return null; - } - - @Override - protected Message.Builder newBuilderForType(BuilderParent parent) { - return null; - } + class TestMessage1 extends 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; + } + } - @Override - public Message.Builder newBuilderForType() { - return null; - } + class TestMessage2 extends TestMessage1 {} - @Override - public Message.Builder toBuilder() { - return null; - } - - @Override - public Message getDefaultInstanceForType() { - return null; - } - }; + TestMessage1 msg = new TestMessage1(); msg.makeExtensionsImmutable(); List logs = logHandler.getStoredLogRecords(); assertThat(logs).hasSize(1); String message = logs.get(0).getMessage(); + // The generated type + assertThat(message).contains( + "Vulnerable protobuf generated type in use: " + + "com.google.protobuf.GeneratedMessageTest$1TestMessage1"); assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE); assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_SILENCE_PROPERTY); + + // Subsequent calls for the same type do not log again. + msg.makeExtensionsImmutable(); + assertThat(logHandler.getStoredLogRecords()).hasSize(1); + + // A call on a second type does log for that type. + TestMessage2 msg2 = new TestMessage2(); + msg2.makeExtensionsImmutable(); + assertThat(logHandler.getStoredLogRecords()).hasSize(2); + // And not again (only once per type). + msg2.makeExtensionsImmutable(); + assertThat(logHandler.getStoredLogRecords()).hasSize(2); } @Test @@ -2059,7 +2078,14 @@ public void extendableMessage_makeExtensionsImmutableShouldThrow() { List logs = logHandler.getStoredLogRecords(); assertThat(logs).hasSize(1); String message = logs.get(0).getMessage(); + assertThat(message).contains( + "Vulnerable protobuf generated type in use: " + + "protobuf_unittest.UnittestProto$TestAllExtensions"); assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE); assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_SILENCE_PROPERTY); + + // Subsequent calls for the same type do not log again. + msg.makeExtensionsImmutable(); + assertThat(logHandler.getStoredLogRecords()).hasSize(1); } }