Skip to content

Python: Add taint-tracking configuration.#1052

Merged
taus-semmle merged 3 commits intogithub:rc/1.20from
markshannon:python-taint-tracking-configuration
Mar 15, 2019
Merged

Python: Add taint-tracking configuration.#1052
taus-semmle merged 3 commits intogithub:rc/1.20from
markshannon:python-taint-tracking-configuration

Conversation

@markshannon
Copy link
Contributor

New configuration based interface for 1.20.

Should help avoid accidentally using extra sources and sinks via import, and brings the interface closer to that of the other languages.

It is unused for 1.20, to avoid risk of breakage.
However, it will be available for customers, will allow documentation to match code on master, and means that the documentation for writing taint-tracking queries will not need to change from 1.20 to 1.21.

This PR on master demonstrates it use:
#1051

…ss-talk between flows and brings the interface closer to that of the other languages.
Copy link
Contributor

@taus-semmle taus-semmle left a comment

Choose a reason for hiding this comment

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

A few points that need to be clarified, but otherwise LGTM.

* there are no `TaintTracking::Configuration`s.
*/
private predicate valid_sanitizer(Sanitizer sanitizer) {
forall (TaintTracking::Configuration c | c.isSanitizer(sanitizer))
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this much too strong, requiring the sanitizer to be part of every configuration present (or are we only allowed to have a single configuration at a time)?
I would expect the body of the predicate to be something like

not exists(TaintTracking::Configuration c) 
or
exists(TaintTracking::Configuration c | c.isSanitizer(sanitizer))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With multiple configurations, either form is wrong unless we track which configuration a taint originates from. For the case of one configuration, both forms are equivalent.
Having said that, I agree that your formulation would be less surprising should multiple configurations be present and that is what the qldoc comment suggests.

fromnode.getContext().getCallee(call) = tocontext and
exists(DataFlowNode fromnodenode |
fromnodenode = fromnode.getNode() and
forall(TaintTracking::Configuration c | c.isExtension(fromnodenode))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as for valid_sanitizer.

…ss surprising for the unlikely case of mutliple configurations.
@markshannon markshannon force-pushed the python-taint-tracking-configuration branch from f28d8e0 to 0866f4d Compare March 13, 2019 17:39
@markshannon markshannon force-pushed the python-taint-tracking-configuration branch from 0866f4d to ab23a15 Compare March 14, 2019 10:23
Copy link
Contributor

@taus-semmle taus-semmle left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Merging.

@taus-semmle taus-semmle merged commit 0b2f44b into github:rc/1.20 Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants