Skip to content

Java: CWE-552 Add sources and sinks to to detect unsafe getResource calls in Java EE applications #8706

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 8 commits into from
May 4, 2022
Merged

Java: CWE-552 Add sources and sinks to to detect unsafe getResource calls in Java EE applications #8706

merged 8 commits into from
May 4, 2022

Conversation

luchua-bc
Copy link
Contributor

Directly incorporating user input into the getResource call on the server side without proper validation of the input can allow any web application resource such as configuration files and source code to be disclosed.

It is very common that Java EE applications load requested resources and return file contents to clients. Constructing a server-side URI path with user input could allow an attacker to download application binaries
(including application classes or jar files) or view arbitrary files within protected directories.

This query detects unsafe usage of getResource or getResourceAsStream from both servlet context and Java Server Faces context. It utilizes the path sanitizer library to reduce FPs:

  • Path traversal check
  • Path encoding check
  • Check of path normalization using the java.nio.file.Path package

Please consider to merge the PR. Thanks.

@luchua-bc luchua-bc requested a review from a team as a code owner April 9, 2022 01:27
@luchua-bc luchua-bc mentioned this pull request Apr 9, 2022
Closed
2 tasks
@luchua-bc luchua-bc changed the title Java: CWE-552 Query to detect unsafe getResource calls in Java EE applications Java: CWE-552 Add sources and sinks to to detect unsafe getResource calls in Java EE applications Apr 11, 2022
atorralba
atorralba reviewed Apr 27, 2022
View reviewed changes
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

Hey @luchua-bc, thank you for your contribution. I added a few inline comments.

@atorralba atorralba added the no-change-note-required This PR does not need a change note label Apr 27, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 27, 2022
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 2 vulnerabilities.

@luchua-bc
Copy link
Contributor Author

Thanks @atorralba for reviewing this PR and your help with merging with main. I've made all requested changes. Please review again.

Thanks,
@luchua-bc

atorralba
atorralba reviewed Apr 28, 2022
View reviewed changes
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

A couple more comments.

@smowton
Copy link
Contributor

smowton commented Apr 29, 2022

The PR description reads like it's purely adding more models of sources and sinks, but the test expectation changes remove a result -- is that expected? Are you fixing an FP here at the same time?

@luchua-bc
Copy link
Contributor Author

Thanks @smowton for reviewing this PR.

I think you are talking about the following test result:

| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) | user-provided value |

Its relevant test case is no longer listed as a BAD case:

codeql/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java

Lines 103 to 112 in 811a2c0

// BAD: Request dispatcher with negation check and path normalization, but without URL decoding
protected void doHead5(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path");
Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) {
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
}
}

Changes in this PR seems to be irrelevant. I'm curious whether a change to a shared library somewhere caused the behaviour change. I will further investigate then report back.

@luchua-bc
Copy link
Contributor Author

Somehow the following taint step was removed during code merge. I've put it back and the issue is resolved.

"java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint"

And thanks for pointing out this issue.

@luchua-bc

smowton
smowton approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

LGTM; started tests running.

@atorralba atorralba merged commit b876431 into github:main May 4, 2022
@luchua-bc luchua-bc deleted the java/unsafe-get-resource branch May 5, 2022 02:14
@jcogs33 jcogs33 mentioned this pull request Mar 11, 2024
Merged
@jcogs33 jcogs33 mentioned this pull request Mar 27, 2024
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 no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

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