fix

apache · Feb 11, 2022 · bd1b602 · bd1b602
1 parent 8365e66
commit bd1b602
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 61 deletions.
diff --git a/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java b/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java
@@ -46,6 +46,7 @@
 import org.apache.kylin.common.util.CliCommandExecutor;
 import org.apache.kylin.common.util.FileUtils;
 import org.apache.kylin.common.util.HadoopUtil;
+import org.apache.kylin.common.util.ParameterFilter;
 import org.apache.kylin.common.util.StringUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -1113,7 +1114,7 @@ public boolean isHiveKeepFlatTable() {
     }
 
     public String getHiveDatabaseForIntermediateTable() {
-        return CliCommandExecutor.checkHiveProperty(this.getOptional("kylin.source.hive.database-for-flat-table", DEFAULT));
+        return ParameterFilter.checkHiveProperty(this.getOptional("kylin.source.hive.database-for-flat-table", DEFAULT));
     }
 
     public String getFlatTableStorageFormat() {

diff --git a/core-common/src/main/java/org/apache/kylin/common/util/CliCommandExecutor.java b/core-common/src/main/java/org/apache/kylin/common/util/CliCommandExecutor.java
@@ -175,50 +175,4 @@ private Pair<Integer, String> runNativeCommand(String command, Logger logAppende
             }
         }
     }
-
-    public static final String COMMAND_BLOCK_LIST = "[ &`>|{}()$;\\-#~!+*\\\\]+";
-    public static final String COMMAND_WHITE_LIST = "[^\\w%,@/:=?.\"\\[\\]]";
-    public static final String HIVE_BLOCK_LIST = "[ <>()$;\\-#!+*\"'/=%@]+";
-
-
-    /**
-     * <pre>
-     * Check parameter for preventing command injection, replace illegal character into empty character.
-     *
-     * Note:
-     * 1. Whitespace is also refused because parameter is a single word, should not contains it
-     * 2. Some character may be illegal but still be accepted because commandParameter maybe a URI/path expression,
-     *     you may check "Character part" in https://docs.oracle.com/javase/8/docs/api/java/net/URI.html,
-     *     here is the character which is not banned.
-     *
-     *     1. dot .
-     *     2. slash /
-     *     3. colon :
-     *     4. equal =
-     *     5. ?
-     *     6. @
-     *     7. bracket []
-     *     8. comma ,
-     *     9. %
-     * </pre>
-     */
-    public static String checkParameter(String commandParameter) {
-        return checkParameter(commandParameter, COMMAND_BLOCK_LIST);
-    }
-
-    public static String checkParameterWhiteList(String commandParameter) {
-        return checkParameter(commandParameter, COMMAND_WHITE_LIST);
-    }
-
-    public static String checkHiveProperty(String hiveProperty) {
-        return checkParameter(hiveProperty, HIVE_BLOCK_LIST);
-    }
-
-    private static String checkParameter(String commandParameter, String rex) {
-        String repaired = commandParameter.replaceAll(rex, "");
-        if (repaired.length() != commandParameter.length()) {
-            logger.warn("Detected illegal character in command {} by {} , replace it to {}.", commandParameter, rex, repaired);
-        }
-        return repaired;
-    }
 }
