New query to check for making a request without cert verification.#530
Conversation
markshannon
force-pushed
the
python-no-cert-validation
branch
from
3c98434 to
bfc001c
Compare
felicitymay
left a comment
felicitymay
left a comment
There was a problem hiding this comment.
Some comments on the text. For some reason, this PR hasn't triggered a qhelp preview.
| | **Query** | **Tags** | **Purpose** | | ||
| |-----------------------------|-----------|--------------------------------------------------------------------| | ||
| | Information exposure through an exception (`py/stack-trace-exposure`) | security, external/cwe/cwe-209, external/cwe/cwe-497 | Finds instances where information about an exception may be leaked to an external user. Enabled on LGTM by default. | | ||
| | Request Without Certificate Validation (`py/request-without-cert-validation`) | security, external/cwe/cwe-295 | Finds requests where certificate verification has been explicitly turned off, possibly allowing man-in-the-middle attacks. Not enabled on LGTM by default. | |
There was a problem hiding this comment.
Please change query name to sentence case, that is: "Request without certificate validation".
| <overview> | ||
| <p> | ||
| Encryption is key to the security of most, if not all, online communication. | ||
| Using TLS can enusre that neither party in the communication is an interloper. |
There was a problem hiding this comment.
TLS - ? Please include the definition the first time you mention this acronym
| <overview> | ||
| <p> | ||
| Encryption is key to the security of most, if not all, online communication. | ||
| Using TLS can enusre that neither party in the communication is an interloper. |
There was a problem hiding this comment.
Using TLS can enusre that neither party in the communication is an interloper.
Firstly: "enusre" is a typo. Secondly, this reads a little strangely. Suggest something like: "Using TLS can ensure that communication cannot be interrupted by an interloper".
| Encryption is key to the security of most, if not all, online communication. | ||
| Using TLS can enusre that neither party in the communication is an interloper. | ||
| For this reason, is is unwise to disable the verification that TLS provides. | ||
| <code>requests</code> provides verification by default, and it is only when |
There was a problem hiding this comment.
Suggest: "The requests method provides verification by default. To disable TLS verification you have to explicitly turn it off using verify=False." - assuming that requests is a method. It should read better and avoids starting a sentence with lowercase.
There was a problem hiding this comment.
requests is a module containing several functions, all of which have the optional verify parameter.
| <references> | ||
| <li> | ||
| Common Weakness Enumeration: | ||
| <a href="https://cwe.mitre.org/data/definitions/295.html">CWE-295: Improper Certificate Validation</a>. |
There was a problem hiding this comment.
Please delete this reference. While this is better than the autogenerated reference, it will result in duplication in the query help output.
| @@ -0,0 +1,36 @@ | |||
| /** | |||
| * @name Request Without Certificate Validation | |||
There was a problem hiding this comment.
Please change this to sentence case.
|
|
||
| <example> | ||
| <p> | ||
| The example shows two unsafe calls to <a href="https://semmle.com">semmle.com</a>, followed by various safe alternatives. |
There was a problem hiding this comment.
Does this text also cover the wrapper?
There was a problem hiding this comment.
Yes. The code should be self explanatory.
|
Is this intended for 1.19? |
|
Comments addressed and re-targeted at rc/1.19 |
|
Thanks for the text changes. Content all LGTM now. |
taus-semmle
left a comment
taus-semmle
left a comment
There was a problem hiding this comment.
One minor comment, otherwise LGTM.
| exists(ModuleObject req | | ||
| req.getName() = "requests" and | ||
| result = req.getAttribute(httpVerbLower()) | ||
| ) |
There was a problem hiding this comment.
This could be written a bit shorter as result = any(ModuleObject req | req.getName() = "requests").getAttribute(httpVerbLower()).
We should consider extending the library to have an easier way of expressing the any(ModuleObject m | m.getName() = "foo") idiom, as it is quite common. Perhaps we could add a NamedModule subclass of ModuleObject that accepts a string name in its characteristic predicate, and adds this.getName() = name as a constraint?
|
Oh, we should move |
3 participants
Checks for calls of the form
request.{http_verb}(verify=False)