Java: Add/improve insecure trustmanager query#4879

intrigus-lgtm

This can be rebased once #4771 is merged.

smowton

Waiting for #4771 before reviewing this

kevinbackhouse

Hi @intrigus-lgtm. It looks like #4771 has been merged. Are you able to rebase this now?

intrigus-lgtm

@kevinbackhouse sure! But I don't have time right now, probably later this day.

intrigus-lgtm

@kevinbackhouse @smowton this has been rebased :)

smowton

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.

Suggested change

      
            An attack would look like this:
          
            An example attack:

This has not been applied.
Instead it has been changed to "An attack might look like this:".
Hope this is ok.

Suggested change

      
            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.

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.

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.

This is reimplementing the Configuration.isBarrierGuard concept

So what do you suggest going forward?

I'll see if I can fit this into a barrier-guard tomorrow.

intrigus-lgtm

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.

How strong do you feel about this?
If you want this changed, I guess this should change as well?
https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql#L109-L152

intrigus-lgtm

@smowton I applied most of your suggestions besides to suggestions in the .qhelp file.
Let me know your thoughts as this section is very similar to the InsecureHostnameVerification.qhelp file.

Suggestions on the barrier stuff are also unresolved, as it is not yet clear how we want to proceed.

smowton

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

smowton

There are also various unaddressed comments here. They don't all have to be applied, but please do respond to them

intrigus-lgtm

There are also various unaddressed comments here. They don't all have to be applied, but please do respond to them

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.

felicitymay

👋🏻 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 @github/docs-content-codeql team and a member of the team will be in touch.

intrigus-lgtm

Sorry for the long wait.

I've experimented a bit with the query.

A query without any filtering, run only on projects that were previously identified when run on all of lgtm.com.
557 of 557 hits.
A query with all filtering applied, that is, filtering based on source/sink enclosing callable name and filtering based on flag guards.
473 of 551 hits.
A query with partial filtering, that is, only filtering based on flag guards.
528 of 551 hits.
A query based on this idea with no filtering.
400 of 551 hits.
A query based on this idea with partial filtering, that is, only filtering based on flag guards.
387 of 551 hits.

With filtering on flag guards I mean code like this:

if(enableHttps) { 
  sink();
}

Re:

Nothing unexpected here.
It would be possible to further reduce the number of hits, by improving the filtering based on source/sink names.
This shows a small reduction of hits. It has the big advantage that I don't have to guess the intention of the developer.
In the case of source/sink filtering, if a method is named disableSSLSecurity, why is it named this way?
a) This method may be for external (=user controlled) consumption only. This should not be detected.
b) The dev wanted to create a re-usable method (for their code only) and always calls it. This should be detected.
But I'm pretty sure that it's not possible to differentiate between a) and b), because how would one determine whether something is for external consumption?
This shows a reduction, although there are probably sinks where the sink is not sslContext.getSocketFactory() but a call to another API that only receives the sslContext.
A small further reduction.

Links for the query runs have been privately shared with @smowton.

smowton

Which version do you want to submit for consideration for a bounty?

intrigus-lgtm

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.
Approach 2 can likely be tuned to filter most results that are likely FPs, but the downside would be that it may also filter TPs.
3 has more FPs by design but will likely not loose TPs.
4 & 5 don't really show a reduction of FPs. Just an overall reduction of results, I think.

smowton

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.

intrigus-lgtm

Rebased.

intrigus-lgtm requested review from a team and felicitymay as code owners December 24, 2020 23:12

github-actions Bot added documentation Java labels Dec 24, 2020

intrigus-lgtm mentioned this pull request Dec 24, 2020

[Java]: CWE 295 - Insecure TrustManager - MiTM github/securitylab#222

Closed

1 task

andripwn mentioned this pull request Jan 3, 2021

[Java]: CVE-2020-26234 - Insecure TrustManager - MiTM github/securitylab#224

Closed

1 task

smowton mentioned this pull request Jan 4, 2021

[Java]: CWE 295 - Insecure TrustManager - MiTM github/securitylab#221

Closed

1 task

smowton self-assigned this Jan 4, 2021

intrigus-lgtm mentioned this pull request Jan 4, 2021

Java: Add unsafe hostname verification query and remove existing overlapping query #4771

Merged

1 task

intrigus-lgtm dismissed a stale review via e5c748f January 15, 2021 16:34

