Skip to content

Java: CWE-552 Query to detect unsafe resource loading in Java Spring applications #9199

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 6 commits into from
Sep 27, 2022
Merged

Java: CWE-552 Query to detect unsafe resource loading in Java Spring applications #9199

merged 6 commits into from
Sep 27, 2022

Conversation

luchua-bc
Copy link
Contributor

Directly incorporating user input into loading resource calls 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 Spring 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 resource loading in Spring including the following APIs:

  • ClassPathResource
  • ResourceUtils
  • ResourceLoader and ApplicationContext

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 May 17, 2022 15:37
@luchua-bc luchua-bc mentioned this pull request May 17, 2022
Closed
2 tasks
atorralba
atorralba reviewed Jun 21, 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, thanks for your contribution. I added a couple comments that should be addressed if you're still interested in merging this PR regardless of bounty status.

@luchua-bc
Copy link
Contributor Author

Hi @atorralba,

I think this query will be beneficial so I'd like to have it merged. I've made requested changes.

@luchua-bc

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 22, 2022
View reviewed changes
java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll Outdated
Comment on lines 41 to 43
/**
* Resource loading method declared in Spring `Resource` with `getInputStream` inherited from the parent interface.
*/

Check warning

Code scanning / CodeQL

Class QLDoc style.

The QLDoc for a class should start with 'A', 'An', or 'The'.
java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll Outdated
Comment on lines 54 to 16
/**
* Resource loading method declared in Spring `ResourceUtils`.
*/

Check warning

Code scanning / CodeQL

Class QLDoc style.

The QLDoc for a class should start with 'A', 'An', or 'The'.
@pwntester
Copy link
Contributor

pwntester commented Jun 24, 2022 •
edited
Loading

Hi! after reviewing the LGTM run results, I spot a number of FPs caused by:

  • Non-classpath related InputStreamSources such as MultipartFile.getResource should not be considered as sinks.
    • sample project exhibiting these FPs: BanqiJane/Bilibili_Danmuji
  • HttpEntity.getBody. Not sure why this was reported or considered a sink in the first place but should be excluded
    • sample project exhibiting these FPs: RomanKonovalov/securityalarm
  • ResourceUtils.toURI not sure why this is considered a sink, if any, it should be considered a taint step, doesnt it?
    • sample project exhibiting these FPs: spring-projects/spring-framework
  • org.springframework.core.io.Resource interface should not be used, we should probably use its implementing ClasspathResource instead.
    • sample project exhibiting these FPs: HongZhaoHua/jstarcraft-core

@luchua-bc would you mind taking a look please?

@luchua-bc
Copy link
Contributor Author

Hi @pwntester,

As per your suggestion, I've removed Resource and InputStreamSource from the model to reduce FPs. FPs from those 4 mentioned repositories disappeared after the change.

@luchua-bc

@pwntester
Copy link
Contributor

Hi @pwntester,

As per your suggestion, I've removed Resource and InputStreamSource from the model to reduce FPs. FPs from those 4 mentioned repositories disappeared after the change.

@luchua-bc

Thanks a lot!

atorralba
atorralba reviewed Sep 23, 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 sorry that this stalled for so long. I think if we remove the commented code and address the QLDoc style warnings above, this should be ready to be merged.

@luchua-bc
Copy link
Contributor Author

Hi @atorralba,

No problem and I know you are busy.

Those text changes are pretty simple to make but it seems that there were other changes in that directory and in the main branch that impacts this query. When I run the query, I got some warnings like:

WARNING: Predicate isSanitizerGuard overrides only deprecated predicates, but is not itself deprecated (/github/vscode-codeql-starter/ql/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql:40,3-42,4)
WARNING: Type BarrierGuard has been deprecated and may be removed in future (/github/vscode-codeql-starter/ql/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql:40,39-61)
WARNING: Type HttpServletRequestGetRequestURIMethod has been deprecated and may be removed in future (/github/vscode-codeql-starter/ql/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql:28,22-59)
WARNING: Type PathTraversalBarrierGuard has been deprecated and may be removed in future (/github/vscode-codeql-starter/ql/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql:41,22-47)

And the expected query results don't match actual results. I think I need to synchronize with the main branch then add changes on top of it. I will further investigate and get it done before next Monday.

@luchua-bc

@luchua-bc luchua-bc force-pushed the java/unsafe-url-forward-dispatch-load branch from 8a5622a to 8effbff Compare September 23, 2022 12:45
@luchua-bc
Copy link
Contributor Author

Thank @atorralba for reviewing this PR. Actually a simple rebase FETCH_HEAD is all we need - there is no need for manually resolving conflicts.

I've committed all the requested changes. Please merge the PR.

Thanks,
@luchua-bc

@luchua-bc
Copy link
Contributor Author

Thank @atorralba for committing the change.

@atorralba atorralba merged commit be9509c into github:main Sep 27, 2022
@luchua-bc luchua-bc deleted the java/unsafe-url-forward-dispatch-load branch September 27, 2022 13:29
@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

@michaelnebel michaelnebel michaelnebel left review comments

@atorralba atorralba atorralba approved these changes

Assignees
No one assigned
Labels
documentation Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luchua-bc @pwntester @michaelnebel @atorralba