Python: Add query for insecure SSH host key policies in Paramiko. - #1128
Conversation
| * @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.
|
|
||
| # ... 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.
|
|
||
| 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())
| 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.
| 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?
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.
| @@ -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.
| 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. "
|
Thank you both for your comments. I believe I have addressed them all in the commits I pushed just now. |
|
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. |
| ## 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.
Adds a query that looks for instances where the host key policy is set to
AutoAddPolicyorWarningPolicyboth of which are insecure, as they do not terminate the connection when the host key is unknown.@felicity-semmle for the documentation.