[branch-2.1][fix](metadata)Add FE metadata-related file checks apache…

…#40546 (apache#41113) ## Proposed changes apache#40546
924060929 · Sep 23, 2024 · e1057ac · e1057ac
1 parent 51bb4e9
commit e1057ac
Show file tree

Hide file tree

Showing 3 changed files with 150 additions and 11 deletions.
diff --git a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java
@@ -2806,4 +2806,8 @@ public static boolean isNotCloudMode() {
     @ConfField(mutable = true, description = {"表示最大锁持有时间，超过该时间会打印告警日志，单位秒",
             "Maximum lock hold time; logs a warning if exceeded"})
     public static long  max_lock_hold_threshold_seconds = 10;
+
+    @ConfField(mutable = true, description = {"元数据同步是否开启安全模式",
+            "Is metadata synchronization enabled in safe mode"})
+    public static boolean meta_helper_security_mode = false;
 }
diff --git a/fe/fe-core/src/main/java/org/apache/doris/master/MetaHelper.java b/fe/fe-core/src/main/java/org/apache/doris/master/MetaHelper.java
@@ -18,31 +18,37 @@
 package org.apache.doris.master;
 
 import org.apache.doris.catalog.Env;
+import org.apache.doris.common.Config;
 import org.apache.doris.common.io.IOUtils;
 import org.apache.doris.common.util.HttpURLUtil;
 import org.apache.doris.httpv2.entity.ResponseBody;
 import org.apache.doris.httpv2.rest.manager.HttpUtils;
 import org.apache.doris.persist.gson.GsonUtils;
 
 import org.apache.commons.codec.digest.DigestUtils;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 
 import java.io.BufferedInputStream;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.net.HttpURLConnection;
