Skip to content

Java: Promote Insecure TrustManager from experimental #7136

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

Conversation

atorralba
Copy link
Contributor

@atorralba atorralba commented Nov 15, 2021

PR to promote the Insecure TrustManager query created in #4879.

Changes

  • Existing files were moved out of experimental.
  • Refactored into different libraries.
  • Refactored tests to use InlineFlowTest.

Evaluation

It was verified that the query still detects the following CVEs:

Copy link
Contributor

@intrigus-lgtm intrigus-lgtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to see this getting promoted :)


/**
* An insecure `X509TrustManager`.
* An `X509TrustManager` is considered insecure if it never throws a `CertificateException`
* and therefore implicitly trusts any certificate as valid.
*/
class InsecureX509TrustManager extends RefType {
private class InsecureX509TrustManager extends RefType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting here, because I can not comment down there, because the code did not change.
When I coded this, there was no good place for CertificateException (Line 37/61), maybe there is now a better place or common library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the moment, this is something that is only used here, and it doesn't seem to fit in any of the currently existing libraries. Nonetheless, this is definitely something to keep in mind if, in the future, CertificateException needs to be reused, or more classes from java.security.cert are modeled. Thanks!

* A configuration to model the flow of an insecure `TrustManager`
* to the initialization of an SSL context.
*/
class InsecureTrustManagerConfiguration extends TaintTracking::Configuration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be changed to DataFlow? I've used taint tracking due to limitations of data flow at the time.
See this comment from @aschackmull:
#4879 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DataFlow could be used, but then we would need to allow Array implicit reads in the sinks (and potentially in the additional flow steps). Something like:

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::Content c) {
  (this.isSink(node) or this.isAdditionalFlowStep(node, _)) and
  node.getType() instanceof Array and
  c instanceof DataFlow::ArrayContent
}

(which is actually part of the default implicit reads in TaintTracking).

We could even restrict node to be an array of TrustManagers, since that's what the sink is expecting.

Of course, the trade-off would be that, if for some reason the insecure TrustManager temporary goes through more complex data structures, e.g. a Map, we'd lose flows that a TaintTracking::Configuration would catch, but that doesn't sound like something that happens often.

@aschackmull what do you think is more appropriate in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this in 967308f.

@atorralba atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Nov 19, 2021
@mchammer01 mchammer01 self-requested a review November 22, 2021 11:22
mchammer01
mchammer01 previously approved these changes Nov 22, 2021
Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atorralba - LGTM ✨
Directly made a few improvements to the qhelp file as it hasn't been changed in this PR. For more details, see this commit.

@atorralba
Copy link
Contributor Author

@atorralba - LGTM ✨ Directly made a few improvements to the qhelp file as it hasn't been changed in this PR. For more details, see this commit.

Thank you so much @mchammer01! 🙌

@atorralba atorralba force-pushed the atorralba/promote-insecure-trustmanager branch from 6bc6015 to 967308f Compare January 20, 2022 09:25
@atorralba
Copy link
Contributor Author

Force-pushed to rebase main and move the change note to the new location.

@atorralba atorralba merged commit b59fd40 into github:main Jan 24, 2022
@atorralba atorralba deleted the atorralba/promote-insecure-trustmanager branch January 24, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants