-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Java: Promote Insecure TrustManager from experimental #7136
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.
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 { |
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.
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?
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.
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 { |
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.
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)
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.
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 TrustManager
s, 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?
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.
I did this in 967308f.
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.
@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! 🙌 |
Fixed typos and carried out and editorial review
6bc6015
to
967308f
Compare
Force-pushed to rebase main and move the change note to the new location. |
PR to promote the Insecure TrustManager query created in #4879.
Changes
InlineFlowTest
.Evaluation
It was verified that the query still detects the following CVEs: