From 0030645b1adc0739fecb6948478acbbae7847631 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Mon, 1 Jun 2026 10:25:42 -0400 Subject: [PATCH] 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. --- .../skills/pr-polisher/scripts/check_diff.py | 20 +++++++++++++++++++ GEMINI.md | 1 + .../blackbox/message/EppMessage.java | 10 ++++++++-- .../google/registry/proxy/ProxyModule.java | 4 ++++ .../util/SelfSignedCaCertificate.java | 3 +-- .../google/registry/util/SerializeUtils.java | 9 ++++++++- .../registry/util/SerializeUtilsTest.java | 18 +++++++++++++++++ 7 files changed, 60 insertions(+), 5 deletions(-) mode change 100755 => 100644 .gemini/skills/pr-polisher/scripts/check_diff.py diff --git a/.gemini/skills/pr-polisher/scripts/check_diff.py b/.gemini/skills/pr-polisher/scripts/check_diff.py old mode 100755 new mode 100644 index 781cd7b9c..2f38d6913 --- a/.gemini/skills/pr-polisher/scripts/check_diff.py +++ b/.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() diff --git a/GEMINI.md b/GEMINI.md index 4b8896c57..b393c0773 100644 --- a/GEMINI.md +++ b/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). diff --git a/prober/src/main/java/google/registry/monitoring/blackbox/message/EppMessage.java b/prober/src/main/java/google/registry/monitoring/blackbox/message/EppMessage.java index 79027ddf3..dbe388fd7 100644 --- a/prober/src/main/java/google/registry/monitoring/blackbox/message/EppMessage.java +++ b/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); diff --git a/proxy/src/main/java/google/registry/proxy/ProxyModule.java b/proxy/src/main/java/google/registry/proxy/ProxyModule.java index 10fdd6166..09ceda05d 100644 --- a/proxy/src/main/java/google/registry/proxy/ProxyModule.java +++ b/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(); } diff --git a/util/src/main/java/google/registry/util/SelfSignedCaCertificate.java b/util/src/main/java/google/registry/util/SelfSignedCaCertificate.java index 31b104333..225ec7afe 100644 --- a/util/src/main/java/google/registry/util/SelfSignedCaCertificate.java +++ b/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 KEY_SIGNATURE_ALGS = diff --git a/util/src/main/java/google/registry/util/SerializeUtils.java b/util/src/main/java/google/registry/util/SerializeUtils.java index 130ad3212..1a17be493 100644 --- a/util/src/main/java/google/registry/util/SerializeUtils.java +++ b/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); diff --git a/util/src/test/java/google/registry/util/SerializeUtilsTest.java b/util/src/test/java/google/registry/util/SerializeUtilsTest.java index fe2bd13b6..b97114d5e 100644 --- a/util/src/test/java/google/registry/util/SerializeUtilsTest.java +++ b/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"); + } + } }