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

Conversation

CalvinKirs
Copy link
Member

@CalvinKirs CalvinKirs commented Nov 26, 2024

Purpose:

To improve the security of audit log files, a new method checkAuditLogFileName has been added to validate the file name and path to ensure they meet security requirements. This method is designed to prevent invalid file names and path traversal attacks, ensuring that only files within the designated directory can be accessed.↳

Changes:

File Name Validation:

A regular expression check has been added to validate the file name: ^[a-zA-Z0-9._-]+$, restricting the file name to letters, numbers, dots, underscores, and hyphens.

If the file name contains invalid characters (e.g., spaces, path traversal characters), a SecurityException is thrown with the message “Invalid file name.”
Path Validation:

The file name is resolved into a normalized path, and it is checked to ensure that it is within the allowed directory.

The path is constructed using Paths.get(Config.audit_log_dir).resolve(logFile).normalize(). If the path does not start with the specified audit log directory (Config.audit_log_dir), indicating an attempt to access outside the permitted directory (e.g., a path traversal attack), a SecurityException is thrown with the message “Invalid file path: Access outside of permitted directory.”

Check List (For Author)

  • Test
    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
 calvinkirs@CalvinKirss-MBP fe % curl -u root: -I "http://127.0.0.1:8030/api/get_log_file?type=fe.audit.log&file=../LICENSE"                                 
HTTP/1.1 200 OK
Date: Tue, 26 Nov 2024 06:49:56 GMT
Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
file_infos: {"fe.audit.log":2480,"fe.audit.log.20241030-1":87297,"fe.audit.log.20241031-1":1250,"fe.audit.log.20241101-1":260067,"fe.audit.log.20241106-1":523614,"fe.audit.log.20241107-1":83146,"fe.audit.log.20241108-1":190639,"fe.audit.log.20241110-1":5071,"fe.audit.log.20241111-1":668553,"fe.audit.log.20241119-1":471175,"fe.audit.log.20241120-1":17077,"fe.audit.log.20241125-1":760146}
Content-Type: application/json
Transfer-Encoding: chunked

calvinkirs@CalvinKirss-MBP fe % curl -u root: -X GET "http://127.0.0.1:8030/api/get_log_file?type=fe.audit.log&file=audit_log_dir/../LICENSE"               
{"msg":"Internal Error","code":500,"data":"Invalid file name","count":0}%                                                                                                                                     calvinkirs@CalvinKirss-MBP fe % curl -u root: -X GET "http://127.0.0.1:8030/api/get_log_file?type=fe.audit.log&file=audit_log_dir/%2e%2e%2f%2e%2e%2fetc%2fpasswd"
{"msg":"Internal Error","code":500,"data":"Invalid file name","count":0}%   
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

## Purpose:

To improve the security of audit log files, a new method checkAuditLogFileName has been added to validate the file name and path to ensure they meet security requirements. This method is designed to prevent invalid file names and path traversal attacks, ensuring that only files within the designated directory can be accessed.↳

### Changes:

#### File Name Validation:

A regular expression check has been added to validate the file name: ^[a-zA-Z0-9._-]+$, restricting the file name to letters, numbers, dots, underscores, and hyphens.

 If the file name contains invalid characters (e.g., spaces, path traversal characters), a SecurityException is thrown with the message “Invalid file name.”
Path Validation:

The file name is resolved into a normalized path, and it is checked to ensure that it is within the allowed directory.

The path is constructed using Paths.get(Config.audit_log_dir).resolve(logFile).normalize().
If the path does not start with the specified audit log directory (Config.audit_log_dir), indicating an attempt to access outside the permitted directory (e.g., a path traversal attack), a SecurityException is thrown with the message “Invalid file path: Access outside of permitted directory.”
@doris-robot
Copy link

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@CalvinKirs
Copy link
Member Author

run buildall

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit test

@CalvinKirs
Copy link
Member Author

Please add unit test↳

done:)

@CalvinKirs
Copy link
Member Author

run buildall

@CalvinKirs
Copy link
Member Author

run buildall

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Dec 1, 2024
Copy link
Contributor

