Java: Add query for improper webview certificate validation #9663
Conversation
java/ql/lib/semmle/code/java/security/AndroidWebViewCertificateValidationQuery.qll
Outdated
Show resolved
Hide resolved
java/ql/lib/semmle/code/java/security/AndroidWebViewCertificateValidationQuery.qll
Outdated
Show resolved
Hide resolved
java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertifiacteValidation.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.ql
Outdated
Show resolved
Hide resolved
java/ql/src/change-notes/2022-06-22-improper-webview-certificate-validation.md
Outdated
Show resolved
Hide resolved
java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.qhelp
Show resolved
Hide resolved
atorralba
added
the
ready-for-doc-review
There was a problem hiding this comment.
@joefarebrother 👋🏻 - I carried out an editorial review and this LGTM ✨
Just a couple of clarifying comments.
| @@ -0,0 +1,18 @@ | |||
| /** | |||
| * @name Android `WebView` that accepts all certificates | |||
| * @description Trusting all certificates allows an attacker to perform a machine-in-the-middle attack. | |||
There was a problem hiding this comment.
Sorry, commenting here to check that machine-in-the-middle attack is a thing (I've only ever heard of man-in-the-middle attacks 🙊 ).
There was a problem hiding this comment.
It should be man-in-the-middle, thanks
Edit In fact both names are accepted, but I'll go with man-in-the-middle as it's the more common name.
There was a problem hiding this comment.
I thought you used machine-in-the-middle as a gender-neutral term :-)
There was a problem hiding this comment.
In fact I copied it from another file. But that reasoning makes sense; I'll keep it as machine then
| <overview> | ||
| <p> | ||
| If the <code>onReceivedSslError</code> method of an Android <code>WebViewClient</code> always calls <code>proceed</code> on the given <code>SslErrorHandler</code>, it trusts any certificate. | ||
| This allows an attacker to perform a machine-in-the-middle attack against the application, therefore breaking any security Transport Layer Security (TLS) gives. |
There was a problem hiding this comment.
Same comment as on the ql file (double-checking)
(gender-neutral language) This reverts commit d5ab330.
joefarebrother
force-pushed
the
android-certificate-validation
branch
from
0c9dfb0 to
dd83c17
Compare
|
Is this ready to merge? @atorralba |
This query finds implentations of
WebViewClient.onRecievedSslErrorthat unconditionaly callSslErrorHandler.proceedand thus accept all certificates.