intrigus-lgtm force-pushed the java/improve-trustmanager branch 2 times, most recently from e5c748f to 6c2475a Compare January 16, 2021 13:08

smowton reviewed Jan 18, 2021

View reviewed changes

intrigus-lgtm commented Jan 27, 2021

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.qhelp Outdated Show resolved Hide resolved

intrigus-lgtm commented Jan 27, 2021

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.qhelp Outdated Show resolved Hide resolved

smowton reviewed Feb 2, 2021

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.ql Outdated Show resolved Hide resolved

felicitymay removed their request for review February 22, 2021 08:38

intrigus and others added 23 commits June 25, 2021 16:47

Java: Fix Typos

d37d922

Java: Apply suggestions for QHelp

8a7f6b7

Java: Explicitly list custom flow steps.

6d09db6

Java: Fix compilation errors in test.

7023793

Java: Flag "intentionally" unsafe methods in tests. …

484533c

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.

Java: Expand tests.

6413af4

Java: Accept test changes.

281e085

Java: Switch from tabs to spaces.

0c1ce74

Java: Use this consistently in QL classes.

45cec3d

Java: Reduce long comment.

4a00670

Java: Apply suggestions from review.

6f217d3

Apply suggestions from code review …

51fdcf8

Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>

Java: Factor out SecurityFlag library.

dc0b06a

Java: Fix qhelp errors.

6bfdf8d

Java: Add change-note.

f0d4b1d

Apply suggestions from code review. …

f527df7

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Java: Move comments to separate lines. …

fe923fa

Move comments to separate lines to improve the rendering in the finished query help.

Move back to experimental.........

36575bb

Fix test location.

5106aec

Remove change-note.

be57aec

Apply suggestions from code review

a79356e

Accept test changes.

5aa711a

intrigus-lgtm dismissed smowton’s stale review via 5aa711a June 25, 2021 15:05

intrigus-lgtm force-pushed the java/improve-trustmanager branch from 50d9ea7 to 5aa711a Compare June 25, 2021 15:05

smowton approved these changes Jun 25, 2021

View reviewed changes

smowton merged commit def4a23 into github:main Jun 25, 2021

intrigus-lgtm mentioned this pull request Jun 28, 2021

Java: Promote Unsafe certificate trust query from experimental #6171

Merged

atorralba mentioned this pull request Nov 15, 2021

Java: Promote Insecure TrustManager from experimental #7136

Merged

+. Java calls the <code>checkServerTrusted</code> method to check whether it should trust the certificate.
+. The <code>checkServerTrusted</code> method of your <code>TrustManager</code> does not throw a <code>CertificateException</code>.
+. Java proceeds with the connection since your <code>TrustManager</code> implicitly trusted it by not throwing an exception.
+. 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.

+              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:

	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.

+              }
+              /** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */
+              private predicate isNodeGuardedByFlag(DataFlow::Node node) {

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Add/improve insecure trustmanager query#4879

Java: Add/improve insecure trustmanager query#4879smowton merged 28 commits intogithub:maingithub/codeql:mainfrom intrigus-lgtm:java/improve-trustmanagerintrigus-lgtm/ql:java/improve-trustmanagerCopy head branch name to clipboard

Conversation

intrigus-lgtm commented Dec 24, 2020

Uh oh!

smowton commented Jan 4, 2021

Uh oh!

kevinbackhouse commented Jan 14, 2021

Uh oh!

intrigus-lgtm commented Jan 14, 2021

Uh oh!

intrigus-lgtm commented Jan 16, 2021

Uh oh!

smowton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intrigus-lgtm commented Jan 27, 2021

Uh oh!

Uh oh!

Uh oh!

intrigus-lgtm commented Jan 27, 2021

Uh oh!

smowton commented Jan 28, 2021

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

intrigus-lgtm commented Feb 2, 2021

Uh oh!

felicitymay commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

intrigus-lgtm commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton commented Mar 25, 2021

Uh oh!

intrigus-lgtm commented Mar 25, 2021

Uh oh!

smowton commented Mar 25, 2021

Uh oh!

intrigus-lgtm commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Java: Add/improve insecure trustmanager query#4879
smowton merged 28 commits into
github:mainfrom
intrigus-lgtm:java/improve-trustmanager

smowton left a comment •
edited
Loading

felicitymay commented Feb 22, 2021 •
edited
Loading

intrigus-lgtm commented Mar 24, 2021 •
edited
Loading

intrigus-lgtm commented Jun 25, 2021 •
edited
Loading