This is an automated email from the ASF dual-hosted git repository.
xxyu pushed a commit to branch kylin5
in repository https://gitbox.apache.org/repos/asf/kylin.git
commit f9b65d350d55d078e0d2776cb6384e357ef0bddc
Author: Lu Cao <whuca...@gmail.com>
AuthorDate: Mon Jun 5 17:17:33 2023 +0800
KYLIN-5700 fix shell command injection
---------
Co-authored-by: Zhiting Guo <35057824+fre...@users.noreply.github.com>
Co-authored-by: Zhiting Guo <zhiting....@kyligence.io>
---
.../apache/kylin/rest/service/SystemService.java | 5 ++
.../org/apache/kylin/common/util/StringHelper.java | 69 ++++++++++++++++++++++
.../apache/kylin/common/util/StringHelperTest.java | 65 ++++++++++++++++++++
.../apache/kylin/source/hive/HiveCmdBuilder.java | 3 +-
.../kylin/source/hive/HiveCmdBuilderTest.java | 2 +-
.../java/org/apache/kylin/tool/InfluxDBTool.java | 5 ++
.../apache/kylin/tool/setup/KapGetClusterInfo.java | 8 ++-
.../kylin/tool/util/HadoopConfExtractor.java | 6 +-
.../kylin/tool/setup/KapGetClusterInfoTest.java | 38 ++++++++++++
.../kylin/tool/util/HadoopConfExtractorTest.java | 42 +++++++++++++
10 files changed, 235 insertions(+), 8 deletions(-)
diff --git
a/src/common-service/src/main/java/org/apache/kylin/rest/service/SystemService.java
b/src/common-service/src/main/java/org/apache/kylin/rest/service/SystemService.java
index 425928703f..24505237e9 100644
---
a/src/common-service/src/main/java/org/apache/kylin/rest/service/SystemService.java
+++
b/src/common-service/src/main/java/org/apache/kylin/rest/service/SystemService.java
@@ -29,6 +29,7 @@ import static org.apache.kylin.tool.constant.StageEnum.DONE;
import java.io.File;
import java.io.IOException;
+import java.util.Arrays;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
@@ -52,6 +53,7 @@ import
org.apache.kylin.common.persistence.transaction.MessageSynchronization;
import org.apache.kylin.common.scheduler.EventBusFactory;
import org.apache.kylin.common.util.BufferedLogger;
import org.apache.kylin.common.util.CliCommandExecutor;
+import org.apache.kylin.common.util.StringHelper;
import org.apache.kylin.helper.MetadataToolHelper;
import org.apache.kylin.job.execution.AbstractExecutable;
import org.apache.kylin.job.execution.NExecutableManager;
@@ -165,6 +167,9 @@ public class SystemService extends BasicService {
Future<?> task = executorService.submit(() -> {
try {
exceptionMap.invalidate(uuid);
+ if
(!Arrays.stream(arguments).allMatch(StringHelper::validateShellArgument)) {
+ throw new IllegalArgumentException("Shell args have
illegal char: " + Arrays.toString(arguments));
+ }
String finalCommand = String.format(Locale.ROOT,
"%s/bin/diag.sh %s", KylinConfig.getKylinHome(),
StringUtils.join(arguments, " "));
commandExecutor.execute(finalCommand, patternedLogger, uuid);
diff --git
a/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java
b/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java
index 3754b11a19..7c31565573 100644
---
a/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java
+++
b/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java
@@ -20,8 +20,11 @@ package org.apache.kylin.common.util;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Arrays;
+import java.util.Iterator;
import java.util.List;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import org.apache.commons.lang3.StringUtils;
@@ -197,6 +200,72 @@ public class StringHelper {
return str.split(splitBy);
}
+ public static boolean validateUrl(String s) {
+ return
Pattern.compile("^(http(s)?://)?[a-zA-Z0-9._-]+(:[0-9]+)?(/[a-zA-Z0-9._-]+)*/?$").matcher(s).matches();
+ }
+
+ public static boolean validateDbName(String s) {
+ return Pattern.compile("^[0-9a-zA-Z_-]+$").matcher(s).matches();
+ }
+
+ public static boolean validateShellArgument(String s) {
+ return Pattern.compile("^[a-zA-Z0-9_./-]+$").matcher(s).matches();
+ }
+
+ public static String escapeShellArguments(String args) {
+ String[] expandArgs = Arrays.stream(args.split(" ")).filter(arg ->
!arg.isEmpty()).toArray(String[]::new);
+ return String.join(" ", escapeShellArguments(expandArgs));
+ }
+
+ /**
+ * support cases:
+ * -u root
+ * --user root
+ * --user=root
+ */
+ public static String[] escapeShellArguments(String[] args) {
+ Pattern keyPattern = Pattern.compile("^[a-zA-Z0-9-]+$");
+ String key;
+ String value;
+ Iterator<String> argsIterator = Arrays.stream(args).iterator();
+ List<String> newArgs = new ArrayList<>();
+ while (argsIterator.hasNext()) {
+ String cur = argsIterator.next();
+ if (!cur.startsWith("-")) {
+ throw new IllegalArgumentException("Unexpected args found: " +
Arrays.toString(args));
+ }
+ boolean useEqual = false;
+ if (cur.contains("=")) {
+ useEqual = true;
+ int index = cur.indexOf('=');
+ key = cur.substring(0, index);
+ value = cur.substring(index + 1);
+ } else {
+ key = cur;
+ value = argsIterator.next();
+ }
+ if (!keyPattern.matcher(key).matches()) {
+ throw new IllegalArgumentException("Unexpected args found: " +
Arrays.toString(args));
+ }
+ // a'b'c -> a b c -> 'a'\''b'\''c'
+ // ''a'b'c'' -> 'a'b'c' -> _ a b c _ -> \\''a'\\''b'\\''c'\\'
+ if (value.startsWith("'") && value.endsWith("'") && value.length()
>= 2) {
+ value = value.substring(1, value.length() - 1);
+ }
+ String[] splitValues = value.split("'", -1);
+ value = Arrays.stream(splitValues).map(v -> v.isEmpty() ? v : "'"
+ v + "'")
+ .collect(Collectors.joining("\\'"));
+
+ if (useEqual) {
+ newArgs.add(key + "=" + value);
+ } else {
+ newArgs.add(key);
+ newArgs.add(value);
+ }
+ }
+ return newArgs.toArray(new String[0]);
+ }
+
public static String backtickToDoubleQuote(String expression) {
return convert(expression, StringHelper.BACKTICK,
StringHelper.DOUBLE_QUOTE);
}
diff --git
a/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
b/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
index dcadc2a013..517e5e08af 100644
---
a/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
+++
b/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
@@ -101,4 +101,69 @@ class StringHelperTest {
Assertions.assertEquals("b", arr[1]);
Assertions.assertEquals("c", arr[2]);
}
+
+ @Test
+ void testValidateUrl() {
+ Assertions.assertTrue(StringHelper.validateUrl("127.0.0.1"));
+ Assertions.assertTrue(StringHelper.validateUrl("kylin.apache.org"));
+ Assertions.assertTrue(StringHelper.validateUrl("kylin"));
+ Assertions.assertTrue(StringHelper.validateUrl("http://127.0.0.1";));
+
Assertions.assertTrue(StringHelper.validateUrl("https://kylin.apache.org";));
+ Assertions.assertTrue(StringHelper.validateUrl("http://kylin";));
+
Assertions.assertTrue(StringHelper.validateUrl("http://127.0.0.1/a_p.i";));
+
Assertions.assertTrue(StringHelper.validateUrl("https://kylin.apache.org/api/te-st/";));
+ Assertions.assertTrue(StringHelper.validateUrl("http://kylin/";));
+ }
+
+ @Test
+ void testValidIllegalUrl() {
+ Assertions.assertFalse(StringHelper.validateUrl("http://kylin/$(rm -rf
/)"));
+ Assertions.assertFalse(StringHelper.validateUrl("http://kylin/`rm
-rf`"));
+ Assertions.assertFalse(StringHelper.validateUrl("http://kylin/'&ls"));
+ Assertions.assertFalse(StringHelper.validateUrl("http://kylin/;ls";));
+ Assertions.assertFalse(StringHelper.validateUrl("http://kylin/>ls"));
+ Assertions.assertFalse(StringHelper.validateUrl(""));
+ }
+
+ @Test
+ void testValidateDB() {
+ Assertions.assertTrue(StringHelper.validateDbName("db_TEST-01"));
+ Assertions.assertFalse(StringHelper.validateDbName("db&&ls"));
+ }
+
+ @Test
+ void testValidateArgument() {
+ Assertions.assertTrue(StringHelper.validateShellArgument("-job"));
+
Assertions.assertTrue(StringHelper.validateShellArgument("uuid-uuid-uuid-uuid"));
+ Assertions.assertTrue(StringHelper.validateShellArgument("-JobId"));
+
Assertions.assertTrue(StringHelper.validateShellArgument("/o-pt.5.0-3/kylin/te_st"));
+
+ Assertions.assertFalse(StringHelper.validateShellArgument("`ls`"));
+ Assertions.assertFalse(StringHelper.validateShellArgument("$(ls)"));
+ Assertions.assertFalse(StringHelper.validateShellArgument("&&"));
+ }
+
+ @Test
+ void testEscapeArguments() {
+ Assertions.assertEquals("", StringHelper.escapeShellArguments(""));
+ Assertions.assertEquals("-u 'root'",
StringHelper.escapeShellArguments("-u root "));
+ Assertions.assertEquals("-u 'root'",
StringHelper.escapeShellArguments("-u 'root' "));
+ Assertions.assertEquals("-u \\''root'\\'",
StringHelper.escapeShellArguments("-u ''root'' "));
+ Assertions.assertEquals("-u \\'",
StringHelper.escapeShellArguments("-u '"));
+ Assertions.assertEquals("-u ", StringHelper.escapeShellArguments("-u
''"));
+ Assertions.assertEquals("-u 'root'",
StringHelper.escapeShellArguments("-u root"));
+ Assertions.assertEquals("-UserName 'root'",
StringHelper.escapeShellArguments("-UserName root"));
+ Assertions.assertEquals("-user='root'",
StringHelper.escapeShellArguments("-user=root"));
+ Assertions.assertEquals("-user='ro=ot'",
StringHelper.escapeShellArguments("-user=ro=ot"));
+
+ Assertions.assertEquals("-u 'root'\\''$(ls)'",
StringHelper.escapeShellArguments("-u root'$(ls)"));
+ Assertions.assertEquals("-UserName 'root'\\''$(ls)'",
+ StringHelper.escapeShellArguments("-UserName root'$(ls)"));
+ Assertions.assertEquals("-user='roo'\\''$(ls)t'",
StringHelper.escapeShellArguments("-user=roo'$(ls)t"));
+ Assertions.assertEquals("-user='roo'\\''$(ls)'\\''t'",
+ StringHelper.escapeShellArguments("-user='roo'$(ls)'t'"));
+
+ Assertions.assertThrows(IllegalArgumentException.class, () ->
StringHelper.escapeShellArguments("u root"));
+ Assertions.assertThrows(IllegalArgumentException.class, () ->
StringHelper.escapeShellArguments("-u.ser root"));
+ }
}
diff --git
a/src/source-hive/src/main/java/org/apache/kylin/source/hive/HiveCmdBuilder.java
b/src/source-hive/src/main/java/org/apache/kylin/source/hive/HiveCmdBuilder.java
index ee7b1c5697..4c68b8b7b1 100644
---
a/src/source-hive/src/main/java/org/apache/kylin/source/hive/HiveCmdBuilder.java
+++
b/src/source-hive/src/main/java/org/apache/kylin/source/hive/HiveCmdBuilder.java
@@ -23,6 +23,7 @@ import java.util.Locale;
import java.util.UUID;
import org.apache.kylin.common.KylinConfig;
+import org.apache.kylin.common.util.StringHelper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -44,7 +45,7 @@ public class HiveCmdBuilder {
}
public String build() {
- String beelineParams = kylinConfig.getHiveBeelineParams();
+ String beelineParams =
StringHelper.escapeShellArguments(kylinConfig.getHiveBeelineParams());
StringBuilder buf = new StringBuilder();
String tmpBeelineHqlPath = null;
StringBuilder beelineHql = new StringBuilder();
diff --git
a/src/source-hive/src/test/java/org/apache/kylin/source/hive/HiveCmdBuilderTest.java
b/src/source-hive/src/test/java/org/apache/kylin/source/hive/HiveCmdBuilderTest.java
index a832be9dc5..9f7e99dee5 100644
---
a/src/source-hive/src/test/java/org/apache/kylin/source/hive/HiveCmdBuilderTest.java
+++
b/src/source-hive/src/test/java/org/apache/kylin/source/hive/HiveCmdBuilderTest.java
@@ -68,7 +68,7 @@ public class HiveCmdBuilderTest {
private void assertBeelineCmd(String cmd) {
String beelineCmd = cmd.substring(cmd.indexOf("EOL\n",
cmd.indexOf("EOL\n") + 1) + 4);
- assertTrue(beelineCmd.startsWith("beeline -u jdbc_url"));
+ assertTrue(beelineCmd.startsWith("beeline -u 'jdbc_url'"));
}
}
diff --git a/src/tool/src/main/java/org/apache/kylin/tool/InfluxDBTool.java
b/src/tool/src/main/java/org/apache/kylin/tool/InfluxDBTool.java
index b728437113..a9564f3aa9 100644
--- a/src/tool/src/main/java/org/apache/kylin/tool/InfluxDBTool.java
+++ b/src/tool/src/main/java/org/apache/kylin/tool/InfluxDBTool.java
@@ -25,6 +25,7 @@ import org.apache.kylin.common.KapConfig;
import org.apache.kylin.common.KylinConfig;
import org.apache.kylin.common.util.CliCommandExecutor;
import org.apache.kylin.common.metrics.service.MonitorDao;
+import org.apache.kylin.common.util.StringHelper;
import org.apache.kylin.tool.util.ToolUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -72,6 +73,10 @@ public class InfluxDBTool {
influxd = new File(influxDBHome + INFLUXD_PATH);
}
+ if (!(StringHelper.validateDbName(database) &&
StringHelper.validateUrl(host))) {
+ throw new IllegalArgumentException(
+ "Something is wrong with Influx database and host: " +
database + ", " + host);
+ }
String cmd = String.format(Locale.ROOT, "%s backup -portable
-database %s -host %s %s",
influxd.getAbsolutePath(), database, host,
destDir.getAbsolutePath());
logger.info("InfluxDB backup cmd is {}.", cmd);
diff --git
a/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java
b/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java
index d3cf151d59..74d518c843 100644
--- a/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java
+++ b/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java
@@ -24,7 +24,6 @@ import java.nio.charset.Charset;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Matcher;
-import java.util.regex.Pattern;
import org.apache.commons.io.FileUtils;
import org.apache.hadoop.conf.Configuration;
@@ -35,6 +34,7 @@ import org.apache.kylin.common.util.BufferedLogger;
import org.apache.kylin.common.util.HadoopUtil;
import org.apache.kylin.common.util.ShellException;
import org.apache.kylin.cluster.SchedulerInfoCmdHelper;
+import org.apache.kylin.common.util.StringHelper;
import org.apache.kylin.common.util.Unsafe;
import org.apache.kylin.tool.util.HadoopConfExtractor;
import org.slf4j.Logger;
@@ -88,9 +88,8 @@ public class KapGetClusterInfo {
}
public void extractYarnMasterHost() {
- Pattern pattern = Pattern.compile("(http://)([^:]*):([^/])*.*");
if (yarnMasterUrlBase != null) {
- Matcher m = pattern.matcher(yarnMasterUrlBase);
+ Matcher m =
HadoopConfExtractor.URL_PATTERN.matcher(yarnMasterUrlBase);
if (m.matches()) {
return;
}
@@ -106,6 +105,9 @@ public class KapGetClusterInfo {
public void getYarnMetrics() throws IOException, ShellException,
YarnException {
extractYarnMasterHost();
String url = yarnMasterUrlBase + YARN_METRICS_SUFFIX;
+ if (!StringHelper.validateUrl(url)) {
+ throw new IllegalArgumentException("Url contains disallowed chars,
url: " + url);
+ }
String command = "curl -s -k --negotiate -u : " + url;
KylinConfig config = KylinConfig.getInstanceFromEnv();
val patternedLogger = new BufferedLogger(logger);
diff --git
a/src/tool/src/main/java/org/apache/kylin/tool/util/HadoopConfExtractor.java
b/src/tool/src/main/java/org/apache/kylin/tool/util/HadoopConfExtractor.java
index ab5c2efebe..8683324e64 100644
--- a/src/tool/src/main/java/org/apache/kylin/tool/util/HadoopConfExtractor.java
+++ b/src/tool/src/main/java/org/apache/kylin/tool/util/HadoopConfExtractor.java
@@ -34,6 +34,7 @@ import
org.apache.kylin.guava30.shaded.common.base.Preconditions;
public class HadoopConfExtractor {
private static final Logger logger =
LoggerFactory.getLogger(HadoopConfExtractor.class);
+ public static final Pattern URL_PATTERN =
Pattern.compile("(http[s]?://)([^:]*):([^/]*).*");
private HadoopConfExtractor() {
}
@@ -41,9 +42,8 @@ public class HadoopConfExtractor {
public static String extractYarnMasterUrl(Configuration conf) {
KylinConfig config = KylinConfig.getInstanceFromEnv();
final String yarnStatusCheckUrl = config.getYarnStatusCheckUrl();
- Pattern pattern = Pattern.compile("(http(s)?://)([^:]*):([^/])*.*");
if (yarnStatusCheckUrl != null) {
- Matcher m = pattern.matcher(yarnStatusCheckUrl);
+ Matcher m = URL_PATTERN.matcher(yarnStatusCheckUrl);
if (m.matches()) {
return m.group(1) + m.group(2) + ":" + m.group(3);
}
@@ -77,7 +77,7 @@ public class HadoopConfExtractor {
if (!rmWebHost.startsWith("http://";) &&
!rmWebHost.startsWith("https://";)) {
rmWebHost = (YarnConfiguration.useHttps(conf) ? "https://"; :
"http://";) + rmWebHost;
}
- Matcher m = pattern.matcher(rmWebHost);
+ Matcher m = URL_PATTERN.matcher(rmWebHost);
Preconditions.checkArgument(m.matches(), "Yarn master URL not found.");
logger.info("yarn master url: {} ", rmWebHost);
return rmWebHost;
diff --git
a/src/tool/src/test/java/org/apache/kylin/tool/setup/KapGetClusterInfoTest.java
b/src/tool/src/test/java/org/apache/kylin/tool/setup/KapGetClusterInfoTest.java
new file mode 100644
index 0000000000..5f4f628574
--- /dev/null
+++
b/src/tool/src/test/java/org/apache/kylin/tool/setup/KapGetClusterInfoTest.java
@@ -0,0 +1,38 @@
+/*
+ * 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.kylin.tool.setup;
+
+import org.apache.kylin.common.KylinConfig;
+import org.apache.kylin.junit.annotation.MetadataInfo;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+@MetadataInfo(onlyProps = true)
+class KapGetClusterInfoTest {
+
+ @Test
+ void testGetYarnMetrics() {
+
KylinConfig.getInstanceFromEnv().setProperty("kylin.job.yarn-app-rest-check-status-url",
+ "http://127.0.0.1:8080`echo`";);
+ Assertions.assertThrows(IllegalArgumentException.class, () -> {
+ KapGetClusterInfo getClusterInfo = new KapGetClusterInfo();
+ getClusterInfo.getYarnMetrics();
+ });
+ }
+}
diff --git
a/src/tool/src/test/java/org/apache/kylin/tool/util/HadoopConfExtractorTest.java
b/src/tool/src/test/java/org/apache/kylin/tool/util/HadoopConfExtractorTest.java
new file mode 100644
index 0000000000..d9f5ea3d26
--- /dev/null
+++
b/src/tool/src/test/java/org/apache/kylin/tool/util/HadoopConfExtractorTest.java
@@ -0,0 +1,42 @@
+/*
+ * 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.kylin.tool.util;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.kylin.junit.annotation.MetadataInfo;
+import org.apache.kylin.junit.annotation.OverwriteProp;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+@MetadataInfo(onlyProps = true)
+class HadoopConfExtractorTest {
+
+ @Test
+ @OverwriteProp(key = "kylin.job.yarn-app-rest-check-status-url", value =
"http://127.0.0.1:8080";)
+ void testExtractYarnMasterHost() {
+ String masterUrl = HadoopConfExtractor.extractYarnMasterUrl(new
Configuration());
+ Assertions.assertEquals("http://127.0.0.1:8080";, masterUrl);
+ }
+
+ @Test
+ void testDefaultExtractYarnMasterHost() {
+ String masterUrl = HadoopConfExtractor.extractYarnMasterUrl(new
Configuration());
+ Assertions.assertEquals("http://0.0.0.0:8088";, masterUrl);
+ }
+}