diff --git a/core-common/src/main/java/org/apache/kylin/common/util/ParameterFilter.java b/core-common/src/main/java/org/apache/kylin/common/util/ParameterFilter.java
@@ -0,0 +1,81 @@
+/*
+ * 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.common.util;
+
+import org.slf4j.LoggerFactory;
+import org.slf4j.Logger;
+
+public class ParameterFilter {
+    private static final Logger logger = LoggerFactory.getLogger(ParameterFilter.class);
+
+    public static final String PARAMETER_REGULAR_EXPRESSION = "[ &`>|{}()$;\\-#~!+*\\\\]+";
+    public static final String URI_REGULAR_EXPRESSION = "[^\\w%,@/:=?.\"\\[\\]]";
+    public static final String HIVE_PROPERTY_REGULAR_EXPRESSION = "[ <>()$;\\-#!+*\"'/=%@]+";
+    public static final String SPARK_CONF_REGULAR_EXPRESSION = "[`$|&;]+";
+
+    /**
+     * <pre>
+     * Check parameter for preventing command injection, replace illegal character into empty character.
+     *
+     * Note:
+     * 1. Whitespace is also refused because parameter is a single word, should not contains it
+     * 2. Some character may be illegal but still be accepted because commandParameter maybe a URI/path expression,
+     *     you may check "Character part" in https://docs.oracle.com/javase/8/docs/api/java/net/URI.html,
+     *     here is the character which is not banned.
+     *
+     *     1. dot .
+     *     2. slash /
+     *     3. colon :
+     *     4. equal =
+     *     5. ?
+     *     6. @
+     *     7. bracket []
+     *     8. comma ,
+     *     9. %
+     * </pre>
+     */
+    public static String checkParameter(String commandParameter) {
+        return checkParameter(commandParameter, PARAMETER_REGULAR_EXPRESSION, false);
+    }
+
+    public static String checkURI(String commandParameter) {
+        return checkParameter(commandParameter, URI_REGULAR_EXPRESSION, false);
+    }
+
+    public static String checkHiveProperty(String hiveProperty) {
+        return checkParameter(hiveProperty, HIVE_PROPERTY_REGULAR_EXPRESSION, false);
+    }
+
+    public static String checkSparkConf(String sparkConf) {
+        return checkParameter(sparkConf, SPARK_CONF_REGULAR_EXPRESSION, true);
+    }
+
+    private static String checkParameter(String commandParameter, String rex, boolean throwException) {
+        String repaired = commandParameter.replaceAll(rex, "");
+        if (repaired.length() != commandParameter.length()) {
+            if (throwException) {
+                throw new IllegalArgumentException("Detected illegal character in " + commandParameter + " by " + rex);
+            } else {
+                logger.warn("Detected illegal character in command {} by {} , replace it to {}.",
+                        commandParameter, rex, repaired);
+            }
+        }
+        return repaired;
+    }
+}
diff --git a/...n/common/util/CliCommandExecutorTest.java → ...ylin/common/util/ParameterFilterTest.java b/...n/common/util/CliCommandExecutorTest.java → ...ylin/common/util/ParameterFilterTest.java
@@ -17,11 +17,12 @@
  */
 package org.apache.kylin.common.util;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
 
