Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix](http)Enhanced Security Checks for Audit Log File Names #44612

Merged
merged 3 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Map;
import java.util.Set;
import javax.servlet.http.HttpServletRequest;
Expand All @@ -51,6 +53,23 @@
*/
@RestController
public class GetLogFileAction extends RestBaseController {
/**
* This method fetches internal logs via HTTP, which is no longer recommended and will
* be deprecated in future versions.
* <p>
* Using HTTP to fetch logs introduces serious security and performance issues:
* - **Security Risks**: Log content may expose sensitive information, allowing attackers to exploit the exposed
* HTTP endpoints.
* - **Performance Problems**: Frequent HTTP requests can cause significant system load, affecting the
* responsiveness and stability of the application.
* <p>
* It is strongly advised not to use this approach for accessing logs. Any new requirements should be
* handled using more secure, reliable, and efficient methods such as log aggregation tools (e.g., ELK, Splunk)
* or dedicated internal APIs.
* <p>
* **Note**: No new HTTP endpoints or types for log access will be accepted.
* Any further attempts to extend this HTTP-based log retrieval method will not be supported.
*/
private final Set<String> logFileTypes = Sets.newHashSet("fe.audit.log");

@RequestMapping(path = "/api/get_log_file", method = {RequestMethod.GET, RequestMethod.HEAD})
Expand Down Expand Up @@ -79,7 +98,13 @@ public Object execute(HttpServletRequest request, HttpServletResponse response)
String fileInfos = getFileInfos(logType);
response.setHeader("file_infos", fileInfos);
return ResponseEntityBuilder.ok();
} else if (method.equals(RequestMethod.GET.name())) {
}
if (method.equals(RequestMethod.GET.name())) {
try {
checkAuditLogFileName(logFile);
} catch (SecurityException e) {
return ResponseEntityBuilder.internalError(e.getMessage());
}
File log = getLogFile(logType, logFile);
if (!log.exists() || !log.isFile()) {
return ResponseEntityBuilder.okWithCommonError("Log file not exist: " + log.getName());
Expand All @@ -97,6 +122,17 @@ public Object execute(HttpServletRequest request, HttpServletResponse response)
return ResponseEntityBuilder.ok();
}

private void checkAuditLogFileName(String logFile) {
if (!logFile.matches("^[a-zA-Z0-9._-]+$")) {
throw new SecurityException("Invalid file name");
}
Path normalizedPath = Paths.get(Config.audit_log_dir).resolve(logFile).normalize();
// check path is valid or not
if (!normalizedPath.startsWith(Config.audit_log_dir)) {
throw new SecurityException("Invalid file path: Access outside of permitted directory");
}
}

private String getFileInfos(String logType) {
Map<String, Long> fileInfos = Maps.newTreeMap();
if (logType.equals("fe.audit.log")) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// 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.doris.httpv2;

import org.apache.doris.common.Config;
import org.apache.doris.httpv2.rest.GetLogFileAction;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import java.io.File;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;

public class GetLogFileActionTest {

@TempDir
public File tempDir;

@BeforeAll
public static void before() {
File tempDir = new File("test/audit.log");
tempDir.mkdir();
Config.audit_log_dir = tempDir.getAbsolutePath();
}

@Test
public void testCheckAuditLogFileName() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {
//private method checkAuditLogFileName
GetLogFileAction action = new GetLogFileAction();
Method method = GetLogFileAction.class.getDeclaredMethod("checkAuditLogFileName", String.class);
method.setAccessible(true);
method.invoke(action, "audit.log");
method.invoke(action, "fe.audit.log.20241104-1");
Assertions.assertThrows(InvocationTargetException.class, () -> method.invoke(action, "../etc/passwd"));
Assertions.assertThrows(InvocationTargetException.class, () -> method.invoke(action,
"fe.audit.log.20241104-1/../../etc/passwd"));
Assertions.assertThrows(InvocationTargetException.class,
() -> method.invoke(action, "fe.audit.log.20241104-1; rm -rf /"));


}
}
Loading