Java: Promote Insecure TrustManager from experimental#7136
Java: Promote Insecure TrustManager from experimental#7136atorralba merged 7 commits intogithub:mainfrom
Conversation
intrigus-lgtm
left a comment
intrigus-lgtm
left a comment
There was a problem hiding this comment.
Happy to see this getting promoted :)
| * and therefore implicitly trusts any certificate as valid. | ||
| */ | ||
| class InsecureX509TrustManager extends RefType { | ||
| private class InsecureX509TrustManager extends RefType { |
There was a problem hiding this comment.
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.
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.
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.
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?
atorralba
added
the
ready-for-doc-review
mchammer01
left a comment
mchammer01
left a comment
There was a problem hiding this comment.
@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
atorralba
force-pushed
the
atorralba/promote-insecure-trustmanager
branch
from
6bc6015 to
967308f
Compare
|
Force-pushed to rebase main and move the change note to the new location. |
4 participants
PR to promote the Insecure TrustManager query created in #4879.
Changes
InlineFlowTest.Evaluation
It was verified that the query still detects the following CVEs: