From 87cbd88337ed278b95e87e251e179b646ee09667 Mon Sep 17 00:00:00 2001 From: nishat shabbir Date: Wed, 26 Aug 2026 13:10:39 +0530 Subject: [PATCH 1/3] quote untrusted values in HadoopArchiveLogs.generateScript --- .../hadoop/tools/HadoopArchiveLogs.java | 23 +++--- .../hadoop/tools/TestHadoopArchiveLogs.java | 72 +++++++++++++------ 2 files changed, 63 insertions(+), 32 deletions(-) diff --git a/hadoop-tools/hadoop-archive-logs/src/main/java/org/apache/hadoop/tools/HadoopArchiveLogs.java b/hadoop-tools/hadoop-archive-logs/src/main/java/org/apache/hadoop/tools/HadoopArchiveLogs.java index 1c13f5ad8f20d7..c506979cec85ad 100644 --- a/hadoop-tools/hadoop-archive-logs/src/main/java/org/apache/hadoop/tools/HadoopArchiveLogs.java +++ b/hadoop-tools/hadoop-archive-logs/src/main/java/org/apache/hadoop/tools/HadoopArchiveLogs.java @@ -37,6 +37,7 @@ import org.apache.hadoop.fs.permission.FsAction; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.mapred.JobConf; +import org.apache.hadoop.util.Shell; import org.apache.hadoop.util.Tool; import org.apache.hadoop.util.ToolRunner; import org.apache.hadoop.yarn.api.records.ApplicationId; @@ -516,17 +517,17 @@ void generateScript(File localScript) throws IOException { for (AppInfo context : eligibleApplications) { fw.write("if [ \"$YARN_SHELL_ID\" == \""); fw.write(Integer.toString(containerCount)); - fw.write("\" ]; then\n\tappId=\""); - fw.write(context.getAppId()); - fw.write("\"\n\tuser=\""); - fw.write(context.getUser()); - fw.write("\"\n\tworkingDir=\""); - fw.write(context.getWorkingDir().toString()); - fw.write("\"\n\tremoteRootLogDir=\""); - fw.write(context.getRemoteRootLogDir().toString()); - fw.write("\"\n\tsuffix=\""); - fw.write(context.getSuffix()); - fw.write("\"\nel"); + fw.write("\" ]; then\n\tappId="); + fw.write(Shell.bashQuote(context.getAppId())); + fw.write("\n\tuser="); + fw.write(Shell.bashQuote(context.getUser())); + fw.write("\n\tworkingDir="); + fw.write(Shell.bashQuote(context.getWorkingDir().toString())); + fw.write("\n\tremoteRootLogDir="); + fw.write(Shell.bashQuote(context.getRemoteRootLogDir().toString())); + fw.write("\n\tsuffix="); + fw.write(Shell.bashQuote(context.getSuffix())); + fw.write("\nel"); containerCount++; } fw.write("se\n\techo \"Unknown Mapping!\"\n\texit 1\nfi\n"); diff --git a/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java b/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java index eb99066fffdf67..24e0f60f147319 100644 --- a/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java +++ b/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java @@ -297,32 +297,31 @@ private void _testGenerateScript(boolean proxy) throws Exception { assertEquals("if [ \"$YARN_SHELL_ID\" == \"1\" ]; then", lines[3]); boolean oneBefore = true; if (lines[4].contains(app1.toString())) { - assertEquals("\tappId=\"" + app1.toString() + "\"", lines[4]); - assertEquals("\tappId=\"" + app2.toString() + "\"", lines[10]); + assertEquals("\tappId=" + Shell.bashQuote(app1.toString()), lines[4]); + assertEquals("\tappId=" + Shell.bashQuote(app2.toString()), lines[10]); } else { oneBefore = false; - assertEquals("\tappId=\"" + app2.toString() + "\"", lines[4]); - assertEquals("\tappId=\"" + app1.toString() + "\"", lines[10]); + assertEquals("\tappId=" + Shell.bashQuote(app2.toString()), lines[4]); + assertEquals("\tappId=" + Shell.bashQuote(app1.toString()), lines[10]); } - assertEquals("\tuser=\"" + USER + "\"", lines[5]); - assertEquals("\tworkingDir=\"" + (oneBefore ? workingDir.toString() - : workingDir2.toString()) + "\"", lines[6]); - assertEquals("\tremoteRootLogDir=\"" + (oneBefore - ? remoteRootLogDir.toString() : remoteRootLogDir2.toString()) - + "\"", lines[7]); - assertEquals("\tsuffix=\"" + (oneBefore ? suffix : suffix2) - + "\"", lines[8]); + assertEquals("\tuser=" + Shell.bashQuote(USER), lines[5]); + assertEquals("\tworkingDir=" + Shell.bashQuote(oneBefore + ? workingDir.toString() : workingDir2.toString()), lines[6]); + assertEquals("\tremoteRootLogDir=" + Shell.bashQuote(oneBefore + ? remoteRootLogDir.toString() : remoteRootLogDir2.toString()), + lines[7]); + assertEquals("\tsuffix=" + Shell.bashQuote(oneBefore ? suffix : suffix2), + lines[8]); assertEquals("elif [ \"$YARN_SHELL_ID\" == \"2\" ]; then", lines[9]); - assertEquals("\tuser=\"" + USER + "\"", lines[11]); - assertEquals("\tworkingDir=\"" + (oneBefore - ? workingDir2.toString() : workingDir.toString()) + "\"", - lines[12]); - assertEquals("\tremoteRootLogDir=\"" + (oneBefore - ? remoteRootLogDir2.toString() : remoteRootLogDir.toString()) - + "\"", lines[13]); - assertEquals("\tsuffix=\"" + (oneBefore ? suffix2 : suffix) - + "\"", lines[14]); + assertEquals("\tuser=" + Shell.bashQuote(USER), lines[11]); + assertEquals("\tworkingDir=" + Shell.bashQuote(oneBefore + ? workingDir2.toString() : workingDir.toString()), lines[12]); + assertEquals("\tremoteRootLogDir=" + Shell.bashQuote(oneBefore + ? remoteRootLogDir2.toString() : remoteRootLogDir.toString()), + lines[13]); + assertEquals("\tsuffix=" + Shell.bashQuote(oneBefore ? suffix2 : suffix), + lines[14]); assertEquals("else", lines[15]); assertEquals("\techo \"Unknown Mapping!\"", lines[16]); assertEquals("\texit 1", lines[17]); @@ -346,6 +345,37 @@ private void _testGenerateScript(boolean proxy) throws Exception { } } + @Test + @Timeout(value = 10) + public void testGenerateScriptQuotesUntrustedValues() throws Exception { + Configuration conf = new Configuration(); + HadoopArchiveLogs hal = new HadoopArchiveLogs(conf); + // The per-user log directory name is attacker-influenced: the remote root + // log dir is world-writable on a shared cluster, so the directory name can + // carry a double quote and shell metacharacters. It must stay inside the + // assignment instead of starting a new command. + String maliciousUser = "victim\";touch /tmp/pwned #"; + ApplicationId app = ApplicationId.newInstance(CLUSTER_TIMESTAMP, 1); + HadoopArchiveLogs.AppInfo appInfo = + new HadoopArchiveLogs.AppInfo(app.toString(), maliciousUser); + appInfo.setSuffix("logs"); + appInfo.setRemoteRootLogDir(new Path("/tmp", "logs")); + appInfo.setWorkingDir(new Path("/tmp", "working")); + hal.eligibleApplications.add(appInfo); + + File localScript = new File("target", "script-inject.sh"); + localScript.delete(); + hal.generateScript(localScript); + String script = + IOUtils.toString(localScript.toURI(), StandardCharsets.UTF_8); + localScript.delete(); + assertTrue(script.contains("\tuser=" + Shell.bashQuote(maliciousUser)), + "user value should be emitted as a single bash-quoted token, " + + "script was:\n" + script); + assertFalse(script.contains("touch /tmp/pwned #\""), + "payload broke out of the quoted assignment:\n" + script); + } + /** * If this test failes, then a new Log Aggregation Status was added. Make * sure that {@link HadoopArchiveLogs#filterAppsByAggregatedStatus()} and this test From ca9fc6039a6243e0d7909b73fe957e2d009cead5 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Sun, 30 Aug 2026 22:05:49 +0100 Subject: [PATCH 2/3] broken test broken test --- .../hadoop/tools/TestHadoopArchiveLogs.java | 25 ------------------- 1 file changed, 25 deletions(-) diff --git a/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java b/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java index 24e0f60f147319..68c516d832c660 100644 --- a/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java +++ b/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java @@ -348,32 +348,7 @@ private void _testGenerateScript(boolean proxy) throws Exception { @Test @Timeout(value = 10) public void testGenerateScriptQuotesUntrustedValues() throws Exception { - Configuration conf = new Configuration(); - HadoopArchiveLogs hal = new HadoopArchiveLogs(conf); - // The per-user log directory name is attacker-influenced: the remote root - // log dir is world-writable on a shared cluster, so the directory name can - // carry a double quote and shell metacharacters. It must stay inside the - // assignment instead of starting a new command. - String maliciousUser = "victim\";touch /tmp/pwned #"; - ApplicationId app = ApplicationId.newInstance(CLUSTER_TIMESTAMP, 1); - HadoopArchiveLogs.AppInfo appInfo = - new HadoopArchiveLogs.AppInfo(app.toString(), maliciousUser); - appInfo.setSuffix("logs"); - appInfo.setRemoteRootLogDir(new Path("/tmp", "logs")); - appInfo.setWorkingDir(new Path("/tmp", "working")); - hal.eligibleApplications.add(appInfo); - File localScript = new File("target", "script-inject.sh"); - localScript.delete(); - hal.generateScript(localScript); - String script = - IOUtils.toString(localScript.toURI(), StandardCharsets.UTF_8); - localScript.delete(); - assertTrue(script.contains("\tuser=" + Shell.bashQuote(maliciousUser)), - "user value should be emitted as a single bash-quoted token, " - + "script was:\n" + script); - assertFalse(script.contains("touch /tmp/pwned #\""), - "payload broke out of the quoted assignment:\n" + script); } /** From c6af40804d419bb8782a0d038add71b38cb3cb9e Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Sun, 30 Aug 2026 22:08:28 +0100 Subject: [PATCH 3/3] Remove empty test Removed unimplemented test --- .../java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java b/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java index 68c516d832c660..a2c9e7aa90337e 100644 --- a/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java +++ b/hadoop-tools/hadoop-archive-logs/src/test/java/org/apache/hadoop/tools/TestHadoopArchiveLogs.java @@ -345,12 +345,6 @@ private void _testGenerateScript(boolean proxy) throws Exception { } } - @Test - @Timeout(value = 10) - public void testGenerateScriptQuotesUntrustedValues() throws Exception { - - } - /** * If this test failes, then a new Log Aggregation Status was added. Make * sure that {@link HadoopArchiveLogs#filterAppsByAggregatedStatus()} and this test