securitylab: [Java]: CWE-552 Add sources and sinks to detect unsafe getResource calls in Java EE applications

Query PR

https://github.com/github/codeql/pull/8706

Language

Java

CVE(s) ID list

CWE

CWE-552: Files or Directories Accessible to External Parties

Report

What is the vulnerability? 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.
How does the vulnerability work? 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.
What strategy do you use in your query to find the vulnerability? This query detects unsafe usage of getResource or getResourceAsStream from both servlet context and Java Server Faces context, which covers all typical Java EE applications.
How have you reduced the number of false positives? 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

Other information? Please refer to test cases and the sample program for more information.

Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc).

Blog post link

No response

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 26 (8 by maintainers)

Most upvoted comments

Thanks @jorgectf for the update. I’ve checked the third category as per our discussion above.

Yes, I want the submission to continue the process. Please move the submission to the next phase so that we can further tune the query then.

@luchua-bc

luchua-bc on Apr 25, 2022

Thank @jorgectf for the prompt update. I’ve also committed several new test cases. The test case doGetGood3 confirms that the query shall handle the second category of compared against a specific string already. Let’s see how the FP fixes behave:-)

luchua-bc on Apr 19, 2022

I will add the new sources and sinks later today then let you know.

luchua-bc on Apr 11, 2022

Thanks @jorgectf for the advice. Please let me know if the new test run discovers more issues then I will try to enhance the query.

@luchua-bc

luchua-bc on Apr 20, 2022

Hi @jorgectf,

For FPs of the first category used in a conditional side only to check whether the file exists, I think the root cause it that only getResource() calls whose retrieved contents are returned to end users shall be taken into account. This helps to address issues of this category and other similar ones. Therefore I changed getResource() from sink to taint step. This helps to remove FPs of all the four projects together with the project of the second category compared against a specific string. Or an enhancement to the equals() check can be made in the PathSanitizer.qll library.

For the third category of adding a suffix, I will check whether code in #4530 can be reused in this query.

And for the last category of compared its extension against an allowlist, I’m wondering whether there is a generalized way to detect FPs. For example, if txt and xml are in allowed extensions, configuration files can still be disclosed.

Please review the changes and let me know if further enhancements are desired for the first category.

Thanks, @luchua-bc

luchua-bc on Apr 19, 2022

Thanks @jorgectf for the results. I will try to address most or all of these patterns to reduce FPs and keep you posted.

@luchua-bc

luchua-bc on Apr 19, 2022

Hi @jorgectf,

I’ve updated the title of both the issue and the PR. Same to several other queries I submitted, they are all URL loading related therefore they share the same sanitizer library although they query/check issues in different areas.

Please let me know if further changes are desired.

Thanks, @luchua-bc

luchua-bc on Apr 11, 2022