Python: model fabric#4513
Conversation
For v1 tests, just extended with explicit calls that use keyword arguments. For v2 tests, rewrote pretty much everything to what it 100% explicit what we support
This required some thought for how to model that we're interested in subclasses
of `fabric.group.Group`, and not so much that class itself. Some thoughts:
---
After initially using this in `module Group`
/** A reference to a subclass of `fabric.group.Group` */
abstract class SubclassRef extends DataFlow::Node { }
private class SubclassInstantiation extends SubclassInstanceSource, DataFlow::CfgNode {
override CallNode node;
SubclassInstantiation() { node.getFunction() = any(SubclassRef ref).asCfgNode() }
}
with this in `module SerialGroup` and `module ThreadingGroup`:
class ClassRef extends DataFlow::Node, fabric::group::Group::SubclassRef {
ClassRef() { this = classRef(DataFlow::TypeTracker::end()) }
}
I wasn't too much of fan of that approach. Since we probably need the `SubclassInstanceSource` anyway, and don't really have a specific use for `SubclassRef`, I just went with concrete (QL) subclasses of `SubclassInstanceSource` in each of the modules for the Python subclasses.
I really don't know what the best approach is, so I'm very open to suggestions. I think we'll really have to flesh this out for handling Django responses, since we're interested in the fact that some subclasses provide default values for the content-type, and keeping track of that is important for XSS (since there is no XSS if response is `text/plain`)
yoff
left a comment
There was a problem hiding this comment.
Generally looks good, just one minor question.
Also, I was momentarily confused by GroupSubclassInstanceSource not being connected to InstanceSource (because the latter is for Connection rather than Group).
| } | ||
|
|
||
| /** | ||
| * A call to `run`, `sudo` on an instance of a subclass of `fabric.group.Group`. |
There was a problem hiding this comment.
too much copy paste 😐 there is no sudo method on a Group, so I'll just remove that bit 👍
| * See https://docs.fabfile.org/en/2.5/api/group.html#fabric.group.Group.run | ||
| */ | ||
| private DataFlow::Node subclassInstanceRunMethod(DataFlow::TypeTracker t) { | ||
| t.startInAttr("run") and |
Co-authored-by: yoff <lerchedahl@gmail.com>
Aha. In my mind, we're at a pretty OK point in regards to modeling classes in an extensible way with |
yoff
left a comment
There was a problem hiding this comment.
Looks good now.
Just to comment on the execute function: The special methods framework would need a slight generalisation, I think, in that it currently always invoke a class method. It might be worth it, if we foresee having to model many library functions that result in some function being called...
I intentionally left out the part of modeling
fabric.api.execute(from version 1.x), since I didn't see an obvious way to integrate this as an additional taint step in our current setup.@yoff proposed that we might be able to add this in the same way as we do for special methods (such as
__add__). However, since these are currently as a part of the algebraic datatypeTDataFlowCallinDataFlowPrivate, that doesn't really make it easy to make such a change entirely in the code modeling fabric (that isFabric.qll).The current implementation is able to do this as a simple additional taint step by inspecting the first argument of
fabric.api.executeand the callable it points to -- and it automatically handles the distinction between passing in functions and bound-methods.I'm hoping we can enable such easy extension as well, but I currently don't see a clear path forwards, so will leave that for future work.
Modeling subclasses of
fabric.group.GroupThe most interesting part of this PR is the part where we want to model subclasses of
fabric.group.Group, and not the class itself. This required some thought for how to do so (you probably need to have read through the changes in 98691fe before you're able to understand what I'm talking about here)After initially using the following code in
module Groupwith this code in
module SerialGroupandmodule ThreadingGroup:I wasn't too much of fan of that approach. Since we probably need the
SubclassInstanceSourceanyway, and don't really have a specific use forSubclassRef, I just went with concrete (QL) subclasses ofSubclassInstanceSourcein each of the modules for the Python subclasses.I really don't know what the best approach is, so I'm very open to suggestions. I think we'll really have to flesh this out for handling Django responses, since we're interested in the fact that some subclasses provide default values for the content-type, and keeping track of that is important for XSS (since there is no XSS if response is
text/plain)