Python: Add query for insecure SSH host key policies in Paramiko. - #1128
Conversation
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
A few issues
Sorry, something went wrong.
| * @kind problem | ||
| * @problem.severity error | ||
| * @precision high | ||
| * @id py/missing-host-key-validation |
There was a problem hiding this comment.
Add "paramiko" to the id, and the description. And maybe the name if not too cumbersome.
Sorry, something went wrong.
|
|
||
| # ... interaction with server | ||
|
|
||
| client.close() |
There was a problem hiding this comment.
Should have a "good" example as well, and encapsulate them in functions, so it is clear which is which.
Sorry, something went wrong.
|
|
||
| client = SSHClient() | ||
|
|
||
| client.set_missing_host_key_policy(AutoAddPolicy) # bad |
There was a problem hiding this comment.
Can we add test for instances as well client.set_missing_host_key_policy(AutoAddPolicy())
Sorry, something went wrong.
| result = theParamikoClientModule().attr("SSHClient") | ||
| } | ||
|
|
||
| private ClassObject unsafe_paramiko_policy(string name) { |
There was a problem hiding this comment.
According to the docs, instances of these classes are usable as well.
Sorry, something went wrong.
| from CallNode call, string name | ||
| where | ||
| call = theParamikoSSHClientClass() | ||
| .declaredAttribute("set_missing_host_key_policy") |
There was a problem hiding this comment.
Why declaredAttribute rather than the more usual lookupAttribute?
Sorry, something went wrong.
felicitymay
left a comment
There was a problem hiding this comment.
This is a new query and so needs a change note. A small text suggestion, otherwise LGTM. Thanks for pinging me.
Sorry, something went wrong.
| @@ -0,0 +1,32 @@ | |||
| /** | |||
| * @name Accepting unknown host keys. | |||
There was a problem hiding this comment.
By convention we don't use period to complete the @name field.
Sorry, something went wrong.
| setting the missing host key policy to either <code>AutoAddPolicy</code> or | ||
| <code>WarningPolicy</code>, as both of these will continue even when the host | ||
| key is unknown. The default <code>RejectPolicy</code> throws an exception when | ||
| unknown host keys are encountered. |
There was a problem hiding this comment.
Suggest something more like this?
"Do not accept unknown host keys. In particular, do not set the default missing host key policy for the Paramiko library to either AutoAddPolicy or WarningPolicy. Both of these policies continue even when the host key is unknown. The default setting of RejectPolicy is secure because it throws an exception when it encounters an unknown host key. "
Sorry, something went wrong.
|
Thank you both for your comments. I believe I have addressed them all in the commits I pushed just now. |
Sorry, something went wrong.
|
Thanks for the changes to the help topic. This still needs a change note, but otherwise looks good to me. @taus-semmle rightly points out that I'd somehow missed spotting the change note. One minor point otherwise text LGTM. |
Sorry, something went wrong.
| ## New queries | ||
| | **Query** | **Tags** | **Purpose** | | ||
| |-----------|----------|-------------| | ||
| | Accepting unknown SSH host keys when using Paramiko. (`py/paramiko-missing-host-key-validation`) | security, external/cwe/cwe-295 | Finds instances where Paramiko is configured to accept unknown host keys. Results are shown on LGTM by default. | |
There was a problem hiding this comment.
Please remove the period from the name.
Sorry, something went wrong.
Adds a query that looks for instances where the host key policy is set to AutoAddPolicy or WarningPolicy both of which are insecure, as they do not terminate the connection when the host key is unknown.
@felicity-semmle for the documentation.