github-actions bot commented Dec 1, 2024

PR approved by at least one committer and no changes requested.

Copy link
Contributor

github-actions bot commented Dec 1, 2024

PR approved by anyone and no changes requested.

@CalvinKirs CalvinKirs merged commit c0b8478 into apache:master Dec 2, 2024
24 of 26 checks passed
@CalvinKirs CalvinKirs deleted the master-get-logfile branch December 2, 2024 01:59
github-actions bot pushed a commit that referenced this pull request Dec 2, 2024
## Purpose:

To improve the security of audit log files, a new method
checkAuditLogFileName has been added to validate the file name and path
to ensure they meet security requirements. This method is designed to
prevent invalid file names and path traversal attacks, ensuring that
only files within the designated directory can be accessed.↳

### Changes:

#### File Name Validation:

A regular expression check has been added to validate the file name:
^[a-zA-Z0-9._-]+$, restricting the file name to letters, numbers, dots,
underscores, and hyphens.

If the file name contains invalid characters (e.g., spaces, path
traversal characters), a SecurityException is thrown with the message
“Invalid file name.”
Path Validation:

The file name is resolved into a normalized path, and it is checked to
ensure that it is within the allowed directory.

The path is constructed using
Paths.get(Config.audit_log_dir).resolve(logFile).normalize(). If the
path does not start with the specified audit log directory
(Config.audit_log_dir), indicating an attempt to access outside the
permitted directory (e.g., a path traversal attack), a SecurityException
is thrown with the message “Invalid file path: Access outside of
permitted directory.”
github-actions bot pushed a commit that referenced this pull request Dec 2, 2024
## Purpose:

To improve the security of audit log files, a new method
checkAuditLogFileName has been added to validate the file name and path
to ensure they meet security requirements. This method is designed to
prevent invalid file names and path traversal attacks, ensuring that
only files within the designated directory can be accessed.↳

### Changes:

#### File Name Validation:

A regular expression check has been added to validate the file name:
^[a-zA-Z0-9._-]+$, restricting the file name to letters, numbers, dots,
underscores, and hyphens.

If the file name contains invalid characters (e.g., spaces, path
traversal characters), a SecurityException is thrown with the message
“Invalid file name.”
Path Validation:

The file name is resolved into a normalized path, and it is checked to
ensure that it is within the allowed directory.

The path is constructed using
Paths.get(Config.audit_log_dir).resolve(logFile).normalize(). If the
path does not start with the specified audit log directory
(Config.audit_log_dir), indicating an attempt to access outside the
permitted directory (e.g., a path traversal attack), a SecurityException
is thrown with the message “Invalid file path: Access outside of
permitted directory.”
yiguolei pushed a commit that referenced this pull request Dec 2, 2024
morningman pushed a commit that referenced this pull request Dec 7, 2024
CalvinKirs added a commit to CalvinKirs/incubator-doris that referenced this pull request Dec 26, 2024
…44612)

## Purpose:

To improve the security of audit log files, a new method
checkAuditLogFileName has been added to validate the file name and path
to ensure they meet security requirements. This method is designed to
prevent invalid file names and path traversal attacks, ensuring that
only files within the designated directory can be accessed.↳

### Changes:

#### File Name Validation:

A regular expression check has been added to validate the file name:
^[a-zA-Z0-9._-]+$, restricting the file name to letters, numbers, dots,
underscores, and hyphens.

If the file name contains invalid characters (e.g., spaces, path
traversal characters), a SecurityException is thrown with the message
“Invalid file name.”
Path Validation:

The file name is resolved into a normalized path, and it is checked to
ensure that it is within the allowed directory.

The path is constructed using
Paths.get(Config.audit_log_dir).resolve(logFile).normalize(). If the
path does not start with the specified audit log directory
(Config.audit_log_dir), indicating an attempt to access outside the
permitted directory (e.g., a path traversal attack), a SecurityException
is thrown with the message “Invalid file path: Access outside of
permitted directory.”

(cherry picked from commit c0b8478)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. dev/2.0.x dev/2.1.8-merged dev/3.0.4-merged reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants