Skip to content

Java: CWE-073 File path injection with the JFinal framework #7712

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Feb 16, 2022
Merged

Java: CWE-073 File path injection with the JFinal framework #7712

merged 11 commits into from
Feb 16, 2022

Conversation

luchua-bc
Copy link
Contributor

@luchua-bc luchua-bc commented Jan 23, 2022 •
edited
Loading

External Control of File Name or Path, also called File Path Injection, is a common attack and injection attack is listed as one of the top attacks in OWASP Top Ten 2021.

Loading files based on unvalidated user-input may cause file information disclosure and uploading files with unvalidated file types to an arbitrary directory may lead to Remote Command Execution (RCE).

JFinal is a widely used Web + ORM framework, which has 1.4K forks and 3.2k stars on GitHub. Multiple CWEs have been submitted for File Path Injection attack associated with this framework.

One sample CWE and its details can be found at Issue#115: ZrLog 2.2.2 Remote command execution vulnerability. Another one can be found at Issue #27: File reading.

This query detects unsafe file loading/downloading operations in code repositories that consume this framework. Please consider to merge this PR. Thanks.

@luchua-bc luchua-bc requested a review from a team as a code owner January 23, 2022 18:20
@luchua-bc luchua-bc mentioned this pull request Jan 23, 2022
Closed
2 tasks
@luchua-bc
Copy link
Contributor Author

Following up on the comment @smowton made with PR# 7286 that java.nio.file.Path normalization check and other path traversal checks are worth having in general, I've created a separate PathSanitizer library with the generic name Path Traversal (not unsafe URL forward) so that the library can be promoted as a shared lib that can be used by other queries as well.

atorralba
atorralba reviewed Feb 7, 2022
View reviewed changes
Copy link
Contributor

@atorralba atorralba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @luchua-bc, thanks for your contribution.

I added some inline comments but, in general, I think this query mostly overlaps with java/path-injection and they should probably be merged at some point (probably during promotion). So IMO the valuable contributions here are the JFinal sources and the PathSanitizer.qll library. The latter I didn't review in detail again since it was mostly taken from #7286.

@luchua-bc
Copy link
Contributor Author

Thanks @atorralba for reviewing this PR. I'll work on suggested changes.

The library PathSanitizer.qll does overlap with PR# 7286 and is actually taken from that PR then renamed. The rationale is that all queries requiring a path traversal check can use the shared library. Shall I move it to the directory experimental.semmle.code.java.security?

And I noticed one thing when testing this query. That is, we included indexOf and lastIndexOf since they are methods commonly used in partial string match, however, they are different from the boolean check contains, startsWith, matches, and regionMatches as they are typically compared with the value -1, e.g. path.indexOf("..") == -1 instead of !path.contains(".."). I tried to enhance the library by adding the integer comparison check but it doesn't seem to fit in the BarrierGuard that you and Chris helped to design. Would you please shed some light on this issue?

Once this PR is merged, I will create a separate PR to replace UnsafeUrlForward.qll in the unsafe URL forward/dispatcher query with the new one.

Thanks,
@luchua-bc

@atorralba
Copy link
Contributor

The library PathSanitizer.qll does overlap with PR# 7286 and is actually taken from that PR then renamed. The rationale is that all queries requiring a path traversal check can use the shared library. Shall I move it to the directory experimental.semmle.code.java.security?

And I noticed one thing when testing this query. That is, we included indexOf and lastIndexOf since they are methods commonly used in partial string match, however, they are different from the boolean check contains, startsWith, matches, and regionMatches as they are typically compared with the value -1, e.g. path.indexOf("..") == -1 instead of !path.contains(".."). I tried to enhance the library by adding the integer comparison check but it doesn't seem to fit in the BarrierGuard that you and Chris helped to design. Would you please shed some light on this issue?

I think it makes sense for that to be in the main library, but it probably should be in a different PR. That way, this PR would only contain the experimental query and would be easier to approve/merge. If you decide to do so, we can discuss the approach to supporting indexOf and lastIndexOf in the other PR.

@luchua-bc
Copy link
Contributor Author

The library PathSanitizer.qll does overlap with PR# 7286 and is actually taken from that PR then renamed. The rationale is that all queries requiring a path traversal check can use the shared library. Shall I move it to the directory experimental.semmle.code.java.security?
And I noticed one thing when testing this query. That is, we included indexOf and lastIndexOf since they are methods commonly used in partial string match, however, they are different from the boolean check contains, startsWith, matches, and regionMatches as they are typically compared with the value -1, e.g. path.indexOf("..") == -1 instead of !path.contains(".."). I tried to enhance the library by adding the integer comparison check but it doesn't seem to fit in the BarrierGuard that you and Chris helped to design. Would you please shed some light on this issue?

I think it makes sense for that to be in the main library, but it probably should be in a different PR. That way, this PR would only contain the experimental query and would be easier to approve/merge. If you decide to do so, we can discuss the approach to supporting indexOf and lastIndexOf in the other PR.

I like the idea. Let's complete this PR as an experimental query then I'll submit a new PR to the main directory that supports indexOf and lastIndexOf.

@luchua-bc
Copy link
Contributor Author

@atorralba - I've fixed an issue related to normalized paths in the latest commit to successfully detect the following test case:

codeql/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java

Lines 39 to 44 in ce03aeb

// GOOD: Upload file to user specified path with path normalization and validation
public void uploadFile2() throws IOException {
String savePath = getPara("dir");
File file = getFile("fileParam").getFile();
String finalFilePath = BASE_PATH + savePath;
Path path = Paths.get(finalFilePath).normalize();

As the method call is after the Paths.get(...) call, a regular sanitizer doesn't help therefore I made the new change.

atorralba
atorralba reviewed Feb 10, 2022
View reviewed changes
Copy link
Contributor

@atorralba atorralba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I like the approach of sanitizing any sink that flows to a normalize call. It could do so conditionally, or the normalized value could never be checked.

This is actually a shortcoming of the current java/path-injection query too, and it needs to be fixed — but I don't think this is the proper way to do it. I can't suggest a better alternative at the moment, though.

Anyway, I added a small suggestion if you decide to keep this.

@smowton
Copy link
Contributor

smowton commented Feb 15, 2022

Also you should use that shared lib you've created in your other experimental query. Suggest moving the shared code under https://github.com/github/codeql/tree/main/java/ql/src/experimental/semmle/code/java and using it in both queries.

@luchua-bc
Copy link
Contributor Author

luchua-bc commented Feb 15, 2022 •
edited
Loading

Thanks @smowton for reviewing this PR. As per your advice, I've moved PathSanitizer.qll to the shared lib directory. And I will submit a small PR to clean up the other query once this one is approved and merged.

@atorralba atorralba merged commit 111aabb into github:main Feb 16, 2022
@luchua-bc luchua-bc deleted the java/file-path-injection branch February 16, 2022 16:08
@luchua-bc luchua-bc mentioned this pull request Feb 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atorralba atorralba atorralba left review comments

@smowton smowton smowton approved these changes

Assignees
No one assigned
Labels
documentation Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luchua-bc @atorralba @smowton