Java: An experimental query for ignored hostname verification #6443

artem-smotrakov · 2021-08-08T09:42:52Z

The method HostnameVerifier.verify() checks that the hostname from the server's certificate matches the server hostname after an HTTPS connection is established. The method returns true if the hostname is acceptable and false otherwise. The contract of the method does not require it to throw an exception if the verification failed. Therefore, a caller has to check the result and drop the connection if the hostname verification failed.

I'd like to add an experimental query that checks whether the result of hostname verification is checked or not. Currently, the query requires en exception to be thrown based on the result of HostnameVerifier.verify(). This makes the query pretty strict. It may result in false positives when an application checks the result of hostname verification but reports a failure without throwing an exception right away. The requirement may be softened. For example, the query can only check that the returned value is evaluated in an if-block. Looking forward to your feedback.

The query detects CVE-2019-11777 in the Eclipse Paho Java client library version 1.2.0.

intrigus-lgtm · 2021-08-08T13:41:37Z

Wouldn't it make sense to filter out cases where the "ignored" value is returned from/inside another HostnameVerifier?
https://lgtm.com/query/1603409360070422690/

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

java/ql/src/experimental/Security/CWE/CWE-297/IgnoredHostnameVerification.ql

kevinbackhouse · 2021-08-26T16:27:49Z

@artem-smotrakov: there are currently quite a few false positives because the ignored predicate isn't working quite right. Here are some examples on alibaba/nacos: https://lgtm.com/query/8750717796235890424/

artem-smotrakov · 2021-11-07T16:39:27Z

I've addressed your comments @Marcono1234 . Thank you!

artem-smotrakov · 2021-11-07T16:43:31Z

Wouldn't it make sense to filter out cases where the "ignored" value is returned from/inside another HostnameVerifier? https://lgtm.com/query/1603409360070422690/

@intrigus-lgtm Makes sense to me. I've updated the query to ignore calls wrapped by another hostname verifier. Thank you for the suggestion!

artem-smotrakov · 2021-11-07T16:47:54Z

@artem-smotrakov: there are currently quite a few false positives because the ignored predicate isn't working quite right. Here are some examples on alibaba/nacos: https://lgtm.com/query/8750717796235890424/

@kevinbackhouse I've addressed @intrigus-lgtm's comment above, and that fixes the false-positive in SelfHostnameVerifier. I am not sure that the query should be updated to address the false-positives in tests. Please let me know what you think.

artem-smotrakov · 2021-11-07T16:48:22Z

Thank you all for the review, and sorry for the delay in addressing your comments!

kevinbackhouse · 2021-11-23T15:23:37Z

@artem-smotrakov: I'm sorry for not looking at this until now. The number of false positives has gone down, but I'm still seeing a lot of similar results to before. For example: https://lgtm.com/query/6043668221389765828/

The vast majority of the false positives involve Assert.assertTrue or Assert.assertFalse, so just fixing those would take care of about 90% of the false positives. The others are slightly more subtle, like these:

https://lgtm.com/query/6215280566352377624/
https://lgtm.com/query/1673043917935932127/

artem-smotrakov · 2022-01-09T22:29:16Z

@artem-smotrakov: I'm sorry for not looking at this until now.

@kevinbackhouse No problem. I am also replying pretty slow, unfurtunately.

The number of false positives has gone down, but I'm still seeing a lot of similar results to before. For example: https://lgtm.com/query/6043668221389765828/
The vast majority of the false positives involve Assert.assertTrue or Assert.assertFalse, so just fixing those would take care of about 90% of the false positives.

Those are alerts in tests. I suppose the database should have been built without the test code. Do you think it makes sense to make the query aware of testing/mocking frameworks?

The others are slightly more subtle, like these:
https://lgtm.com/query/6215280566352377624/

That is a good one. I've fixed it.

https://lgtm.com/query/1673043917935932127/

I am looking into this one.

- Added IgnoredHostnameVerification.ql - Added a qhelp file with examples - Added tests

artem-smotrakov · 2022-01-16T19:00:23Z

https://lgtm.com/query/1673043917935932127/

@kevinbackhouse I've fixed this one. I am still now sure that it is right to make the query aware of Assert.assertTrue and Assert.assertFalse. I think test code should not be included in the database.

kevinbackhouse · 2022-01-19T13:38:45Z

@artem-smotrakov: I'm afraid there's still a very large number of results, all of which look like false positives to me. I think it might be better to change this query so that it only warns when the verify() call is completely ignored. I.e. do not warn if the result is used in an expression, assigned to a variable, or passed as an argument to a method call. CVE-2019-11777 was caused by the result being completely ignored, so I think that would be the best pattern to look for.

artem-smotrakov · 2022-02-06T09:59:04Z

I'm afraid there's still a very large number of results, all of which look like false positives to me.
I think it might be better to change this query so that it only warns when the verify() call is completely ignored. I.e. do not warn if the result is used in an expression, assigned to a variable, or passed as an argument to a method call.

GHSA-63qc-p2x4-9fgf was caused by the result being completely ignored, so I think that would be the best pattern to look for.

Hi @kevinbackhouse Sure, we can trade off TPs for FPs. Let me see what I can do. If you could share those results that you think might be FPs, that would be great.

…Verifier.verify()

artem-smotrakov · 2022-02-06T11:27:14Z

@kevinbackhouse I've updated the query to check if the result of hostname verification is used in a general expression, if-statement and a method call. Technically, the result may be used in other statement like

while (!result) {
    throw new Exception("Failed");
}

but they look pretty exotic, so I am not sure if we want to cover them.

Please let me know what you think.

atorralba

I added some inline comments, but otherwise LGTM.

atorralba · 2022-02-10T15:58:48Z

java/ql/src/experimental/Security/CWE/CWE-297/CheckedHostnameVerification.java

@@ -0,0 +1,8 @@
+SSLSocket socket = (SSLSocket) SSLSocketFactory.getDefault().createSocket(host, port);
+socket.startHandshake();
+boolean successful = verifier.verify(host, socket.getSession());


I think it would make sense to clarify what verifier is. Like adding a line that at least makes clear it's a HostnameVerifier object. Likewise in IgnoredHostnameVerification.java.

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

atorralba · 2022-02-10T16:01:26Z

java/ql/src/experimental/Security/CWE/CWE-297/IgnoredHostnameVerification.ql

