-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Java: CWE-552 Query to detect unsafe request dispatcher usage #7286
Conversation
There was a problem hiding this 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.
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
Outdated
Show resolved
Hide resolved
Hey @luchua-bc. I reviewed your changes and took the liberty of adjusting some things.
|
Thanks @atorralba a lot for the changes and the detailed explanation. I learned a new way to make the query more concise:-) |
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: Chris Smowton <smowton@github.com>
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
5a1dbd4
to
fb1287d
Compare
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. |
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java
Outdated
Show resolved
Hide resolved
* Always sanitize after the second guard, not the first * Only check basic-block dominance in one place * One BarrierGuard extension per final guard
Thanks a lot for all the help from @atorralba and @smowton. |
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:
java.nio.file.Path
packageThe 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.