Harden XML parsing, serialization, and randomness (#3075)

This commit introduces several security hardening improvements across the codebase:
1. XML Processing: Hardened `TransformerFactory` and `SchemaFactory` instantiations in `EppMessage.java` by explicitly enabling `XMLConstants.FEATURE_SECURE_PROCESSING` and disabling external schema access.
2. Randomness: Replaced instances of `java.util.Random` with `java.security.SecureRandom` in `SelfSignedCaCertificate.java` for stronger entropy. (Added documentation in `ProxyModule.java` explaining why `java.util.Random` is intentionally retained there for metrics sampling).
3. Deserialization: Hardened `SerializeUtils.java` by injecting an `ObjectInputFilter` into the `ObjectInputStream`, restricting deserialization strictly to expected `google.registry` classes and standard Java collections.

.gemini/skills/pr-polisher/scripts/check_diff.py

@@ -285,6 +285,25 @@ def check_diff_anti_patterns():
             if 'src/test/' in current_file and clock_now_pattern.search(code_line):
                 log_warning(f"[{current_file}] Prefer using a fixed, static constant Instant over capturing clock.now() in tests to prevent flakiness.")
 
+def check_missing_tests():
+    print("\n--- Checking for Missing Tests ---")
+    diff_files = run_cmd("git diff HEAD^ --name-only").split('\n')
+    main_files_modified = False
+    test_files_modified = False
+
+    for f in diff_files:
+        if f.startswith('core/src/main/') or f.startswith('util/src/main/'):
+            if f.endswith('.java'):
+                main_files_modified = True
+        if f.startswith('core/src/test/') or f.startswith('util/src/test/'):
+            if f.endswith('Test.java'):
+                test_files_modified = True
+
+    if main_files_modified and not test_files_modified:
+        log_warning("Production code (.java files in src/main/) was modified, but no corresponding tests (.java files in src/test/) were added or updated. You MUST proactively write tests for any new logic or bug fixes.")
+    else:
+        log_success("Test coverage check passed (tests were included or no production Java code was changed).")
+
 def main():
     print("========================================")
     print("     NOMULUS PR POLISHER CHECKLIST      ")
@@ -295,6 +314,7 @@ def main():
     check_workspace_clean()
     check_package_lock()
     check_license_headers()
+    check_missing_tests()
     check_formatting()
     check_diff_anti_patterns()
 

GEMINI.md

@@ -46,6 +46,7 @@ This document outlines foundational mandates, architectural patterns, and projec
     - **Transactional Time:** Ensure code that relies on `tm().getTransactionTime()` (or `tm().getTxTime()`) is executed within a transaction context.
 
 ### 5. Testing Best Practices
+- **Mandatory Proactive Testing:** You MUST automatically write and update tests alongside your code changes WITHOUT waiting for the user to prompt you. If you add a new feature, fix a bug, or change core logic, you are explicitly required to identify the corresponding `*Test.java` file and implement comprehensive test coverage for your changes.
 - **FakeClock and Sleeper:** Use `FakeClock` and `Sleeper` for any logic involving timeouts, delays, or expiration.
 - **Empirical Reproduction:** Before fixing a bug, always create a test case that reproduces the failure.
 - **Base Classes:** Leverage `CommandTestCase`, `EppToolCommandTestCase`, etc., to reduce boilerplate and ensure consistent setup (e.g., clock initialization).

prober/src/main/java/google/registry/monitoring/blackbox/message/EppMessage.java

@@ -177,6 +177,8 @@ public class EppMessage {
             new StreamSource(readResource(path + "launch.xsd")),
           };
       SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI);
