This is an automated email from the ASF dual-hosted git repository.
morningman pushed a commit to branch branch-3.0
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-3.0 by this push:
new aa8515d4593 [fix](metadata)Add FE metadata-related file checks
(#40546) (#42170)
aa8515d4593 is described below
commit aa8515d4593cfe652b3dc1a1a05a35efc35b7af9
Author: Rayner Chen <[email protected]>
AuthorDate: Mon Oct 21 16:36:06 2024 +0800
[fix](metadata)Add FE metadata-related file checks (#40546) (#42170)
bp #40546
Co-authored-by: Calvin Kirs <[email protected]>
---
.../main/java/org/apache/doris/common/Config.java | 6 +-
.../java/org/apache/doris/master/MetaHelper.java | 93 +++++++++++++++++++---
.../org/apache/doris/master/MetaHelperTest.java | 47 +++++++++++
3 files changed, 135 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 be494505a31..62a6ebf1655 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
@@ -3129,5 +3129,9 @@ public class Config extends ConfigBase {
@ConfField(mutable = true, description = {"表示最大锁持有时间，超过该时间会打印告警日志，单位秒",
"Maximum lock hold time; logs a warning if exceeded"})
- public static long max_lock_hold_threshold_seconds = 10;
+ 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 fe516b02bfd..fff37472137 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;
@@ -30,7 +31,6 @@ 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;
@@ -43,6 +43,8 @@ public class MetaHelper {
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 =
"^image\\.\\d+(\\.part)?$";
+
public static File getMasterImageDir() {
String metaDir = Env.getCurrentEnv().getImageDir();
@@ -53,24 +55,89 @@ public class MetaHelper {
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.");
+ }
}
- public static File getFile(String filename, File dir) {
- return new File(dir, filename + MetaHelper.PART_SUFFIX);
+
+ private static void checkIsValidFileName(String filename) {
+ if (!Config.meta_helper_security_mode) {
+ return;
+ }
+ if (!filename.matches(VALID_FILENAME_REGEX)) {
+ throw new IllegalArgumentException("Invalid filename");
+ }
+ }
+
+ 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 {
@@ -82,6 +149,8 @@ public class MetaHelper {
public static void getRemoteFile(String urlStr, int timeout, File file)
throws IOException {
HttpURLConnection conn = null;
+ checkFile(file);
+ boolean md5Matched = true;
OutputStream out = new FileOutputStream(file);
try {
conn = HttpURLUtil.getConnectionWithNodeIdent(urlStr);
@@ -111,6 +180,7 @@ public class MetaHelper {
if (remoteMd5 != null) {
String localMd5 = DigestUtils.md5Hex(new
FileInputStream(file));
if (!remoteMd5.equals(localMd5)) {
+ md5Matched = false;
throw new IOException("Unexpected image md5, expected: " +
remoteMd5 + ", actual: " + localMd5);
}
}
@@ -121,6 +191,9 @@ public class MetaHelper {
if (out != null) {
out.close();
}
+ if (!md5Matched && file.exists() &
Config.meta_helper_security_mode) {
+ file.delete();
+ }
}
}
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 070979494bf..40083abf956 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.core.JsonProcessingException;
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,45 @@ public class MetaHelperTest {
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(IllegalArgumentException.class, () ->
MetaHelper.complete(errorFilename, tempDir));
+ Assert.assertThrows(IllegalArgumentException.class, () ->
MetaHelper.getFile(errorFilename, tempDir));
+ if (rightFileWithSuffix.exists()) {
+ rightFileWithSuffix.delete();
+ }
+ Assert.assertEquals(rightFileWithSuffix.getName() + ".part",
MetaHelper.getFile(rightFilename, tempDir).getName());
+
+ }
+
+ @AfterEach
+ public void tearDown() {
+ if (tempDir.exists()) {
+ tempDir.delete();
+ }
+ }
+
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
