fix: check path permission & other CodeQL problems (#311)

问题1：用户输入正则表达式
之前的方式：

替换用户输入的路径中的特殊字符，CodeQL 仍然认为不安全。
现在的方式，重新构造一个字符串来防止用户注入预期外的正则表达式:
按照*进行分割
*替换为.*
其余字符全部使用字面匹配，即Pattern.quote
问题2：在 FileSystem 对接层中检查非法路径
在 FileSystemManager 中，进行读写之前，对路径进行过滤

问题3：CodeQL 报告Uncontrolled data used in path expression的假阳性问题
我们对路径进行了检查，但是 CodeQL 并不感知到我们的检查操作，所以在我们检查的同时，对 CodeQL 进行欺骗，来避免假阳性问题。

CodeQL 是基于污点分析来报告Uncontrolled data used in path expression问题的，所以我们只要设法阻断污点传播，就可以欺骗 CodeQL。

我尝试了使用 @Untainted 来阻断污点传播，但是 CodeQL 似乎并不支持该标注。
然后我使用了一个并不优雅的方式来欺骗 CodeQL。

欺骗 CodeQL
由于 CodeQL 认为下面的检查方式是安全的，记为 checkNode

if (node.contains("..")) {
  return Optional.empty();
}
所以可以把路径拆成若干个文件夹/文件名称，分别应用上述检查，然后再拼接为一个路径，来避免让 CodeQL 报告该问题。例如路径 /root/data/example.txt 被拆分为 root、data、example.txt，然后对这三个字符串分别使用checkNode进行检查，再将这三个字符串和根路径拼接为/root/data/example.txt。

由于欺骗操作是内置在路径过滤接口中的，所以这种欺骗是安全的。

core/src/main/java/cn/edu/tsinghua/iginx/IginxWorker.java

@@ -837,16 +837,19 @@ public Status registerTask(RegisterTaskReq req) {
 
     Predicate<String> ruleNameFilter = FilePermissionRuleNameFilters.transformerRulesWithDefault();
 
-    Predicate<Path> sourceChecker =
+    FilePermissionManager.Checker sourceChecker =
         FilePermissionManager.getInstance()
             .getChecker(null, ruleNameFilter, FileAccessType.EXECUTE);
 
-    File sourceFile = new File(filePath);
-    if (!sourceChecker.test(sourceFile.toPath())) {
+    Optional<Path> sourceCheckedPath = sourceChecker.normalize(filePath);
+
+    if (!sourceCheckedPath.isPresent()) {
       errorMsg = String.format("Register file %s has no execute permission", filePath);
       LOGGER.error(errorMsg);
       return RpcUtils.FAILURE.setMessage(errorMsg);
     }
+
+    File sourceFile = sourceCheckedPath.get().toFile();
     if (!sourceFile.exists()) {
       errorMsg = String.format("Register file not exist in declared path, path=%s", filePath);
       LOGGER.error(errorMsg);
@@ -890,19 +893,21 @@ public Status registerTask(RegisterTaskReq req) {
     String fileName = sourceFile.getName();
     String destPath =
         String.join(File.separator, config.getDefaultUDFDir(), "python_scripts", fileName);
-    File destFile = new File(destPath);
 
-    if (destFile.exists()) {
-      errorMsg = String.format("Register file(s) already exist, name=%s", fileName);
+    FilePermissionManager.Checker destChecker =
+        FilePermissionManager.getInstance().getChecker(null, ruleNameFilter, FileAccessType.WRITE);
+
+    Optional<Path> destCheckedPath = destChecker.normalize(destPath);
+    if (!destCheckedPath.isPresent()) {
+      errorMsg = String.format("Register file %s has no write permission", destPath);
       LOGGER.error(errorMsg);
       return RpcUtils.FAILURE.setMessage(errorMsg);
     }
 
-    Predicate<Path> destChecker =
-        FilePermissionManager.getInstance().getChecker(null, ruleNameFilter, FileAccessType.WRITE);
+    File destFile = destCheckedPath.get().toFile();
 
-    if (!destChecker.test(destFile.toPath())) {
-      errorMsg = String.format("Register file %s has no write permission", destPath);
+    if (destFile.exists()) {
+      errorMsg = String.format("Register file(s) already exist, name=%s", fileName);
       LOGGER.error(errorMsg);
       return RpcUtils.FAILURE.setMessage(errorMsg);
     }
@@ -984,25 +989,25 @@ public Status dropTask(DropTaskReq req) {
             + "python_scripts"
             + File.separator
             + transformTaskMeta.getFileName();
-    File file = new File(filePath);
-
-    if (!file.exists()) {
-      metaManager.dropTransformTask(name);
-      errorMsg = String.format("Register file not exist, path=%s", filePath);
-      LOGGER.error(errorMsg);
-      return RpcUtils.FAILURE.setMessage(errorMsg);
-    }
 
-    String pythonDir = config.getDefaultUDFDir() + File.separator + "python_scripts";
     Predicate<String> ruleNameFilter = FilePermissionRuleNameFilters.transformerRulesWithDefault();
-    Predicate<Path> destChecker =
+    FilePermissionManager.Checker destChecker =
         FilePermissionManager.getInstance().getChecker(null, ruleNameFilter, FileAccessType.WRITE);
+    Optional<Path> normalizedFile = destChecker.normalize(filePath);
 
-    Path pythonDirPath = Paths.get(pythonDir);
-    if (!destChecker.test(pythonDirPath)) {
+    if (!normalizedFile.isPresent()) {
       errorMsg =
           String.format(
-              "User has no write permission in target directory, udf %s cannot be dropped.", name);
+              "User has no write permission in target directory, task %s cannot be dropped.", name);
+      LOGGER.error(errorMsg);
+      return RpcUtils.FAILURE.setMessage(errorMsg);
+    }
+
+    File file = normalizedFile.get().toFile();
+
+    if (!file.exists()) {
+      metaManager.dropTransformTask(name);
+      errorMsg = String.format("Register file not exist, path=%s", filePath);
       LOGGER.error(errorMsg);
       return RpcUtils.FAILURE.setMessage(errorMsg);
     }

core/src/main/java/cn/edu/tsinghua/iginx/auth/FilePermissionManager.java

@@ -4,6 +4,7 @@
 import cn.edu.tsinghua.iginx.conf.FilePermissionConfig;
 import cn.edu.tsinghua.iginx.conf.entity.FilePermissionDescriptor;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.*;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Predicate;
@@ -32,7 +33,57 @@ public static FilePermissionManager getInstance() {
     filePermissionConfig.onReload(this::reload);
   }
 
-  public Predicate<Path> getChecker(
+  public interface Checker {
+    boolean test(Path path);
+
+    default Optional<Path> normalize(String path) {
+      Optional<Path> p = cheatNormalize(Paths.get(path));
+      if (!p.isPresent()) {
+        return Optional.empty();
+      }
+      Path cheated = p.get();
+      if (!test(cheated)) {
+        return Optional.empty();
+      }
+      return Optional.of(cheated);
+    }
+
+    default boolean testRelativePath(String path) {
+      return !path.contains("..");
+    }
+
+    /**
+     * cheat CodeQL to not complain about path traversal vulnerability. This method should not be
+     * used except you know what you are doing.
+     *
+     * @param path the path to normalize
+     * @return the normalized path if it is safe, otherwise empty
+     */
+    @Deprecated
+    default Optional<Path> cheatNormalize(Path path) {
+      Path p = path.toAbsolutePath();
+      // split path nodes
+      String root = p.getRoot().toString();
+      String[] nodes = new String[p.getNameCount()];
+      for (int i = 0; i < p.getNameCount(); i++) {
+        nodes[i] = p.getName(i).toString();
+      }
+      // check if any node contains "."
+      if (!testRelativePath(root)) {
+        return Optional.empty();
+      }
+      for (String node : nodes) {
+        if (!testRelativePath(node)) {
+          return Optional.empty();
+        }
+      }
+      // rebuild path
+      Path rebuiltPath = Paths.get(root, nodes);
+      return Optional.of(rebuiltPath);
+    }
+  }
+
+  public Checker getChecker(
       @Nullable String user, Predicate<String> ruleNameFilter, FileAccessType... type) {
     return path -> {
       UserRules userRules = rules.get();

core/src/main/java/cn/edu/tsinghua/iginx/auth/utils/FilePermissionRuleNameFilters.java

@@ -23,4 +23,12 @@ public static Predicate<String> udfRules() {
   public static Predicate<String> udfRulesWithDefault() {
     return udfRules().or(defaultRules());
   }
+
+  public static Predicate<String> filesystemRules() {
+    return ruleName -> ruleName.startsWith("filesystem");
+  }
+
+  public static Predicate<String> filesystemRulesWithDefault() {
+    return filesystemRules().or(defaultRules());
+  }
 }

core/src/main/java/cn/edu/tsinghua/iginx/transform/data/FileAppendWriter.java

@@ -24,13 +24,26 @@ public class FileAppendWriter extends ExportWriter {
 
   private boolean hasWriteHeader;
 
-  public FileAppendWriter(String fileName) {
-    this.fileName = fileName;
+  public FileAppendWriter(String name) {
+    this.fileName = normalizeFileName(name).toString();
     this.hasWriteHeader = false;
     File file = new File(fileName);
     createFileIfNotExist(file);
   }
 
+  private Path normalizeFileName(String fileName) {
+    Predicate<String> ruleNameFilter = FilePermissionRuleNameFilters.transformerRulesWithDefault();
+
+    FilePermissionManager.Checker checker =
+        FilePermissionManager.getInstance().getChecker(null, ruleNameFilter, FileAccessType.WRITE);
+
+    return checker
+        .normalize(fileName)
+        .orElseThrow(
+            () ->
+                new SecurityException("transformer has no permission to write file: " + fileName));
+  }
+
   @Override
   public void write(BatchData batchData) {
     if (!hasWriteHeader) {
@@ -50,20 +63,13 @@ public void write(BatchData batchData) {
   }
 
   private void createFileIfNotExist(File file) {
-    Predicate<String> ruleNameFilter = FilePermissionRuleNameFilters.transformerRulesWithDefault();
-
-    Predicate<Path> pathChecker =
-        FilePermissionManager.getInstance().getChecker(null, ruleNameFilter, FileAccessType.WRITE);
-    if (!pathChecker.test(file.toPath())) {
-      throw new SecurityException(
-          ("transformer has no permission to write file: " + file.getAbsolutePath()));
-    }
     if (!file.exists()) {
       LOGGER.info("File not exists, create it...");
       // get and create parent dir
-      if (!file.getParentFile().exists()) {
-        System.out.println("Parent dir not exists, create it...");
-        file.getParentFile().mkdirs();
+      File normalizeParentFile = normalizeFileName(file.getParentFile().getPath()).toFile();
+      if (!normalizeParentFile.exists()) {
+        LOGGER.info("Parent dir not exists, create it...");
+        normalizeParentFile.mkdirs();
       }
       try {
         // create file

core/src/main/resources/file-permission.properties

@@ -75,10 +75,11 @@
 #  The rule name is used to distinguish different rules. Different modules
 #  determine the subset of rules used by filter. In details:
 #
-#    * the udf module uses rules starting with "udf"
-#    * the transform module uses rules starting with "transformer".
 #    * the default module uses rules starting with "default".
 #      + the rules of the default module is applied to all modules.
+#    * the udf module uses rules starting with "udf"
+#    * the transform module uses rules starting with "transformer".
+#    * the filesystem driver uses rules starting with "filesystem".
 #
 # Rule Application Order:
 #   1. For the same user, the rules are applied in the order of the configuration

core/src/test/java/cn/edu/tsinghua/iginx/auth/FilePermissionManagerTest.java

@@ -8,7 +8,7 @@
 import cn.edu.tsinghua.iginx.conf.FilePermissionConfig;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.function.Predicate;
+import java.util.Optional;
 import org.apache.commons.configuration2.ex.ConfigurationException;
 import org.junit.Test;
 
@@ -31,22 +31,23 @@ public void testCheckNull() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
     try {
-      Predicate<Path> predicate = manager.getChecker(null, null, FileAccessType.READ);
+      FilePermissionManager.Checker predicate = manager.getChecker(null, null, FileAccessType.READ);
       predicate.test(Paths.get("test"));
       fail("Should throw NullPointerException");
     } catch (NullPointerException ignored) {
     }
 
     try {
-      Predicate<Path> predicate = manager.getChecker(null, defaultRules(), null);
+      FilePermissionManager.Checker predicate = manager.getChecker(null, defaultRules(), null);
       predicate.test(Paths.get("test"));
       fail("Should throw NullPointerException");
     } catch (NullPointerException ignored) {
     }
 
-    Predicate<Path> predicate = manager.getChecker(null, defaultRules(), FileAccessType.READ);
+    FilePermissionManager.Checker predicate =
+        manager.getChecker(null, defaultRules(), FileAccessType.READ);
     try {
-      predicate.test(null);
+      predicate.test((Path) null);
       fail("Should throw NullPointerException");
     } catch (NullPointerException ignored) {
     }
@@ -58,7 +59,7 @@ public void testCheckFallback() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
 
-    Predicate<Path> predicate =
+    FilePermissionManager.Checker predicate =
         manager.getChecker(null, udfRulesWithDefault(), FileAccessType.EXECUTE);
     assertTrue(predicate.test(Paths.get("udf.py")));
     assertFalse(predicate.test(Paths.get("udf.sh")));
@@ -70,7 +71,7 @@ public void testCheckDefaultModule() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
 
-    Predicate<Path> predicate =
+    FilePermissionManager.Checker predicate =
         manager.getChecker(null, udfRulesWithDefault(), FileAccessType.EXECUTE);
     assertTrue(predicate.test(Paths.get("test.bat")));
   }
@@ -81,11 +82,11 @@ public void testCheckDefaultUser() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
 
-    Predicate<Path> predicate =
+    FilePermissionManager.Checker predicate =
         manager.getChecker("root", udfRulesWithDefault(), FileAccessType.EXECUTE);
     assertTrue(predicate.test(Paths.get("test.sh")));
 
-    Predicate<Path> rootPredicate =
+    FilePermissionManager.Checker rootPredicate =
         manager.getChecker("test", udfRulesWithDefault(), FileAccessType.EXECUTE);
     assertFalse(rootPredicate.test(Paths.get("test.sh")));
   }
@@ -96,7 +97,7 @@ public void testCheckDefault() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
 
-    Predicate<Path> predicate =
+    FilePermissionManager.Checker predicate =
         manager.getChecker(null, udfRulesWithDefault(), FileAccessType.READ);
     assertTrue(predicate.test(Paths.get("test.py")));
   }
@@ -107,7 +108,7 @@ public void testCheckEmpty() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
 
-    Predicate<Path> predicate = manager.getChecker(null, udfRulesWithDefault());
+    FilePermissionManager.Checker predicate = manager.getChecker(null, udfRulesWithDefault());
     assertTrue(predicate.test(Paths.get("test.sh")));
   }
 
@@ -117,7 +118,7 @@ public void testCheckMultiple() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
 
-    Predicate<Path> predicate =
+    FilePermissionManager.Checker predicate =
         manager.getChecker(
             null, udfRulesWithDefault(), FileAccessType.EXECUTE, FileAccessType.WRITE);
     assertFalse(predicate.test(Paths.get("test.py")));
@@ -129,12 +130,21 @@ public void testCheckChinese() throws ConfigurationException {
     config.reload();
     FilePermissionManager manager = new FilePermissionManager(config);
 
-    Predicate<Path> predicate =
+    FilePermissionManager.Checker predicate =
         manager.getChecker(
             "测试用户",
             ruleName -> ruleName.equals("第一条规则"),
             FileAccessType.EXECUTE,
             FileAccessType.WRITE);
     assertFalse(predicate.test(Paths.get("不允许.py")));
   }
+
+  @Test
+  public void testNormalize() {
+    FilePermissionManager.Checker checker = path -> true;
+
+    Optional<Path> p = checker.normalize("test");
+    assertTrue(p.isPresent());
+    assertEquals(Paths.get("test").toAbsolutePath(), p.get());
+  }
 }