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

intrigus-lgtm · 2020-12-02T20:02:50Z

This PR adds an improved query for hostname verification vulnerabilities to the supported query set.
The overlapping existing code in https://github.com/github/codeql/blob/main/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql is then removed, as it was easier for me to simply add a "new" query instead of duplicating/refactoring the existing query.

Todo:

Add change note.

smowton

Can you remind me of the context why this is being replicated like this? I probably meant to split or move code without changing its behaviour rather than to produce (transiently) 3 identical copies of the same query, but I can't remember the original discussion off hand.

java/ql/src/experimental/Security/CWE/CWE-297/MissingSslEndpointIdentification.qhelp

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql

java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp

java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java

intrigus-lgtm · 2020-12-09T22:35:15Z

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql

+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking2


Ignore these two imports, they are for a later commit.

intrigus-lgtm · 2020-12-09T22:53:07Z

java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java

+			@Override
+			public boolean verify(String hostname, SSLSession session) {
+				verify(hostname, session.getPeerCertificates());
+				return true; // GOOD [but detected as BAD]. The verification of the certificate is done in another method and


This kind of scenario is rather rare, of ~100 Alerts I've only seen this pattern once.

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql

intrigus-lgtm · 2020-12-14T14:52:24Z

I applied all suggestions.

smowton · 2020-12-14T14:55:35Z

Have requested a codeowner review

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp

felicitymay

@intrigus-lgtm - Apologies, my earlier suggestion to "fix" the query help was wrong.
I should have looked at the whole file and not jut the small part that the check reported.

Much of the syntax should be used as for HTML, so the <li> tags in the <recommendation> section need to be inside either <ol> or <ul> tags. However, it's not clear whether you really want a one sentence paragraph followed by a single bullet point for lines 23-27 so you might want to think about that.

You may find Query help files — CodeQL helpful. While the earlier lines weren't rejected by the PR check, I think that you're probably missing quite a bit of syntax here.

intrigus-lgtm · 2020-12-17T23:27:00Z

The <li> was an artifact of an older revision.
I've removed it and I think it should work now.

Is it possible for a user to generate the qhelp file themselves?
IIRC there were some changes in that area enabling the use for normal users?

This ignores results that are guarded by a feature flag that suggests an intentionally insecure feature. Inspired by Go's `InsecureFeatureFlag.qll` and `DisabledCertificateCheck.ql`.

Co-authored-by: Chris Smowton <smowton@github.com>

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

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

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp

felicitymay

@intrigus-lgtm - thanks for the most recent changes to the query help file for "Disabled hostname verification".

From the docs point of view, the text is ready to merge once CodeQL reviewers are happy.

One last comment that I'd make is that the filename, ID and @name for the query are all different and possibly it would be clearer for other users if they were more similar. I'm not sure how much effort it would be to rename the file and update the ID so that they were inline with the query @name.

intrigus-lgtm · 2021-01-12T11:17:14Z

@felicitymay what would you prefer?
Renaming @id and @name to java/unsafe-hostname-verification and Unsafe hostname verification?
That would be easier as I would not have to rename the files 🙂

aschackmull · 2021-01-12T14:10:35Z

The test output of ql/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref also needs fixing.

aschackmull · 2021-01-12T14:23:13Z

Renaming @id and @name to java/unsafe-hostname-verification and Unsafe hostname verification?

That sounds fine to me.

We're also at the point where this should get a change note (add a file to ql/java/change-notes with a name that includes todays date, cf. the other files in that dir). See e.g. ql/java/change-notes/2020-09-21-jhipster-gen-prng-query.md for an example of another change note that also describes the addition of a new query.

intrigus-lgtm · 2021-01-12T14:39:32Z

Added change note, accepted test output of ql/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref.

github-actions bot added documentation Java labels Dec 2, 2020

intrigus-lgtm force-pushed the split-cwe-295 branch from 203ac23 to 5c2adb1 Compare December 2, 2020 20:49

intrigus-lgtm changed the title ~~Duplicate UnsafeCert query so it can be split.~~ Triplicate UnsafeCert query so it can be split. Dec 2, 2020

intrigus-lgtm changed the title ~~Triplicate UnsafeCert query so it can be split.~~ [Java] Triplicate UnsafeCert query so it can be split. Dec 2, 2020

intrigus-lgtm marked this pull request as ready for review December 2, 2020 20:51

intrigus-lgtm requested a review from a team as a code owner December 2, 2020 20:51

smowton reviewed Dec 3, 2020

View reviewed changes

intrigus-lgtm force-pushed the split-cwe-295 branch 2 times, most recently from 0b5b5f1 to f064ea0 Compare December 8, 2020 20:10

intrigus-lgtm requested a review from felicitymay as a code owner December 8, 2020 20:10

intrigus-lgtm changed the title ~~[Java] Triplicate UnsafeCert query so it can be split.~~ Java: Add unsafe hostname verification query and remove existing overlapping query Dec 8, 2020

intrigus-lgtm requested review from smowton and removed request for a team December 8, 2020 20:19

smowton reviewed Dec 9, 2020

View reviewed changes

intrigus-lgtm commented Dec 9, 2020

View reviewed changes

intrigus-lgtm requested a review from smowton December 11, 2020 12:48

smowton reviewed Dec 14, 2020

View reviewed changes

intrigus-lgtm requested a review from smowton December 14, 2020 14:52

felicitymay reviewed Dec 16, 2020

View reviewed changes

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp Outdated Show resolved Hide resolved

intrigus-lgtm requested a review from felicitymay December 16, 2020 18:44

felicitymay reviewed Dec 16, 2020

View reviewed changes

intrigus-lgtm requested a review from felicitymay December 17, 2020 23:27

intrigus-lgtm mentioned this pull request Dec 24, 2020

Java: Add/improve insecure trustmanager query #4879

Merged

This was referenced Jan 4, 2021

[WIP] Java: Improve CWE 295 #4536

Closed

[Java] CWE-295 - Incorrect Hostname Verification #3581

Closed

intrigus and others added 14 commits January 11, 2021 13:42

Java: Make EnvTaintedMethod public + QL-Doc

d98b171

Java: Improve QL-Doc

a62a2e5

Java: Filter results by feature flags.

9e2ef9b

This ignores results that are guarded by a feature flag that suggests an intentionally insecure feature. Inspired by Go's `InsecureFeatureFlag.qll` and `DisabledCertificateCheck.ql`.

Java: Update test

33b0ff2

Java: Accept test output

c88f07d

Apply suggestions from code review

10fc2cf

Co-authored-by: Chris Smowton <smowton@github.com>

Fix Qhelp format

355cb6e

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

Java: Fix Qhelp

502e4c3

Apply suggestions from code review

b8f3e64

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

Java: Autoformat

e11304a

Apply suggestions from doc review

f4b912c

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

Java: Add QLDoc improve query message

b469273

Java: Further reduce FPs, simply Flag2Guard flow

1eb2b75

Java: Rename to EnvReadMethod

5c1e746

intrigus-lgtm force-pushed the split-cwe-295 branch from 2f6e9bc to 5c1e746 Compare January 11, 2021 12:43

Java: Improve QLDoc & simplify code

4cfdb10

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

intrigus-lgtm commented Jan 11, 2021

View reviewed changes

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp Outdated Show resolved Hide resolved

intrigus-lgtm and others added 2 commits January 11, 2021 18:57

Java: Revise qhelp

722bd4d

Java: Replace global flow by local flow

85286f3

felicitymay reviewed Jan 12, 2021

View reviewed changes

intrigus added 3 commits January 12, 2021 15:29

Java: Accept test changes

4fa8f5e

Java: Make @id @name of query more similar.

1901f6b

Java: Add change note for github#4771

2931e1f

intrigus-lgtm force-pushed the split-cwe-295 branch from 45a29d7 to 2931e1f Compare January 12, 2021 14:38

aschackmull approved these changes Jan 13, 2021

View reviewed changes

aschackmull merged commit 29935e1 into github:main Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

intrigus-lgtm commented Dec 2, 2020 •

edited

Loading

smowton left a comment

intrigus-lgtm Dec 9, 2020

intrigus-lgtm Dec 9, 2020

intrigus-lgtm commented Dec 14, 2020

smowton commented Dec 14, 2020

felicitymay left a comment

intrigus-lgtm commented Dec 17, 2020

felicitymay left a comment

intrigus-lgtm commented Jan 12, 2021

aschackmull commented Jan 12, 2021

aschackmull commented Jan 12, 2021

intrigus-lgtm commented Jan 12, 2021

		import semmle.code.java.dataflow.FlowSources
		import semmle.code.java.dataflow.TaintTracking2

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

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

Conversation

intrigus-lgtm commented Dec 2, 2020 • edited Loading

smowton left a comment

Choose a reason for hiding this comment

intrigus-lgtm Dec 9, 2020

Choose a reason for hiding this comment

intrigus-lgtm Dec 9, 2020

Choose a reason for hiding this comment

intrigus-lgtm commented Dec 14, 2020

smowton commented Dec 14, 2020

felicitymay left a comment

Choose a reason for hiding this comment

intrigus-lgtm commented Dec 17, 2020

felicitymay left a comment

Choose a reason for hiding this comment

intrigus-lgtm commented Jan 12, 2021

aschackmull commented Jan 12, 2021

aschackmull commented Jan 12, 2021

intrigus-lgtm commented Jan 12, 2021

intrigus-lgtm commented Dec 2, 2020 •

edited

Loading