Skip to content

Java: CWE-552 Query to detect unsafe request dispatcher usage #7286

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

Merged
merged 17 commits into from
Jan 18, 2022

Conversation

luchua-bc
Copy link
Contributor

Directly incorporating user input into HTTP requests dispatched from the Java EE RequestDispatcher without proper validation of the input can allow any web application resource such as configuration files and source code to be disclosed.

As stated in the Java API doc, when using a Java EE RequestDispatcher, requests may be dispatched to any part of the web application bypassing both implicit (no direct access to WEB-INF or META-INF) and explicit (defined by the web application) security constraints. Unsanitized user provided data must not be used to construct the path passed to the RequestDispatcher as it is very likely to create a security vulnerability in the application.

This query detects unsafe invocations of RequestDispatcher with user controlled input. Important features include:

  • RequestDispatcher constructed from both HTTP request and servlet context
  • Path traversal check
  • Path encoding check
  • Check of path normalization using the java.nio.file.Path package

The query was previously submitted then closed to wait for the closing of another relevant PR. The current query provided further enhancement in addition to new scenarios and was validated against published CVEs.

Please consider to merge the PR. Thanks.

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.

Thanks for your submission @luchua-bc. I added some inline comments.

@atorralba
Copy link
Contributor

Hey @luchua-bc. I reviewed your changes and took the liberty of adjusting some things.

  • In cd9a485 I changed NullOrEmptyCheckSanitizer to be a BarrierGuard. Since you mentioned you couldn't make it work, I thought providing this example would help you. In any case, if this is only for a rare few edge cases, and considering that nullness checks are always a little problematic, I think the query would be simpler if we just removed this.
  • In 81feaae I removed NullOrEmptyCheckGuard, for the reasons mentioned above, and also made a significant refactor to PatchMatchSanitizer. Since this is a big change, I want to explain why I did it. Feel free to disregard (revert) this commit if you don't agree with it:
    • You were checking for uses of the ! operator, assuming that the use of the negation meant that the developer wanted to exclude some words (blocklist), and if it wasn't used then it was checking for safe paths (allowlist). This isn't accurate — a developer could just use the negation with a safe path to implement a guard clause, or use an unsafe path (like WEB-INF) without the negation for the same reason.
    • Instead of that, what I thought could work better was just checking the strings being compared: if they're known "bad" strings like "WEB-INF" or "..", we consider that the check is a blocklist (and thus we set branch = false in the BarrierGuard). Otherwise, we consider it an allowlist and set branch = true. That way you don't need to worry about whether the check is being negated or not, or any other potential complexity around boolean checks. Of course this has caveats, but as an heuristic I think it improves the previous approach a little.
    • Also, I got rid of StringOperationSanitizer, since in my opinion it was too broad and wasn't modeling any security protection, just an access to the string contents. Even if the string is read, its contents need to be checked in some way to be considered safe, and I think the current guards already do that job.
    • Note that I changed the test case doHead5 from GOOD to BAD. If I'm not mistaken, that case would still be exploitable via double URL encoding, since that would bypass the "WEB-INF" check and would get decoded by getRequestDispatcher. That also fits with the simplified approach of requiring decoding protections for blocklist checks and path traversal protections for allowlist checks.
  • Finally, in b6886b8 I just moved everything except the configuration to the QLL file, for convenience.

@luchua-bc
Copy link
Contributor Author

luchua-bc commented Jan 13, 2022

Thanks @atorralba a lot for the changes and the detailed explanation. I learned a new way to make the query more concise:-)

luchua-bc and others added 2 commits January 14, 2022 12:13
@atorralba
Copy link
Contributor

Thanks for your review @smowton.

@luchua-bc, I applied @smowton's comments regarding the rewrite I pushed. All the other suggestions are yours to handle :)

Add clarification comments to PathMatchGuard
@atorralba atorralba force-pushed the java/unsafe-url-forward-dispatch branch from 5a1dbd4 to fb1287d Compare January 14, 2022 14:28
@luchua-bc
Copy link
Contributor Author

Thanks @atorralba for those new commits. I've made changes for all the other suggestions.

@smowton - please let me know if more changes are needed. Thanks.

* Always sanitize after the second guard, not the first
* Only check basic-block dominance in one place
* One BarrierGuard extension per final guard
smowton
smowton previously approved these changes Jan 18, 2022
@luchua-bc
Copy link
Contributor Author

Thanks a lot for all the help from @atorralba and @smowton.

@smowton smowton merged commit 8409746 into github:main Jan 18, 2022
@luchua-bc luchua-bc deleted the java/unsafe-url-forward-dispatch branch January 19, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants