Skip to content

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

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.

Sign up for GitHub

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

Merged
aschackmull merged 25 commits into from
Jan 13, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8df5d77
Java: Model `HostnameVerifier` method
intrigus Dec 4, 2020
3da1cb0
Java: Add unsafe hostname verification query
intrigus Dec 4, 2020
70b0703
Java: Remove overlapping code
intrigus Dec 4, 2020
0a9df07
Apply suggestions from review.
intrigus Dec 9, 2020
e021158
Java: Tighter model of `HostnameVerifier#verify`
intrigus Dec 9, 2020
d98b171
Java: Make `EnvTaintedMethod` public + QL-Doc
intrigus Dec 9, 2020
a62a2e5
Java: Improve QL-Doc
intrigus Dec 9, 2020
9e2ef9b
Java: Filter results by feature flags.
intrigus Dec 9, 2020
33b0ff2
Java: Update test
intrigus Dec 9, 2020
c88f07d
Java: Accept test output
intrigus Dec 9, 2020
10fc2cf
Apply suggestions from code review
intrigus-lgtm Dec 14, 2020
355cb6e
Fix Qhelp format
intrigus-lgtm Dec 16, 2020
502e4c3
Java: Fix Qhelp
intrigus Dec 17, 2020
b8f3e64
Apply suggestions from code review
intrigus-lgtm Jan 4, 2021
e11304a
Java: Autoformat
intrigus Jan 5, 2021
f4b912c
Apply suggestions from doc review
intrigus-lgtm Jan 9, 2021
b469273
Java: Add QLDoc improve query message
intrigus Jan 7, 2021
1eb2b75
Java: Further reduce FPs, simply Flag2Guard flow
intrigus Jan 10, 2021
5c1e746
Java: Rename to `EnvReadMethod`
intrigus Jan 11, 2021
4cfdb10
Java: Improve QLDoc & simplify code
intrigus-lgtm Jan 11, 2021
722bd4d
Java: Revise qhelp
intrigus-lgtm Jan 11, 2021
85286f3
Java: Replace global flow by local flow
intrigus Jan 11, 2021
4fa8f5e
Java: Accept test changes
intrigus Jan 12, 2021
1901f6b
Java: Make @id @name of query more similar.
intrigus Jan 12, 2021
2931e1f
Java: Add change note for #4771
intrigus Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Apply suggestions from code review 
Co-authored-by: Chris Smowton <smowton@github.com>
  • Loading branch information
@intrigus-lgtm @smowton @intrigus
2 people authored and intrigus committed Jan 11, 2021
commit 10fc2cf9f803acf2c4d5eb34071ad2b6a1e16e87
8 changes: 4 additions & 4 deletions java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ An attack would look like this:
5. Java wants to reject the certificate because the hostname does not match. Before doing this it checks whether there exists a <code>HostnameVerifier</code>.
6. Your <code>HostnameVerifier</code> is called which returns <code>true</code> for any certificate so also for this one.
7. Java proceeds with the connection since your <code>HostnameVerifier</code> accepted it.
8. The attacker can now read the data (Man-in-the-middle) your program sends to <code>https://example.com</code> while the program thinks the connection is secure.
8. 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.
</p>
Comment thread
intrigus-lgtm marked this conversation as resolved.
</overview>

<recommendation>
<p>
Do NOT use an unverifying <code>HostnameVerifier</code>!
<li>If you use an unverifying verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead.
Do not use an open <code>HostnameVerifier</code>.
<li>If you use an open verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead.
</li>
</p>
Comment thread
intrigus-lgtm marked this conversation as resolved.

Expand All @@ -42,4 +42,4 @@ In the second (good) example, the <code>HostnameVerifier</code> only returns <co
<li><a href="https://tersesystems.com/blog/2014/03/23/fixing-hostname-verification/">Further Information on Hostname Verification</a>.</li>
Comment thread
intrigus-lgtm marked this conversation as resolved.
Outdated
<li>OWASP: <a href="https://cwe.mitre.org/data/definitions/297.html">CWE-297</a>.</li>
Comment thread
intrigus-lgtm marked this conversation as resolved.
Outdated
</references>
</qhelp>
</qhelp>
4 changes: 2 additions & 2 deletions java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @name Disabled hostname verification
* @description Accepting any certificate as valid for a host allows an attacker to perform a man-in-the-middle attack.
* @description Accepting any certificate as valid for a host allows an attacker to perform a machine-in-the-middle attack.
Comment thread
intrigus-lgtm marked this conversation as resolved.
Outdated
* @kind path-problem
* @problem.severity error
* @precision high
Expand Down Expand Up @@ -29,7 +29,7 @@ private predicate alwaysReturnsTrue(HostnameVerifierVerify m) {
}

/**
* A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true` (ignoring exceptional flow), thus
* A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true` (though it could also exit due to an uncaught exception), thus
* accepting any certificate despite a hostname mismatch.
*/
class TrustAllHostnameVerifier extends RefType {
Expand Down