-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: Rewrite py/request-without-cert-validation
#9463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ert-validation` This is very much like the Ruby query, except we also have the origin that does the disabling. https://github.com/github/codeql/blob/976daddd36a63bf46836d141d04172e90bb4b33c/ruby/ql/src/queries/security/cwe-295/RequestWithoutValidation.ql#L18-L20
Also fix links
and improve QLDocs a bit
We might also consider changing the precision to high. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single suggestion (that will hopefully pass the test), but LGTM.
I like how readable it all is using API::CallNode
:-)
argumentOrigin.asExpr().(ImmutableLiteral).booleanValue() = false and | ||
not argumentOrigin.asExpr() instanceof None | ||
) | ||
// TODO: Handling of SSLContext passed as ssl/ssl_context arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this would need a treatment similar to the SecureProtocol queries, making it difficult to fit into a single predicate like this..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to deal with that when we get there. I think having both disablingNode
and argumentOrigin
would allow us enough flexibility to get pretty far, but we'll see.
Co-authored-by: yoff <lerchedahl@gmail.com>
For the record, performance looks fine, and so does the new results from the additional modeling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I like how you made the alert have more information only in cases where that information adds value.
Also improves library modeling. I did not look into
libtaxii
orpycurl
, it did not seem worth the time.I left in a few
TODO: Handling of insecure SSLContext ...
for relevant libraries, I'll leave handling those as future work. We're going to be in a much better state with the changes in this PR 👍