Copilot commented on code in PR #8689:
URL: https://github.com/apache/hadoop/pull/8689#discussion_r3812213294
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/container/ResourceMappings.java:
##########
@@ -89,9 +91,24 @@ public void updateAssignedResources(List<Serializable> list)
{
public static AssignedResources fromBytes(byte[] bytes)
throws IOException {
final List<Serializable> resources;
- try {
- resources = SerializationUtils.deserialize(bytes);
- } catch (SerializationException e) {
+ // The bytes come from the NM recovery state store and are read back
+ // during container recovery on restart. Deserialize through a
+ // ValidatingObjectInputStream so a tampered record cannot instantiate
+ // arbitrary serializable classes on the NodeManager classpath. The
+ // allowed graph is the assigned-resource value objects the resource
+ // plugins store (device / NUMA descriptors, plain strings) plus the
+ // collection types that wrap them.
+ try (ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
+ ValidatingObjectInputStream ois =
+ new ValidatingObjectInputStream(bais)) {
+ ois.accept(
+ "org.apache.hadoop.yarn.server.nodemanager.*",
+ "org.apache.hadoop.thirdparty.com.google.common.collect.*",
+ "java.util.*",
+ "java.lang.*",
+ "[Ljava.lang.Object;");
+ resources = (List<Serializable>) ois.readObject();
Review Comment:
If the payload deserializes to an allowed but non-`List` type, this cast
will throw `ClassCastException`, which escapes as an unchecked exception
despite the method contract advertising failure via `IOException`. Recommend
validating the deserialized object type (and optionally element types) and
converting `ClassCastException` into an `IOException` so callers consistently
see checked I/O failure for malformed/tampered recovery records.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/container/TestResourceMappings.java:
##########
@@ -94,6 +99,38 @@ public void
testAssignedResourcesCanDeserializePreviouslySerializedValues() {
}
}
+ @Test
+ public void testRoundTripCoversResourcePluginTypes() throws IOException {
+ // The elements a NodeManager actually stores for gpu / fpga / numa
+ // resources must survive the allowlist, otherwise recovery would break.
+ ResourceMappings.AssignedResources pluginResources =
+ new ResourceMappings.AssignedResources();
+ pluginResources.updateAssignedResources(ImmutableList.of(
+ new GpuDevice(2, 3),
+ new FpgaDevice("IntelOpenCL", 247, 0, "aclv0"),
+ new NumaResourceAllocation("0", 1024L, "0", 4),
+ "cpu-0"));
+
+ ResourceMappings.AssignedResources deserialized =
+
ResourceMappings.AssignedResources.fromBytes(pluginResources.toBytes());
+
+ assertEquals(pluginResources.getAssignedResources(),
+ deserialized.getAssignedResources());
+ }
+
+ @Test
+ public void testFromBytesRejectsUnexpectedType() throws IOException {
+ // A tampered record whose top-level list is fine but which carries an
+ // element of a type the resource plugins never store. This stands in for a
+ // serialization gadget (e.g. a commons-beanutils BeanComparator): the
+ // allowlist rejects it by class name during readObject, before the class
+ // is loaded or any of its logic runs.
Review Comment:
This comment states the class is rejected 'before the class is loaded', but
depending on the `ValidatingObjectInputStream` implementation and JVM behavior,
the class may still be resolved/loaded during deserialization even if
instantiation is blocked. Suggest rewording to the verifiable property (e.g.,
'before the object is instantiated/deserialized') to avoid overstating the
guarantee.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/container/ResourceMappings.java:
##########
@@ -89,9 +91,24 @@ public void updateAssignedResources(List<Serializable> list)
{
public static AssignedResources fromBytes(byte[] bytes)
throws IOException {
final List<Serializable> resources;
- try {
- resources = SerializationUtils.deserialize(bytes);
- } catch (SerializationException e) {
+ // The bytes come from the NM recovery state store and are read back
+ // during container recovery on restart. Deserialize through a
+ // ValidatingObjectInputStream so a tampered record cannot instantiate
+ // arbitrary serializable classes on the NodeManager classpath. The
+ // allowed graph is the assigned-resource value objects the resource
+ // plugins store (device / NUMA descriptors, plain strings) plus the
+ // collection types that wrap them.
+ try (ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
+ ValidatingObjectInputStream ois =
+ new ValidatingObjectInputStream(bais)) {
+ ois.accept(
+ "org.apache.hadoop.yarn.server.nodemanager.*",
+ "org.apache.hadoop.thirdparty.com.google.common.collect.*",
+ "java.util.*",
+ "java.lang.*",
+ "[Ljava.lang.Object;");
Review Comment:
The allowlist is currently very broad
(`org.apache.hadoop.yarn.server.nodemanager.*`, `java.util.*`, `java.lang.*`,
and `...guava...*`). This undermines the goal of restricting deserialization to
known resource-plugin value types, and it can re-enable gadget-style
exploitation using classes that happen to be `Serializable` within those
packages (including JDK collection gadgets). Recommend tightening to an
explicit allowlist of concrete classes actually written (e.g., `GpuDevice`,
`FpgaDevice`, `NumaResourceAllocation`, `String`, primitive wrappers) and only
the minimal concrete collection types observed on the wire (e.g.,
`java.util.ArrayList` / specific Guava immutable collection implementation
classes), rather than whole-package wildcards.
--
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]
