From e1057ac26dc9a84aa1a0c2c6bc5bc67873cc90ba Mon Sep 17 00:00:00 2001 From: Calvin Kirs Date: Mon, 23 Sep 2024 17:13:35 +0800 Subject: [PATCH] [branch-2.1][fix](metadata)Add FE metadata-related file checks #40546 (#41113) ## Proposed changes #40546 --- .../java/org/apache/doris/common/Config.java | 4 + .../org/apache/doris/master/MetaHelper.java | 98 ++++++++++++++++--- .../apache/doris/master/MetaHelperTest.java | 59 +++++++++++ 3 files changed, 150 insertions(+), 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 index 3bf55f3da4044b..c9a83d93146bf2 100644 --- 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 index fe516b02bfd360..e95a7a1cebc98b 100644 --- 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,6 +18,7 @@ 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; @@ -25,24 +26,29 @@ 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,28 +59,97 @@ 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 ResponseBody doGet(String url, int timeout, Class clazz) throws IOException { - String response = HttpUtils.doGet(url, HttpURLUtil.getNodeIdentHeaders(), timeout); + Map headers = HttpURLUtil.getNodeIdentHeaders(); + LOG.info("meta helper, url: {}, timeout{}, headers: {}", url, timeout, headers); + String response = HttpUtils.doGet(url, headers, timeout); return parseResponse(response, clazz); } @@ -82,6 +157,7 @@ public static ResponseBody doGet(String url, int timeout, Class clazz) th 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 index 070979494bfd6c..1c8c8a2a7ddf70 100644 --- 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 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(); + } + } + }