Java: Add/improve insecure trustmanager query #4879
Conversation
|
Waiting for #4771 before reviewing this |
|
Hi @intrigus-lgtm. It looks like #4771 has been merged. Are you able to rebase this now? |
|
@kevinbackhouse sure! But I don't have time right now, probably later this day. |
intrigus-lgtm
force-pushed
the
java/improve-trustmanager
branch
2 times, most recently
from
e5c748f to
6c2475a
Compare
|
@kevinbackhouse @smowton this has been rebased :) |
There was a problem hiding this comment.
How about instead of implementing custom logic for barrier guards to pick out the various "if(acceptAllCertificates) ..." cases, we treat context.getSocketFactory as the sink and init(...) as a taint propagator? That way we'll pick out cases where the if(acceptAll) ... logic happens in a parent function, while the flow from instantiating a bad trust manager to calling context.init happens entirely within a child function. By lengthening the passage of code we consider as the source -> sink path, we should be able to use Configuraton.isBarrierGuard/isSanitizerGuard instead of implementing our own guard.controls logic.
| If the <code>checkServerTrusted</code> method of a <code>TrustManager</code> never throws a <code>CertificateException</code> it trusts every certificate. | ||
| This allows an attacker to perform a Man-in-the-middle attack against the application therefore breaking any security Transport Layer Security (TLS) gives. | ||
|
|
||
| An attack would look like this: |
There was a problem hiding this comment.
| An attack would look like this: | |
| An example attack: |
There was a problem hiding this comment.
This has not been applied.
Instead it has been changed to "An attack might look like this:".
Hope this is ok.
| 3. Java calls the <code>checkServerTrusted</code> method to check whether it should trust the certificate. | ||
| 4. The <code>checkServerTrusted</code> method of your <code>TrustManager</code> does not throw a <code>CertificateException</code>. | ||
| 5. Java proceeds with the connection since your <code>TrustManager</code> implicitly trusted it by not throwing an exception. | ||
| 6. The attacker can now read the data your program sends to <code>https://example.com</code> and/or alter its replies while the program thinks the connection is secure. |
There was a problem hiding this comment.
| 6. The attacker can now read the data your program sends to <code>https://example.com</code> and/or alter its replies while the program thinks the connection is secure. | |
| 6. The attacker can now read the data your program sends to <code>https://example.com</code> and/or alter its replies while the user might expect that the connection is secure. |
There was a problem hiding this comment.
Currently not applied as this is the same as https://github.com/intrigus-lgtm/ql/blob/2931e1f3fb4f68205c20db825d85d81f6782631b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp#L23-L24
If you still want to see it changed, I can apply it.
There was a problem hiding this comment.
I see, ok let's leave this the way it is for now and when I get a moment I'll see if that query can be improved.
| } | ||
|
|
||
| /** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */ | ||
| private predicate isNodeGuardedByFlag(DataFlow::Node node) { |
There was a problem hiding this comment.
This is reimplementing the Configuration.isBarrierGuard concept
There was a problem hiding this comment.
So what do you suggest going forward?
There was a problem hiding this comment.
I'll see if I can fit this into a barrier-guard tomorrow.
How strong do you feel about this? |
|
@smowton I applied most of your suggestions besides to suggestions in the Suggestions on the barrier stuff are also unresolved, as it is not yet clear how we want to proceed. |
|
OK, if you don't want to pursue that you don't have to; I'll start a benchmark run with the query as-is |
Will do, but I'm currently lacking a continuous chunk of time needed for evaluating the barrier guard stuff which could make some comments outdated. |
|
👋🏻 I've removed myself as a reviewer so that I don't get nagged about reviewing this PR every morning 😄 @github/codeql-java When this is ready for a docs team review, please ping the new |
|
Sorry for the long wait. I've experimented a bit with the query.
With filtering on flag guards I mean code like this: if(enableHttps) {
sink();
}Re:
Links for the query runs have been privately shared with @smowton. |
|
Which version do you want to submit for consideration for a bounty? |
The best one :D But I don't know what you would consider best. |
|
I don't have the time to assess 5 variants to decide which is best -- you should pick one. To assess which is doing a better job you could use the LGTM API to determine which projects differ between the different versions and take a closer look at those ones. Then you'll know whether version 4 or 5 is actually useful or whether your guess that they're just producing less results is correct. |
This removes the sanitizer part that classified some results as FP if the results were in methods with certain names, like `disableVerification()`. I now think that it's a bad idea to filter based on the method name. The custom flow steps in `flagFlowStep` are now listed explicitly. Simplified check whether a method throws an exception.
Previously intentionally unsafe methods such as `disableCertificate` would be ignored by this query. But now they will also be flagged as it is hard to guess intentions... Adjust the tests to account for this change.
Co-authored-by: Anders Schack-Mulligen <[email protected]>
Co-authored-by: Felicity Chapman <[email protected]>
Move comments to separate lines to improve the rendering in the finished query help.
intrigus-lgtm
force-pushed
the
java/improve-trustmanager
branch
from
50d9ea7 to
5aa711a
Compare
|
Rebased. |
This can be rebased once #4771 is merged.