sarutak commented on code in PR #58016: URL: https://github.com/apache/spark/pull/58016#discussion_r4012545339
########## launcher/src/test/java/org/apache/spark/launcher/FilteredObjectInputStreamSuite.java: ########## @@ -0,0 +1,194 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.launcher; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.ObjectOutputStream; +import java.io.ObjectStreamClass; +import java.util.ArrayList; +import java.util.HashMap; + +import org.apache.spark.launchermalicious.LauncherPrefixSpoof; +import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.*; + +/** + * Tests for {@link FilteredObjectInputStream} - the SPARK-20922 allow-list + * guarding the launcher's local socket protocol. Covers the security-relevant + * {@code resolveClass} path that previously had zero direct tests. + */ +public class FilteredObjectInputStreamSuite extends BaseSuite { + + @Test + public void testAllowedJavaLangStringIsAccepted() throws Exception { + String original = "hello"; + Object deserialized = roundTrip(original); + assertEquals(original, deserialized); + } + + @Test + public void testAllowedJavaLangIntegerIsAccepted() throws Exception { + Integer original = 42; + Object deserialized = roundTrip(original); + assertEquals(original, deserialized); + } + + @Test + public void testAllowedLauncherMessageIsAccepted() throws Exception { + LauncherProtocol.Hello original = new LauncherProtocol.Hello("secret", "3.5.0"); + LauncherProtocol.Hello deserialized = + (LauncherProtocol.Hello) roundTrip(original); + assertEquals(original.secret, deserialized.secret); + assertEquals(original.sparkVersion, deserialized.sparkVersion); + } + + @Test + public void testAllowedLauncherSetAppIdIsAccepted() throws Exception { + LauncherProtocol.SetAppId original = new LauncherProtocol.SetAppId("app-123"); + LauncherProtocol.SetAppId deserialized = + (LauncherProtocol.SetAppId) roundTrip(original); + assertEquals(original.appId, deserialized.appId); + } + + @Test + public void testDisallowedHashMapIsRejected() throws Exception { + HashMap<String, String> payload = new HashMap<>(); + payload.put("k", "v"); + byte[] bytes = serialize(payload); + IllegalArgumentException thrown = assertThrows( + IllegalArgumentException.class, + () -> deserializeFiltered(bytes)); + assertTrue(thrown.getMessage().contains("Unexpected class in stream")); + assertTrue(thrown.getMessage().contains("java.util.HashMap")); + } + + @Test + public void testDisallowedArrayListIsRejected() throws Exception { + ArrayList<String> payload = new ArrayList<>(); + payload.add("a"); + byte[] bytes = serialize(payload); + IllegalArgumentException thrown = assertThrows( + IllegalArgumentException.class, + () -> deserializeFiltered(bytes)); + assertTrue(thrown.getMessage().contains("Unexpected class in stream")); + assertTrue(thrown.getMessage().contains("java.util.ArrayList")); + } + + @Test + public void testDisallowedCustomClassIsRejected() throws Exception { + // File is Serializable but lives in java.io, not in the allow-list. + File payload = new File("/tmp/evil"); + byte[] bytes = serialize(payload); + IllegalArgumentException thrown = assertThrows( + IllegalArgumentException.class, + () -> deserializeFiltered(bytes)); + assertTrue(thrown.getMessage().contains("Unexpected class in stream")); + assertTrue(thrown.getMessage().contains("java.io.File")); + } + + // ALLOWED_PACKAGES entries end in a literal dot, so a class merely sharing the + // "org.apache.spark.launcher" text without being in that package must still be + // rejected: LauncherPrefixSpoof (org.apache.spark.launchermalicious.LauncherPrefixSpoof) + // pins down that boundary, since startsWith("org.apache.spark.launcher.") is false + // once the character after the prefix is "M" rather than ".". A matching spoof of + // the "java.lang." prefix (e.g. java.langfoo.Bar) can't be tested the same way: the + // JVM refuses to define any class whose package starts with "java.", so no real + // Class backs that name. + @Test + public void testDisallowedLauncherPrefixSpoofIsRejected() throws Exception { + LauncherPrefixSpoof payload = new LauncherPrefixSpoof(); + byte[] bytes = serialize(payload); + IllegalArgumentException thrown = assertThrows( + IllegalArgumentException.class, + () -> deserializeFiltered(bytes)); + assertTrue(thrown.getMessage().contains("Unexpected class in stream")); + assertTrue(thrown.getMessage().contains( + "org.apache.spark.launchermalicious.LauncherPrefixSpoof")); + } + + // The three tests below document CURRENT resolveClass behavior for classes in + // java.lang.* subpackages (java.lang.reflect, java.lang.invoke, java.lang.ref). + // desc.getName().startsWith("java.lang.") matches these too, since a subpackage's + // fully-qualified name still starts with the literal string "java.lang." - not just + // the java.lang package itself. The original SPARK-20922 PR's stated intent was "just + // two packages" (an exact-package match), so this is a real gap between intent and + // implementation, not a design choice made in this PR; fixing resolveClass itself is + // intentionally out of scope here (see SPARK-58785 discussion) and left for a follow-up. + // + // None of Field, MethodHandle, or WeakReference are actually serializable (constructing + // them for a round-trip throws NotSerializableException), so this boundary can't be + // exercised the way the tests above are; resolveClass is called directly against a + // synthetic descriptor instead. That gap in reachability - not just missing tests - is + // why this went uncovered by LauncherServerSuite's indirect coverage for 9 years. + + @Test + public void testJavaLangReflectFieldIsCurrentlyAllowed() throws Exception { Review Comment: The three `testJavaLang*IsCurrentlyAllowed` tests pin down current over-broad behavior and will (correctly) fail once `resolveClass` is tightened to an exact-package match. The comment points to "SPARK-58785 discussion" for the follow-up, but SPARK-58785 is this PR's own ticket, so there's no tracking anchor for the eventual fix. Could you file a dedicated follow-up JIRA and cite its id in those test comments? That way the tests are discoverable when the behavior is flipped. (Optionally, the missing `ObjectInputFilter` maxarray/maxrefs/maxdepth DoS limits could be tracked in the same ticket.) One test worth *adding* here (the only genuinely new branch I found): the same subpackage over-reach exists for the **other** allowed prefix, and is currently uncovered. `org.apache.spark.launcher.foo.Bar` satisfies `startsWith("org.apache.spark.launcher.")` and is therefore **allowed**, exactly mirroring the `java.lang.*` subpackage case. Unlike `java.lang.*`, there is no JVM restriction here, so a Serializable fixture in a launcher subpackage (e.g. `org.apache.spark.launcher.testpkg.SubpackageAllowed`) can be exercised with a normal round-trip - no direct `resolveClass` call needed. Adding it would document both allowed prefixes' over-reach symmetrically and give the future tightening a second test to flip. Optional, but it closes the one real gap. ########## launcher/src/test/java/org/apache/spark/launcher/FilteredObjectInputStreamSuite.java: ########## @@ -0,0 +1,194 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.launcher; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.ObjectOutputStream; +import java.io.ObjectStreamClass; +import java.util.ArrayList; +import java.util.HashMap; + +import org.apache.spark.launchermalicious.LauncherPrefixSpoof; +import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.*; + +/** + * Tests for {@link FilteredObjectInputStream} - the SPARK-20922 allow-list + * guarding the launcher's local socket protocol. Covers the security-relevant + * {@code resolveClass} path that previously had zero direct tests. + */ +public class FilteredObjectInputStreamSuite extends BaseSuite { + + @Test + public void testAllowedJavaLangStringIsAccepted() throws Exception { + String original = "hello"; + Object deserialized = roundTrip(original); + assertEquals(original, deserialized); + } + + @Test + public void testAllowedJavaLangIntegerIsAccepted() throws Exception { + Integer original = 42; + Object deserialized = roundTrip(original); + assertEquals(original, deserialized); + } + + @Test + public void testAllowedLauncherMessageIsAccepted() throws Exception { + LauncherProtocol.Hello original = new LauncherProtocol.Hello("secret", "3.5.0"); + LauncherProtocol.Hello deserialized = + (LauncherProtocol.Hello) roundTrip(original); + assertEquals(original.secret, deserialized.secret); + assertEquals(original.sparkVersion, deserialized.sparkVersion); + } + + @Test + public void testAllowedLauncherSetAppIdIsAccepted() throws Exception { + LauncherProtocol.SetAppId original = new LauncherProtocol.SetAppId("app-123"); + LauncherProtocol.SetAppId deserialized = + (LauncherProtocol.SetAppId) roundTrip(original); + assertEquals(original.appId, deserialized.appId); + } + + @Test + public void testDisallowedHashMapIsRejected() throws Exception { Review Comment: `resolveClass` decides purely on the fully-qualified name: `desc.getName().startsWith(p)`. It never looks at the class's kind, origin, or classloader, and whether the class is a JDK built-in or a user-defined type is irrelevant. Viewed that way, the reject-side tests really exercise *which prefix is (not) matched*, so it helps to name and pick them by the boundary they cover: - `testDisallowedHashMapIsRejected` (`java.util.`) and `testDisallowedArrayListIsRejected` (`java.util.`) hit the **same** `java.util.` prefix and take an identical path, so one is redundant. But note this generalizes: the reject decision is simply "matches neither allowed prefix", so `java.util.*`, `java.io.*`, or any other unrelated package are all the *same* path - swapping in or adding another unrelated prefix does not add branch coverage either. A single unrelated-package reject case is enough; the `HashMap`/`ArrayList`/`File` trio can collapse to one. - `testDisallowedCustomClassIsRejected` uses `java.io.File` - a JDK class, not a custom class - and the inline comment says exactly that. Given the point above, it does not cover a meaningfully different branch from the `HashMap` case; it is just another unrelated package. If it is kept, `testDisallowedJavaIoFileIsRejected` would at least be an accurate, payload-based name consistent with its siblings. - The genuinely user-defined / "custom class" case is in fact already covered by `testDisallowedLauncherPrefixSpoofIsRejected` (`LauncherPrefixSpoof` is a user-defined Serializable class in a non-allowed package), and it additionally pins the trailing-dot boundary. Since the filter doesn't distinguish custom from built-in, there's no need to add a separate custom-class test - the "custom" label on the `File` test is just a misnomer. Net: collapse the unrelated-package reject tests to one (they share a single path) and give it an accurate name; the meaningful boundaries are already covered by the spoof and `java.lang.*` tests. All cosmetic - no behavior impact. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