+import java.util.Map;
 
 public class MetaHelper {
+    public static final Logger LOG = LogManager.getLogger(MetaHelper.class);
     private static final String PART_SUFFIX = ".part";
     public static final String X_IMAGE_SIZE = "X-Image-Size";
     public static final String X_IMAGE_MD5 = "X-Image-Md5";
     private static final int BUFFER_BYTES = 8 * 1024;
     private static final int CHECKPOINT_LIMIT_BYTES = 30 * 1024 * 1024;
+    private static final String VALID_FILENAME_REGEX = "^(?!\\.)[a-zA-Z0-9_\\-.]+$";
 
     public static File getMasterImageDir() {
         String metaDir = Env.getCurrentEnv().getImageDir();
@@ -53,35 +59,105 @@ public static int getLimit() {
         return CHECKPOINT_LIMIT_BYTES;
     }
 
+    private static void completeCheck(File dir, File file, File newFile) throws IOException {
+        if (!Config.meta_helper_security_mode) {
+            return;
+        }
+        String dirPath = dir.getCanonicalPath(); // Get the canonical path of the directory
+        String filePath = file.getCanonicalPath(); // Get the canonical path of the original file
+        String newFilePath = newFile.getCanonicalPath(); // Get the canonical path of the new file
+
+        // Ensure both file paths are within the specified directory to prevent path traversal attacks
+        if (!filePath.startsWith(dirPath) || !newFilePath.startsWith(dirPath)) {
+            throw new SecurityException("File path traversal attempt detected.");
+        }
+
+        // Ensure the original file exists and is a valid file to avoid renaming a non-existing file
+        if (!file.exists() || !file.isFile()) {
+            throw new IOException("Source file does not exist or is not a valid file.");
+        }
+
+    }
+
     // rename the .PART_SUFFIX file to filename
     public static File complete(String filename, File dir) throws IOException {
-        File file = new File(dir, filename + MetaHelper.PART_SUFFIX);
-        File newFile = new File(dir, filename);
+        // Validate that the filename does not contain illegal path elements
+        checkIsValidFileName(filename);
+
+        File file = new File(dir, filename + MetaHelper.PART_SUFFIX); // Original file with a specific suffix
+        File newFile = new File(dir, filename); // Target file without the suffix
+
+        completeCheck(dir, file, newFile);
+        // Attempt to rename the file. If it fails, throw an exception
         if (!file.renameTo(newFile)) {
-            throw new IOException("Complete file" + filename + " failed");
+            throw new IOException("Complete file " + filename + " failed");
         }
-        return newFile;
+
+        return newFile; // Return the newly renamed file
     }
 
-    public static OutputStream getOutputStream(String filename, File dir)
-            throws FileNotFoundException {
+    public static File getFile(String filename, File dir) throws IOException {
+        checkIsValidFileName(filename);
         File file = new File(dir, filename + MetaHelper.PART_SUFFIX);
-        return new FileOutputStream(file);
+        checkFile(dir, file);
+        return file;
+    }
+
+    private static void checkFile(File dir, File file) throws IOException {
+        if (!Config.meta_helper_security_mode) {
+            return;
+        }
+        String dirPath = dir.getCanonicalPath();
+        String filePath = file.getCanonicalPath();
+
+        if (!filePath.startsWith(dirPath)) {
+            throw new SecurityException("File path traversal attempt detected.");
+        }
+    }
+
+    protected static void checkIsValidFileName(String filename) {
+        if (!Config.meta_helper_security_mode) {
+            return;
+        }
+        if (StringUtils.isBlank(filename)) {
+            return;
+        }
+        if (!filename.matches(VALID_FILENAME_REGEX)) {
+            throw new IllegalArgumentException("Invalid filename : " + filename);
+        }
     }
 
-    public static File getFile(String filename, File dir) {
-        return new File(dir, filename + MetaHelper.PART_SUFFIX);
+    private static void checkFile(File file) throws IOException {
+        if (!Config.meta_helper_security_mode) {
+            return;
+        }
+        if (!file.getAbsolutePath().startsWith(file.getCanonicalFile().getParent())) {
+            throw new IllegalArgumentException("Invalid file path");
+        }
+
+        File parentDir = file.getParentFile();
+        if (!parentDir.canWrite()) {
+            throw new IOException("No write permission in directory: " + parentDir);
+        }
+
+        if (file.exists() && !file.delete()) {
+            throw new IOException("Failed to delete existing file: " + file);
+        }
+        checkIsValidFileName(file.getName());
     }
 
     public static <T> ResponseBody doGet(String url, int timeout, Class<T> clazz) throws IOException {
-        String response = HttpUtils.doGet(url, HttpURLUtil.getNodeIdentHeaders(), timeout);
+        Map<String, String> headers = HttpURLUtil.getNodeIdentHeaders();
+        LOG.info("meta helper, url: {}, timeout{}, headers: {}", url, timeout, headers);
+        String response = HttpUtils.doGet(url, headers, timeout);
         return parseResponse(response, clazz);
     }
 
     // download file from remote node
     public static void getRemoteFile(String urlStr, int timeout, File file)
             throws IOException {
         HttpURLConnection conn = null;
+        checkFile(file);
         OutputStream out = new FileOutputStream(file);
         try {
             conn = HttpURLUtil.getConnectionWithNodeIdent(urlStr);

diff --git a/fe/fe-core/src/test/java/org/apache/doris/master/MetaHelperTest.java b/fe/fe-core/src/test/java/org/apache/doris/master/MetaHelperTest.java
@@ -17,6 +17,7 @@
 
 package org.apache.doris.master;
 
+import org.apache.doris.common.Config;
 import org.apache.doris.httpv2.entity.ResponseBody;
 import org.apache.doris.httpv2.rest.RestApiStatusCode;
 import org.apache.doris.persist.StorageInfo;
@@ -25,6 +26,11 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.junit.Assert;
 import org.junit.Test;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+
+import java.io.File;
+import java.io.IOException;
 
 public class MetaHelperTest {
 
@@ -49,4 +55,57 @@ private ResponseBody<StorageInfo> buildResponseBody() {
         bodyBefore.setMsg("msg");
         return bodyBefore;
     }
+
+    File tempDir = new File(System.getProperty("java.io.tmpdir"), "tempDir");
+
+    @BeforeEach
+    void setUp() {
+
+        if (tempDir.exists()) {
+            tempDir.delete();
+        }
+        tempDir.mkdir();
+    }
+
+    @Test
+    public void testFile() throws IOException {
+
+        String errorFilename = "..testfile.";
+        File errorFileWithSuffix = new File(tempDir, errorFilename);
+        String rightFilename = "image.1";
+        File rightFileWithSuffix = new File(tempDir, rightFilename);
+
+        Config.meta_helper_security_mode = true;
+
+        if (errorFileWithSuffix.exists()) {
+            errorFileWithSuffix.delete();
+        }
+        Assert.assertThrows(Exception.class, () -> MetaHelper.complete(errorFilename, tempDir));
+        Assert.assertThrows(Exception.class, () -> MetaHelper.getFile(errorFilename, tempDir));
+        if (rightFileWithSuffix.exists()) {
+            rightFileWithSuffix.delete();
+        }
+        Assert.assertEquals(rightFileWithSuffix.getName() + ".part", MetaHelper.getFile(rightFilename, tempDir).getName());
+
+    }
+
+    @Test
+    public void testFileNameCheck() {
+        Config.meta_helper_security_mode = true;
+        MetaHelper.checkIsValidFileName("VERSION");
+        MetaHelper.checkIsValidFileName("image.1");
+        MetaHelper.checkIsValidFileName("image.1.part");
+        MetaHelper.checkIsValidFileName("image.1.part.1");
+        Assert.assertThrows(IllegalArgumentException.class, () -> MetaHelper.checkIsValidFileName("../testfile."));
+
+
+    }
+
+    @AfterEach
+    public void tearDown() {
+        if (tempDir.exists()) {
+            tempDir.delete();
+        }
+    }
+
 }