+
+/** The `HostnameVerifier.verify()` method. */
+private class HostnameVerifierVerifyMethod extends Method {
+  HostnameVerifierVerifyMethod() {
+    this.getDeclaringType().getASupertype*().hasQualifiedName("javax.net.ssl", "HostnameVerifier") and
+    this.hasStringSignature("verify(String, SSLSession)")
+  }
+}
+
+/** Defines `HostnameVerifier.verity()` calls that is not wrapped in another `HostnameVerifier`. */
+private class HostnameVerificationCall extends MethodAccess {
+  HostnameVerificationCall() {
+    this.getMethod() instanceof HostnameVerifierVerifyMethod and
+    not this.getCaller() instanceof HostnameVerifierVerifyMethod
+  }


Let's reuse already defined types.

Suggested change

/** The `HostnameVerifier.verify()` method. */

private class HostnameVerifierVerifyMethod extends Method {

HostnameVerifierVerifyMethod() {

this.getDeclaringType().getASupertype*().hasQualifiedName("javax.net.ssl", "HostnameVerifier") and

this.hasStringSignature("verify(String, SSLSession)")

}

}

/** Defines `HostnameVerifier.verity()` calls that is not wrapped in another `HostnameVerifier`. */

private class HostnameVerificationCall extends MethodAccess {

HostnameVerificationCall() {

this.getMethod() instanceof HostnameVerifierVerifyMethod and

not this.getCaller() instanceof HostnameVerifierVerifyMethod

}

import semmle.code.java.security.Encryption

/** Defines `HostnameVerifier.verity()` calls that is not wrapped in another `HostnameVerifier`. */

private class HostnameVerificationCall extends MethodAccess {

HostnameVerificationCall() {

this.getMethod() instanceof HostnameVerifierVerify and

not this.getCaller() instanceof HostnameVerifierVerify

}

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

java/ql/src/experimental/Security/CWE/CWE-297/IgnoredHostnameVerification.ql

smowton · 2022-02-10T17:49:30Z

java/ql/src/experimental/Security/CWE/CWE-297/IgnoredHostnameVerification.ql

+
+  /** Holds if the result of the call is not used. */
+  predicate isIgnored() {
+    not exists(Expr expr, IfStmt ifStmt, MethodAccess ma |


This is relying on the CodeQL optimiser not to compute the product of all Exprs, IfStmts and MethodAccesses because there are disjuncts where each of them are in scope but unused.

Instead, do something like

not exists(Expr e | this = e.getAChildExpr()) and not exists(IfStmt ifStmt | ...

However you can also determine if a method call is unused like this by checking whether it's the direct child of an ExprStmt--

this = any(ExprStmt es).getExpr()

All other kinds of use (assignment, passing to a method call, direct use in a conditional...) will be nested within some other statement or expression.

Thanks for the suggestions!

Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com> Co-authored-by: Chris Smowton <smowton@github.com>

artem-smotrakov · 2022-02-12T15:53:48Z

@atorralba @smowton Thanks for the review. I've applied your suggestions.

smowton · 2022-02-14T16:27:51Z

I've removed the redundant conditions from isIgnored (an expression that is the direct child of an ExprStmt can't be an if condition or be the child of any expression)

artem-smotrakov · 2022-02-14T18:54:23Z

I've removed the redundant conditions from isIgnored (an expression that is the direct child of an ExprStmt can't be an if condition or be the child of any expression)

Thanks!

artem-smotrakov requested a review from a team as a code owner August 8, 2021 09:42

github-actions bot added documentation Java labels Aug 8, 2021

artem-smotrakov mentioned this pull request Aug 8, 2021

Java: An experimental query for ignored hostname verification github/securitylab#419

Closed

1 task

Marcono1234 reviewed Aug 9, 2021

View reviewed changes

artem-smotrakov force-pushed the ignored-hostname-verifier branch from 5978475 to 60a0021 Compare November 7, 2021 16:36

artem-smotrakov added 6 commits January 16, 2022 18:25

Added a query for ignored hostname verification

e11cb94

- Added IgnoredHostnameVerification.ql - Added a qhelp file with examples - Added tests

Fixed a false-positive in CWE-297/IgnoredHostnameVerification.ql

f78002b

Added a query for ignored hostname verification

2b33265

- Added IgnoredHostnameVerification.ql - Added a qhelp file with examples - Added tests

Fixed typos in IgnoredHostnameVerification.qhelp

dcf251b

Ignore wrapped HostnameVerifier.vefify() calls

6dad0e2

Fixed another false-positive in CWE-297/IgnoredHostnameVerification.ql

825fe17

artem-smotrakov force-pushed the ignored-hostname-verifier branch from 84b5c47 to 825fe17 Compare January 16, 2022 18:56

Updated IgnoredHostnameVerification.ql to cover more uses of Hostname…

f53b2fc

…Verifier.verify()

smowton assigned atorralba Feb 9, 2022

atorralba reviewed Feb 10, 2022

View reviewed changes

smowton reviewed Feb 10, 2022

View reviewed changes

Apply suggestions from code review (typos/formatting)

0ba229a

Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com> Co-authored-by: Chris Smowton <smowton@github.com>

artem-smotrakov added 3 commits February 12, 2022 12:24

Clarify what verifier is

651e43d

Use classes from semmle.code.java.security.Encryption

36e565d

Better HostnameVerificationCall.isIgnored()

48604cd

Remove redundant conditions from HostnameVerificationCall.isIgnored

a62eae5

format

f2bc584

smowton approved these changes Feb 14, 2022

View reviewed changes

smowton merged commit fd4dc95 into github:main Feb 14, 2022

Java: An experimental query for ignored hostname verification #6443

Java: An experimental query for ignored hostname verification #6443

Uh oh!

Conversation

artem-smotrakov commented Aug 8, 2021

Uh oh!

intrigus-lgtm commented Aug 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinbackhouse commented Aug 26, 2021

Uh oh!

artem-smotrakov commented Nov 7, 2021

Uh oh!

artem-smotrakov commented Nov 7, 2021

Uh oh!

artem-smotrakov commented Nov 7, 2021

Uh oh!

artem-smotrakov commented Nov 7, 2021

Uh oh!

kevinbackhouse commented Nov 23, 2021

Uh oh!

artem-smotrakov commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artem-smotrakov commented Jan 16, 2022

Uh oh!

kevinbackhouse commented Jan 19, 2022

Uh oh!

artem-smotrakov commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artem-smotrakov commented Feb 6, 2022

Uh oh!

atorralba left a comment

Choose a reason for hiding this comment

Uh oh!

atorralba Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atorralba Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smowton Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

artem-smotrakov Feb 12, 2022

Choose a reason for hiding this comment

Uh oh!

artem-smotrakov commented Feb 12, 2022

Uh oh!

smowton commented Feb 14, 2022

Uh oh!

artem-smotrakov commented Feb 14, 2022

Uh oh!

Uh oh!

artem-smotrakov commented Jan 9, 2022 •

edited

Loading

artem-smotrakov commented Feb 6, 2022 •

edited

Loading