Python: Remove getResponse and do manual taint steps

Author: RasmusWL

python/ql/lib/semmle/python/Concepts.qll

@@ -822,9 +822,6 @@ module HTTP {
      * extend `Request::Range` instead.
      */
     class Request extends DataFlow::Node instanceof Request::Range {
-      /** Gets a node that provides the response to this request. */
-      DataFlow::Node getResponse() { result = super.getResponse() }
-
       /**
        * Gets a node that contributes to the URL of the request.
        * Depending on the framework, a request may have multiple nodes which contribute to the URL.
@@ -857,9 +854,6 @@ module HTTP {
        * extend `Request` instead.
        */
       abstract class Range extends DataFlow::Node {
-        /** Gets a node that provides the response to this request. */
-        abstract DataFlow::Node getResponse();
-
         /**
          * Gets a node that contributes to the URL of the request.
          * Depending on the framework, a request may have multiple nodes which contribute to the URL.
@@ -881,18 +875,6 @@ module HTTP {
         );
       }
     }
-
-    /**
-     * Additional taint step from a client request with user-controlled URL to the response.
-     */
-    private class HttpClientRequestAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
-      override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-        exists(Request req |
-          nodeFrom = req.getAUrlPart() and
-          nodeTo = req.getResponse()
-        )
-      }
-    }
     // TODO: investigate whether we should treat responses to client requests as
     // remote-flow-sources in general.
   }

python/ql/lib/semmle/python/frameworks/Requests.qll

@@ -10,6 +10,7 @@ private import python
 private import semmle.python.Concepts
 private import semmle.python.ApiGraphs
 private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.dataflow.new.TaintTracking
 private import semmle.python.frameworks.internal.InstanceTaintStepsHelper
 private import semmle.python.frameworks.Stdlib
 
@@ -53,8 +54,6 @@ private module Requests {
       result = this.getArg(1)
     }
 
-    override DataFlow::Node getResponse() { result = this }
-
     /** Gets the `verify` argument to this outgoing requests call. */
     DataFlow::Node getVerifyArg() { result = this.getArgByName("verify") }
 
@@ -70,6 +69,16 @@ private module Requests {
     override string getFramework() { result = "requests" }
   }
 
+  /**
+   * Extra taint propagation for outgoing requests calls,
+   * to ensure that responses to user-controlled URL are tainted.
+   */
+  private class OutgoingRequestCallTaintStep extends TaintTracking::AdditionalTaintStep {
+    override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+      nodeFrom = nodeTo.(OutgoingRequestCall).getAUrlPart()
+    }
+  }
+
   /** Gets a back-reference to the verify argument `arg`. */
   private DataFlow::TypeTrackingNode verifyArgBacktracker(
     DataFlow::TypeBackTracker t, DataFlow::Node arg
@@ -120,7 +129,7 @@ private module Requests {
 
     /** Return value from making a reuqest. */
     private class RequestReturnValue extends InstanceSource, DataFlow::Node {
-      RequestReturnValue() { this = any(OutgoingRequestCall c).getResponse() }
+      RequestReturnValue() { this = any(OutgoingRequestCall c) }
     }
 
     /** Gets a reference to an instance of `requests.models.Response`. */

python/ql/lib/semmle/python/frameworks/Stdlib.qll

@@ -2165,22 +2165,10 @@ private module StdlibPrivate {
     private class RequestCall extends HTTP::Client::Request::Range, DataFlow::MethodCallNode {
       RequestCall() { this.calls(instance(_), ["request", "_send_request", "putrequest"]) }
 
-      override DataFlow::Node getResponse() {
-        // TODO: this does not seem like the right abstraction, to allow for nice path-explanations
-        //
-        // For nice path-explanation, we would like either
-        // 1: tainting instance
-        // 1a. host on object creation -> obj
-        // 1b. url on request call -> obj
-        // 2. obj -> obj.getresponse()
-        //
-        // For now, that's really all we use the `getResponse` predicate for.
-        result.(HttpConnectionGetResponseCall).getObject().getALocalSource() =
-          this.getObject().getALocalSource()
-      }
+      DataFlow::Node getUrlArg() { result in [this.getArg(1), this.getArgByName("url")] }
 
       override DataFlow::Node getAUrlPart() {
-        result in [this.getArg(1), this.getArgByName("url")]
+        result = this.getUrlArg()
         or
         this.getObject() = instance(result)
       }
@@ -2202,6 +2190,32 @@ private module StdlibPrivate {
       HTTPResponse::InstanceSource {
       HttpConnectionGetResponseCall() { this.calls(instance(_), "getresponse") }
     }
+
+    /**
+     * Extra taint propagation for `http.client.HTTPConnection`,
+     * to ensure that responses to user-controlled URL are tainted.
+     */
+    private class AdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
+      override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+        // constructor
+        exists(InstanceSource instanceSource |
+          nodeFrom = instanceSource.getHostArgument() and
+          nodeTo = instanceSource
+        )
+        or
+        // a request method
+        exists(RequestCall call |
+          nodeFrom = call.getUrlArg() and
+          nodeTo.(DataFlow::PostUpdateNode).getPreUpdateNode() = call.getObject()
+        )
+        or
+        // `getresponse` call
+        exists(HttpConnectionGetResponseCall call |
+          nodeFrom = call.getObject() and
+          nodeTo = call
+        )
+      }
+    }
   }
 
   /**