Skip to content
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.

Sign up for GitHub

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

Python: Pycurl SSL Disabled #16812

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Python: Pycurl SSL Disabled #16812

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2dd126
Python: Pycurl SSL Disabled
porcupineyhairs Jun 21, 2024
7ef2d79
Include changes from review
porcupineyhairs Oct 20, 2024
f6369a6
Include changes from review
porcupineyhairs Oct 21, 2024
c74f6f5
Merge branch 'main' into pyloadSsl
porcupineyhairs Oct 21, 2024
c93f0ed
Include change-note
porcupineyhairs Oct 21, 2024
c7610b3
Include change-note
porcupineyhairs Oct 21, 2024
c78aeec
Update python/ql/lib/semmle/python/frameworks/Pycurl.qll
yoff Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions python/ql/lib/semmle/python/frameworks/Pycurl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ module Pycurl {
/** Gets a reference to an instance of `pycurl.Curl`. */
private API::Node instance() { result = classRef().getReturn() }

/** Gets a reference to an instance of `pycurl.Curl.setopt`. */
private API::Node setopt() { result = instance().getMember("setopt") }

/** Gets a reference to an instance of `pycurl.Curl.SSL_VERIFYPEER`. */
yoff marked this conversation as resolved.
Show resolved Hide resolved
private API::Node sslverifypeer() {
result = API::moduleImport("pycurl").getMember("SSL_VERIFYPEER") or
result = instance().getMember("SSL_VERIFYPEER")
}

/**
* When the first parameter value of the `setopt` function is set to `pycurl.URL`,
* the second parameter value is the request resource link.
Expand All @@ -45,7 +54,7 @@ module Pycurl {
*/
private class OutgoingRequestCall extends Http::Client::Request::Range, DataFlow::CallCfgNode {
OutgoingRequestCall() {
this = instance().getMember("setopt").getACall() and
this = setopt().getACall() and
this.getArg(0).asCfgNode().(AttrNode).getName() = "URL"
}

Expand All @@ -58,9 +67,37 @@ module Pycurl {
override predicate disablesCertificateValidation(
DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
) {
// TODO: Look into disabling certificate validation
none()
}
}

/**
* When the first parameter value of the `setopt` function is set to `SSL_VERIFYPEER` or `SSL_VERIFYHOST`,
* the second parameter value disables or enable SSL certifiacte verification.
*
* See http://pycurl.io/docs/latest/curlobject.html#pycurl.Curl.setopt.
*/
private class CurlSslCall extends Http::Client::Request::Range, DataFlow::CallCfgNode {
CurlSslCall() {
this = setopt().getACall() and
this.getArg(0).asCfgNode().(AttrNode).getName() = ["SSL_VERIFYPEER", "SSL_VERIFYHOST"]
}

override DataFlow::Node getAUrlPart() { none() }

override string getFramework() { result = "pycurl.Curl" }

override predicate disablesCertificateValidation(
DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
) {
sslverifypeer().getAValueReachableFromSource() = this.getArg(0) and
(
this.getArg(1).asExpr().(IntegerLiteral).getValue() = 0
or
this.getArg(1).asExpr().(BooleanLiteral).booleanValue() = false
) and
(disablingNode = this and argumentOrigin = this.getArg(1))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is representing calls to setopt where the first argument is specifically "PyCurl.URL". For these calls, the current implementation of disablesCertificateValidation as none() is correct. What you have done is expressing that this call disables certificate validation if there is another call with arguments (PyCurl.SSL_VERIFYPEER, 0).

What you would need to do is likely to create a new class extending Http::Client::Request::Range representing calls to setopt where the first argument is specifically "PyCurl.SSL_VERIFYPEER" and then you can implement disablesCertificateValidation by comparing the second argument with 0 (and also with False, please).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closer, but you are still looking at another call (c = setopt().getACall()). You have to look at the argument for the call represented by the class; something like c = this.getACall(). Also, the boolean has to come from the call; you are now just checking if they wrote false anywhere in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoff Changes done! PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, much better :-) I think the boolean would also have to go into arg(1)?

}
}
4 changes: 4 additions & 0 deletions python/ql/src/change-notes/2024-10-21-pyloadSsl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Improved modelling for the `pycurl` framework.
Loading