Skip to content

[Java] CWE-552: Query to detect unsafe request dispatcher usage #495

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
Closed
1 task done
luchua-bc opened this issue Dec 2, 2021 · 15 comments
Closed
1 task done

[Java] CWE-552: Query to detect unsafe request dispatcher usage #495

luchua-bc opened this issue Dec 2, 2021 · 15 comments
Labels
All For One Submissions to the All for One, One for All bounty

Comments

@luchua-bc
Copy link

Query

Link to pull request with your CodeQL query:

Relevant PR: github/codeql#7286

CVE ID(s)

List the CVE ID(s) associated with this vulnerability. GitHub will automatically link CVE IDs to the GitHub Advisory Database.

Report

Describe the vulnerability. Provide any information you think will help GitHub assess the impact your query has on the open source community.

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
  • Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc). We would love to have you spread the word about the good work you are doing

Result(s)

Provide at least one useful result found by your query, on some revision of a real project.
@luchua-bc luchua-bc added the All For One Submissions to the All for One, One for All bounty label Dec 2, 2021
@luchua-bc luchua-bc mentioned this issue Dec 2, 2021
Closed
1 task
@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Test run.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Results analysis.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@m-y-mo
Copy link
Contributor

m-y-mo commented Dec 14, 2021

@luchua-bc Thank you for the submission, we've done an initial analysis of the query results. Overall the results are of good quality. However, we noticed that some results are FP due to the taint flow going into virtual methods like Map::get and Object::toString, which then resulted in the taint being propagated to every call of Map::get and Object::toString, many of which are from objects that are not tainted. While these are limitations of the standard CodeQL library that is known to us, it seems to affect this query in a disproportionate manner which results in a number of FPs. I'd suggest adding sanitizers to the query to either filter out taint steps that is inside the Object::toString and the Map::get method or to filter out steps that are the outputs of these methods. Please let us know if you think this is reasonable or if you have other suggestions. Thanks.

@luchua-bc
Copy link
Author

Thanks @m-y-mo for reviewing this PR. Could you share URLs of one or a few repositories with FPs mentioned above? This will help me to validate the query after making the changes.

@m-y-mo
Copy link
Contributor

m-y-mo commented Dec 14, 2021

For example, try running this query on Apache Struts and you should see that the results are going through the toString method. After you changed the query, we'll also do another run and check the results to see if it addresses the issue. Thanks.

@luchua-bc
Copy link
Author

Hi @m-y-mo,

I've added a sanitizer for those virtual method calls as per your suggestion. Now the toString call with Apache Struts doesn't appear in query results. Although the project still reports ten matches, it's expected since Struts is a framework service while applications developed on it are supposed to provide real invocation and input validation.

Please check again and let me know if more changes are required.

Thanks,
@luchua-bc

@perovovao perovovao mentioned this issue Dec 15, 2021
Closed
1 task
@luchua-bc
Copy link
Author

luchua-bc commented Jan 6, 2022 •
edited
Loading

Hi @m-y-mo,

Have you got a chance to check again? Would you please provide an update? Thanks.

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Query review.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@m-y-mo
Copy link
Contributor

m-y-mo commented Jan 6, 2022

Thanks and sorry for the late reply. I had a look at the results and the problem with toString is fixed, although as you mentioned, there are some application specific FP that are hard to eliminate. I'm happy with the results and have moved the status to Query review, which will mostly be some feedback on the query and PR itself. Thanks.

@luchua-bc
Copy link
Author

Thanks @m-y-mo a lot for the prompt update.

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Final decision.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Pay.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@xcorail
Copy link
Contributor

xcorail commented Jan 19, 2022

Created Hackerone report 1454582 for bounty 362499 : [495] [Java] CWE-552: Query to detect unsafe request dispatcher usage

@xcorail xcorail closed this as completed Jan 19, 2022
@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Closed.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@luchua-bc
Copy link
Author

Thanks @xcorail for the bounty and the quick turn-around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
All For One Submissions to the All for One, One for All bounty
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@xcorail @m-y-mo @luchua-bc @ghsecuritylab