+      schemaFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+      schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
       eppSchema = schemaFactory.newSchema(sources);
     } catch (SAXException | IOException e) {
       throw new ExceptionInInitializerError(e);
@@ -271,7 +273,9 @@ public class EppMessage {
       return null;
     }
     try {
-      Transformer transformer = TransformerFactory.newInstance().newTransformer();
+      TransformerFactory tf = TransformerFactory.newInstance();
+      tf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+      Transformer transformer = tf.newTransformer();
       StreamResult result = new StreamResult(new StringWriter());
       DOMSource source = new DOMSource(xml);
       transformer.transform(source, result);
@@ -291,7 +295,9 @@ public class EppMessage {
    */
   public static byte[] xmlDocToByteArray(Document xml) throws EppClientException {
     try {
-      Transformer transformer = TransformerFactory.newInstance().newTransformer();
+      TransformerFactory tf = TransformerFactory.newInstance();
+      tf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+      Transformer transformer = tf.newTransformer();
       StreamResult result = new StreamResult(new StringWriter());
       DOMSource source = new DOMSource(xml);
       transformer.transform(source, result);

proxy/src/main/java/google/registry/proxy/ProxyModule.java

@@ -383,6 +383,10 @@ public class ProxyModule {
   @Singleton
   @Provides
   static Random provideRandom() {
+    // Note: We intentionally use java.util.Random instead of SecureRandom here.
+    // This Random instance is injected into the hot path of the proxy exclusively for
+    // stochastic metrics sampling. Using SecureRandom would introduce severe lock
+    // contention and exhaust the system entropy pool under high load, causing DoS.
     return new Random();
   }
 

util/src/main/java/google/registry/util/SelfSignedCaCertificate.java

@@ -27,7 +27,6 @@ import java.security.SecureRandom;
 import java.security.cert.X509Certificate;
 import java.time.Instant;
 import java.util.Date;
-import java.util.Random;
 import org.bouncycastle.asn1.ASN1ObjectIdentifier;
 import org.bouncycastle.asn1.x500.X500Name;
 import org.bouncycastle.asn1.x509.BasicConstraints;
@@ -45,7 +44,7 @@ public class SelfSignedCaCertificate {
 
   private static final String DEFAULT_ISSUER_FQDN = "registry-test";
 
-  private static final Random RANDOM = new Random();
+  private static final SecureRandom RANDOM = new SecureRandom();
   private static final BouncyCastleProvider PROVIDER = new BouncyCastleProvider();
   private static final KeyPairGenerator keyGen = createKeyPairGenerator();
   private static final ImmutableMap<String, String> KEY_SIGNATURE_ALGS =

util/src/main/java/google/registry/util/SerializeUtils.java

@@ -20,6 +20,7 @@ import static com.google.common.io.BaseEncoding.base16;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.ObjectInputFilter.Config;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
@@ -60,7 +61,13 @@ public final class SerializeUtils {
       return null;
     }
     try {
-      return type.cast(new ObjectInputStream(new ByteArrayInputStream(objectBytes)).readObject());
+      ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(objectBytes));
+      // Restrict deserialization to known, trusted packages to prevent RCE gadget chain attacks.
+      ois.setObjectInputFilter(
+          Config.createFilter(
+              "google.registry.**;com.google.common.**;java.**;javax.**;jakarta.**;"
+                  + "org.hibernate.**;org.joda.**;com.googlecode.objectify.**;!*"));
+      return type.cast(ois.readObject());
     } catch (ClassNotFoundException | IOException e) {
       throw new IllegalArgumentException(
           "Unable to deserialize: objectBytes=" + base16().encode(objectBytes), e);

util/src/test/java/google/registry/util/SerializeUtilsTest.java

@@ -20,9 +20,12 @@ import static google.registry.util.SerializeUtils.parse;
 import static google.registry.util.SerializeUtils.serialize;
 import static google.registry.util.SerializeUtils.stringify;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.fail;
 
+import java.io.InvalidClassException;
 import java.io.Serializable;
 import org.junit.jupiter.api.Test;
+import org.opentest4j.AssertionFailedError;
 
 /** Unit tests for {@link SerializeUtils}. */
 class SerializeUtilsTest {
@@ -111,4 +114,19 @@ class SerializeUtilsTest {
         assertThrows(NullPointerException.class, () -> parse(String.class, null));
     assertThat(thrown).hasMessageThat().contains("Object string cannot be null");
   }
+
+  @Test
+  void testDeserialize_unauthorizedClass_isRejectedByFilter() {
+    // AssertionFailedError implements Serializable but is NOT in the
+    // whitelist of allowed deserialization packages.
+    AssertionFailedError untrustedObject = new AssertionFailedError("test");
+
+    try {
+      deserialize(Object.class, serialize(untrustedObject));
+      fail("Expected an exception");
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(InvalidClassException.class);
+      assertThat(e).hasCauseThat().hasMessageThat().contains("REJECTED");
+    }
+  }
 }