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.

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: Model the Pyramid framework #16300

Merged
merged 9 commits into from
May 14, 2024
1 change: 1 addition & 0 deletions python/ql/lib/semmle/python/Frameworks.qll
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ private import semmle.python.frameworks.PyMongo
private import semmle.python.frameworks.Pymssql
private import semmle.python.frameworks.PyMySQL
private import semmle.python.frameworks.Pyodbc
private import semmle.python.frameworks.Pyramid
private import semmle.python.frameworks.Requests
private import semmle.python.frameworks.RestFramework
private import semmle.python.frameworks.Rsa
Expand Down
87 changes: 87 additions & 0 deletions python/ql/lib/semmle/python/frameworks/Pyramid.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* Provides classes modeling security-relevant aspects of the `pyramid` PyPI package.
* See https://docs.pylonsproject.org/projects/pyramid/.
*/

private import python
private import semmle.python.dataflow.new.DataFlow
Fixed Show fixed Hide fixed
private import semmle.python.dataflow.new.RemoteFlowSources
private import semmle.python.dataflow.new.TaintTracking
private import semmle.python.Concepts
private import semmle.python.ApiGraphs
private import semmle.python.dataflow.new.FlowSummary
private import semmle.python.frameworks.internal.PoorMansFunctionResolution
private import semmle.python.frameworks.data.ModelsAsData

/**
* Provides models for the `pyramid` PyPI package.
* See https://docs.pylonsproject.org/projects/pyramid/.
*/
module Pyramid {
// TODO: qldoc
module View {
/**
* A callable that could be used as a pyramid view callable.
*/
private class PotentialViewCallable extends Function {
PotentialViewCallable() {
this.getPositionalParameterCount() = 1 and
this.getArgName(0) = "request"
Copy link
Member

Choose a reason for hiding this comment

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

is there a hard requirement that the request parameter MUST be called request? or could it be named req as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation suggests that it should:

View callables must, at a minimum, accept a single argument named request.

however empirical testing seems to show it can allow other names.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, let's include that in our tests, and let's change our modeling to handle that 👍

or
this.getPositionalParameterCount() = 2 and
this.getArgName(0) = "context" and
this.getArgName(1) = "request"
}

/** Gets the `request` parameter of this view callable. */
Parameter getRequestParameter() { result = this.getArgByName("request") }
}

abstract class ViewCallable extends PotentialViewCallable, Http::Server::RequestHandler::Range {
override Parameter getARoutedParameter() { result = this.getRequestParameter() }
Copy link
Member

Choose a reason for hiding this comment

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

for a route like /hello/<name> that is handled by def handle_hello(request, name): ... the <name> is a routed parameter. I haven't looked at the pyramid docs whether this is something they do, but that's sort of what it's for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think pyramid has parameters like that (instead name would be in a matchdict object on the request.
Should the request parameter be modeled as a remote flow source separately from this then?

Copy link
Member

Choose a reason for hiding this comment

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

Should the request parameter be modeled as a remote flow source separately from this then?

Yes please 👍


override string getFramework() { result = "Pyramid" }
}

private class ViewCallableFromDecorator extends ViewCallable {
ViewCallableFromDecorator() {
this.getADecorator() =
API::moduleImport("pyramid")
.getMember("view")
.getMember("view_config")
.getACall()
.asExpr()
}
}

private class ViewCallableFromConfigurator extends ViewCallable {
ViewCallableFromConfigurator() {
any(Configurator::AddViewCall c).getViewArg() = poorMansFunctionTracker(this)
}
}
}

module Configurator {
/** Gets a reference to the class `pyramid.config.Configurator`. */
API::Node classRef() {
result = API::moduleImport("pyramid").getMember("config").getMember("Configurator")
}

/** Gets a reference to an instance of `pyramid.config.Configurator`. */
private DataFlow::TypeTrackingNode instance(DataFlow::TypeTracker t) {
t.start() and
result = classRef().getACall()
or
exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
}

/** Gets a reference to an instance of `pyramid.config.Configurator`. */
DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }

class AddViewCall extends DataFlow::MethodCallNode {
AddViewCall() { this.calls(instance(), "add_view") }

DataFlow::Node getViewArg() { result = [this.getArg(0), this.getArgByName("view")] }
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
argumentToEnsureNotTaintedNotMarkedAsSpurious
untaintedArgumentToEnsureTaintedNotMarkedAsMissing
testFailures
failures
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import experimental.meta.InlineTaintTest
import MakeInlineTaintTest<TestTaintTrackingConfig>
21 changes: 21 additions & 0 deletions python/ql/test/library-tests/frameworks/pyramid/pyramid_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from pyramid.view import view_config
from pyramid.config import Configurator

@view_config(route_name="test1")
def test1(request):
ensure_tainted(
request, # $ tainted
request.body, # $ MISSING:tainted
request.GET['a'] # $ MISSING:tainted
)

def test2(request):
ensure_tainted(request) # $ tainted

@view_config(route_name="test1")
def test3(context, request):
ensure_tainted(request) # $ tainted

if __name__ == "__main__":
with Configurator() as config:
config.add_view(test2, route_name="test2")