-public class CliCommandExecutorTest {
+public class ParameterFilterTest {
 
     private String[][] commands = {
             {"nslookup unknown.com &", "nslookupunknown.com"},
@@ -41,24 +42,41 @@ public class CliCommandExecutorTest {
             {"db and 1=2", "dband12"}
     };
 
+    private String[] sparkConf = {"kylin.engine.spark-conf.'`touch /tmp/test`'", "'$(touch /tmp/test)'",
+    "'|touch /tmp/test|'", "';touch /tmp/test;'", "'&touch /tmp/test&'", "'$(|;&touch /tmp/test&;|)'", "default"};
+
     @Test
-    public void testCmd() {
+    public void testParameter() {
         for (String[] pair : commands) {
-            assertEquals(pair[1], CliCommandExecutor.checkParameter(pair[0]));
+            assertEquals(pair[1], ParameterFilter.checkParameter(pair[0]));
         }
     }
 
     @Test
-    public void testCmd2() {
+    public void testURI() {
         for (String[] pair : commands) {
-            assertEquals(pair[1], CliCommandExecutor.checkParameterWhiteList(pair[0]));
+            assertEquals(pair[1], ParameterFilter.checkURI(pair[0]));
         }
     }
 
     @Test
     public void testHiveProperties() {
         for (String[] pair : properties) {
-            assertEquals(pair[1], CliCommandExecutor.checkHiveProperty(pair[0]));
+            assertEquals(pair[1], ParameterFilter.checkHiveProperty(pair[0]));
+        }
+    }
+
+    @Test
+    public void testSparkConf() {
+        int exceptionNum = 0;
+        for(String conf : sparkConf) {
+            try {
+                ParameterFilter.checkSparkConf(conf);
+            } catch (Exception exception) {
+                Assert.assertTrue(exception instanceof IllegalArgumentException);
+                exceptionNum++;
+            }
         }
+        assertEquals(6, exceptionNum);
     }
 }
diff --git a/.../kylin-spark-engine/src/main/java/org/apache/kylin/engine/spark/job/NSparkExecutable.java b/.../kylin-spark-engine/src/main/java/org/apache/kylin/engine/spark/job/NSparkExecutable.java
@@ -27,13 +27,15 @@
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.Objects;
 import java.util.Map.Entry;
 
+import org.apache.kylin.common.util.ParameterFilter;
 import org.apache.kylin.cube.CubeInstance;
 import org.apache.kylin.cube.CubeManager;
 import org.apache.kylin.engine.spark.utils.MetaDumpUtil;
@@ -110,10 +112,20 @@ public String getDistMetaUrl() {
 
     @Override
     protected ExecuteResult doWork(ExecutableContext context) throws ExecuteException {
-        //context.setLogPath(getSparkDriverLogHdfsPath(context.getConfig()));
         CubeManager cubeMgr = CubeManager.getInstance(KylinConfig.getInstanceFromEnv());
         CubeInstance cube = cubeMgr.getCube(this.getCubeName());
         KylinConfig config = cube.getConfig();
+
+        Map<String, String> overrideKylinProps = new HashMap<>();
+        LinkedHashMap<String, String> cubeConfig = cube.getDescriptor().getOverrideKylinProps();
+        LinkedHashMap<String, String> projectConfig = cube.getProjectInstance().getOverrideKylinProps();
+        overrideKylinProps.putAll(projectConfig);
+        overrideKylinProps.putAll(cubeConfig);
+        for (Map.Entry<String, String> configEntry : overrideKylinProps.entrySet()) {
+            ParameterFilter.checkSparkConf(configEntry.getKey());
+            ParameterFilter.checkSparkConf(configEntry.getValue());
+        }
+
         this.setLogPath(getSparkDriverLogHdfsPath(context.getConfig()));
         config = wrapConfig(config);
 

diff --git a/server-base/src/main/java/org/apache/kylin/rest/controller/JobController.java b/server-base/src/main/java/org/apache/kylin/rest/controller/JobController.java
@@ -27,8 +27,8 @@
 import java.util.Map;
 import java.util.Locale;
 
-import org.apache.kylin.common.util.CliCommandExecutor;
 import org.apache.kylin.common.KylinConfig;
+import org.apache.kylin.common.util.ParameterFilter;
 import org.apache.kylin.job.JobInstance;
 import org.apache.kylin.job.constant.JobStatusEnum;
 import org.apache.kylin.job.constant.JobTimeFilterEnum;
@@ -195,8 +195,8 @@ public EnvelopeResponse<String> downloadLogFile(@PathVariable("job_id") String j
         checkRequiredArg("job_id", jobId);
         checkRequiredArg("step_id", stepId);
         checkRequiredArg("project", project);
-        String validatedPrj =  CliCommandExecutor.checkParameter(project);
-        String validatedStepId =  CliCommandExecutor.checkParameter(stepId);
+        String validatedPrj = ParameterFilter.checkParameter(project);
+        String validatedStepId =  ParameterFilter.checkParameter(stepId);
         String downloadFilename = String.format(Locale.ROOT, "%s_%s.log", validatedPrj, validatedStepId);
 
         String jobOutput = jobService.getAllJobStepOutput(jobId, stepId);

diff --git a/server-base/src/main/java/org/apache/kylin/rest/controller/ProjectController.java b/server-base/src/main/java/org/apache/kylin/rest/controller/ProjectController.java
@@ -89,7 +89,7 @@ public class ProjectController extends BasicController {
     @RequestMapping(value = "", method = { RequestMethod.GET }, produces = { "application/json" })
     @ResponseBody
     public List<ProjectInstance> getProjects(@RequestParam(value = "limit", required = false) Integer limit,
-            @RequestParam(value = "offset", required = false) Integer offset) {
+                                             @RequestParam(value = "offset", required = false) Integer offset) {
         return projectService.listProjects(limit, offset);
     }
 

diff --git a/server-base/src/main/java/org/apache/kylin/rest/service/CubeService.java b/server-base/src/main/java/org/apache/kylin/rest/service/CubeService.java
@@ -35,6 +35,7 @@
 import org.apache.kylin.common.persistence.RootPersistentEntity;
 import org.apache.kylin.common.util.CliCommandExecutor;
 import org.apache.kylin.common.util.Pair;
+import org.apache.kylin.common.util.ParameterFilter;
 import org.apache.kylin.cube.CubeInstance;
 import org.apache.kylin.cube.CubeManager;
 import org.apache.kylin.cube.CubeSegment;
@@ -1152,10 +1153,10 @@ public void migrateCube(CubeInstance cube, String projectName) {
         String cmd = String.format(Locale.ROOT,
                 stringBuilder,
                 KylinConfig.getKylinHome(),
-                CliCommandExecutor.checkParameterWhiteList(srcCfgUri),
-                CliCommandExecutor.checkParameterWhiteList(dstCfgUri),
+                ParameterFilter.checkURI(srcCfgUri),
+                ParameterFilter.checkURI(dstCfgUri),
                 cube.getName(),
-                CliCommandExecutor.checkParameterWhiteList(projectName),
+                ParameterFilter.checkParameter(projectName),
                 config.isAutoMigrateCubeCopyAcl(),
                 config.isAutoMigrateCubePurge());