Merge pull request #9463 from RasmusWL/req-wo-cert-validation

Python: Rewrite `py/request-without-cert-validation`

Author: yoff

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

@@ -662,7 +662,7 @@ private module AiohttpClientModel {
     private API::Node instance() { result = classRef().getReturn() }
 
     /** A method call on a ClientSession that sends off a request */
-    private class OutgoingRequestCall extends HTTP::Client::Request::Range, DataFlow::CallCfgNode {
+    private class OutgoingRequestCall extends HTTP::Client::Request::Range, API::CallNode {
       string methodName;
 
       OutgoingRequestCall() {
@@ -685,8 +685,14 @@ private module AiohttpClientModel {
       override predicate disablesCertificateValidation(
         DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
       ) {
-        // TODO: Look into disabling certificate validation
-        none()
+        exists(API::Node param | param = this.getKeywordParameter(["ssl", "verify_ssl"]) |
+          disablingNode = param.getARhs() and
+          argumentOrigin = param.getAValueReachingRhs() and
+          // aiohttp.client treats `None` as the default and all other "falsey" values as `False`.
+          argumentOrigin.asExpr().(ImmutableLiteral).booleanValue() = false and
+          not argumentOrigin.asExpr() instanceof None
+        )
+        // TODO: Handling of SSLContext passed as ssl/ssl_context arguments
       }
     }
   }

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

@@ -18,7 +18,12 @@ private import semmle.python.ApiGraphs
  * - https://www.python-httpx.org/
  */
 private module HttpxModel {
-  private class RequestCall extends HTTP::Client::Request::Range, DataFlow::CallCfgNode {
+  /**
+   * An outgoing HTTP request, from the `httpx` library.
+   *
+   * See https://www.python-httpx.org/api/
+   */
+  private class RequestCall extends HTTP::Client::Request::Range, API::CallNode {
     string methodName;
 
     RequestCall() {
@@ -39,32 +44,32 @@ private module HttpxModel {
     override predicate disablesCertificateValidation(
       DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
     ) {
-      // TODO: Look into disabling certificate validation
-      none()
+      disablingNode = this.getKeywordParameter("verify").getARhs() and
+      argumentOrigin = this.getKeywordParameter("verify").getAValueReachingRhs() and
+      // unlike `requests`, httpx treats `None` as turning off verify (and not as the default)
+      argumentOrigin.asExpr().(ImmutableLiteral).booleanValue() = false
+      // TODO: Handling of insecure SSLContext passed to verify argument
     }
   }
 
   /**
    * Provides models for the `httpx.[Async]Client` class
    *
-   * See https://www.python-httpx.org/async/
+   * See https://www.python-httpx.org/api/#client
    */
   module Client {
     /** Get a reference to the `httpx.Client` or `httpx.AsyncClient` class. */
     private API::Node classRef() {
       result = API::moduleImport("httpx").getMember(["Client", "AsyncClient"])
     }
 
-    /** Get a reference to an `httpx.Client` or `httpx.AsyncClient` instance. */
-    private API::Node instance() { result = classRef().getReturn() }
-
     /** A method call on a Client that sends off a request */
     private class OutgoingRequestCall extends HTTP::Client::Request::Range, DataFlow::CallCfgNode {
       string methodName;
 
       OutgoingRequestCall() {
         methodName in [HTTP::httpVerbLower(), "request", "stream"] and
-        this = instance().getMember(methodName).getACall()
+        this = classRef().getReturn().getMember(methodName).getACall()
       }
 
       override DataFlow::Node getAUrlPart() {
@@ -80,8 +85,16 @@ private module HttpxModel {
       override predicate disablesCertificateValidation(
         DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
       ) {
-        // TODO: Look into disabling certificate validation
-        none()
+        exists(API::CallNode constructor |
+          constructor = classRef().getACall() and
+          this = constructor.getReturn().getMember(methodName).getACall()
+        |
+          disablingNode = constructor.getKeywordParameter("verify").getARhs() and
+          argumentOrigin = constructor.getKeywordParameter("verify").getAValueReachingRhs() and
+          // unlike `requests`, httpx treats `None` as turning off verify (and not as the default)
+          argumentOrigin.asExpr().(ImmutableLiteral).booleanValue() = false
+          // TODO: Handling of insecure SSLContext passed to verify argument
+        )
       }
     }
   }

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

@@ -20,9 +20,14 @@ private import semmle.python.frameworks.Stdlib
  *
  * See
  * - https://pypi.org/project/requests/
- * - https://docs.python-requests.org/en/latest/
+ * - https://requests.readthedocs.io/en/latest/
  */
 private module Requests {
+  /**
+   * An outgoing HTTP request, from the `requests` library.
+   *
+   * See https://requests.readthedocs.io/en/latest/api/#requests.request
+   */
   private class OutgoingRequestCall extends HTTP::Client::Request::Range, API::CallNode {
     string methodName;
 
@@ -58,6 +63,7 @@ private module Requests {
     ) {
       disablingNode = this.getKeywordParameter("verify").getARhs() and
       argumentOrigin = this.getKeywordParameter("verify").getAValueReachingRhs() and
+      // requests treats `None` as the default and all other "falsey" values as `False`.
       argumentOrigin.asExpr().(ImmutableLiteral).booleanValue() = false and
       not argumentOrigin.asExpr() instanceof None
     }
@@ -81,7 +87,7 @@ private module Requests {
   /**
    * Provides models for the `requests.models.Response` class
    *
-   * See https://docs.python-requests.org/en/latest/api/#requests.Response.
+   * See https://requests.readthedocs.io/en/latest/api/#requests.Response.
    */
   module Response {
     /** Gets a reference to the `requests.models.Response` class. */

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

@@ -42,7 +42,8 @@ private module Urllib {
       override predicate disablesCertificateValidation(
         DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
       ) {
-        // TODO: Look into disabling certificate validation
+        // cannot enable/disable certificate validation on this object, only when used
+        // with `urlopen`, which is modeled below
         none()
       }
     }
@@ -63,7 +64,8 @@ private module Urllib {
       override predicate disablesCertificateValidation(
         DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
       ) {
-        // TODO: Look into disabling certificate validation
+        // will validate certificate by default, see https://github.com/python/cpython/blob/243ed5439c32e8517aa745bc2ca9774d99c99d0f/Lib/http/client.py#L1420-L1421
+        // TODO: Handling of insecure SSLContext passed to context argument
         none()
       }
     }

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

@@ -30,7 +30,8 @@ private module Urllib2 {
     override predicate disablesCertificateValidation(
       DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
     ) {
-      // TODO: Look into disabling certificate validation
+      // cannot enable/disable certificate validation on this object, only when used
+      // with `urlopen`, which is modeled below
       none()
     }
   }
@@ -49,7 +50,8 @@ private module Urllib2 {
     override predicate disablesCertificateValidation(
       DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
     ) {
-      // TODO: Look into disabling certificate validation
+      // will validate certificate by default
+      // TODO: Handling of insecure SSLContext passed to context argument
       none()
     }
   }

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

@@ -42,20 +42,18 @@ private module Urllib3 {
             .getASubclass+()
     }
 
-    /** Gets a reference to an instance of a `urllib3.request.RequestMethods` subclass. */
-    private API::Node instance() { result = classRef().getReturn() }
-
     /**
      * A call to a method making an outgoing request.
      *
      * See
      * - https://urllib3.readthedocs.io/en/stable/reference/urllib3.request.html#urllib3.request.RequestMethods
      * - https://urllib3.readthedocs.io/en/stable/reference/urllib3.connectionpool.html#urllib3.HTTPConnectionPool.urlopen
      */
-    private class RequestCall extends HTTP::Client::Request::Range, DataFlow::CallCfgNode {
+    private class RequestCall extends HTTP::Client::Request::Range, API::CallNode {
       RequestCall() {
         this =
-          instance()
+          classRef()
+              .getReturn()
               .getMember(["request", "request_encode_url", "request_encode_body", "urlopen"])
               .getACall()
       }
@@ -67,8 +65,22 @@ private module Urllib3 {
       override predicate disablesCertificateValidation(
         DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
       ) {
-        // TODO: Look into disabling certificate validation
-        none()
+        exists(API::CallNode constructor |
+          constructor = classRef().getACall() and
+          this = constructor.getReturn().getAMember().getACall()
+        |
+          // cert_reqs
+          // see https://urllib3.readthedocs.io/en/stable/user-guide.html?highlight=cert_reqs#certificate-verification
+          disablingNode = constructor.getKeywordParameter("cert_reqs").getARhs() and
+          argumentOrigin = constructor.getKeywordParameter("cert_reqs").getAValueReachingRhs() and
+          argumentOrigin.asExpr().(StrConst).getText() = "CERT_NONE"
+          or
+          // assert_hostname
+          // see https://urllib3.readthedocs.io/en/stable/reference/urllib3.connectionpool.html?highlight=assert_hostname#urllib3.HTTPSConnectionPool
+          disablingNode = constructor.getKeywordParameter("assert_hostname").getARhs() and
+          argumentOrigin = constructor.getKeywordParameter("assert_hostname").getAValueReachingRhs() and
+          argumentOrigin.asExpr().(BooleanLiteral).booleanValue() = false
+        )
       }
     }
   }

python/ql/src/Security/CWE-295/RequestWithoutValidation.ql

@@ -15,12 +15,14 @@ private import semmle.python.dataflow.new.DataFlow
 private import semmle.python.Concepts
 private import semmle.python.ApiGraphs
 
-from API::CallNode call, DataFlow::Node falseyOrigin, string verb
+from
+  HTTP::Client::Request request, DataFlow::Node disablingNode, DataFlow::Node origin, string ending
 where
-  verb = HTTP::httpVerbLower() and
-  call = API::moduleImport("requests").getMember(verb).getACall() and
-  falseyOrigin = call.getKeywordParameter("verify").getAValueReachingRhs() and
-  // requests treats `None` as the default and all other "falsey" values as `False`.
-  falseyOrigin.asExpr().(ImmutableLiteral).booleanValue() = false and
-  not falseyOrigin.asExpr() instanceof None
-select call, "Call to requests." + verb + " with verify=$@", falseyOrigin, "False"
+  request.disablesCertificateValidation(disablingNode, origin) and
+  // Showing the origin is only useful when it's a different node than the one disabling
+  // certificate validation, for example in `requests.get(..., verify=arg)`, `arg` would
+  // be the `disablingNode`, and the `origin` would be the place were `arg` got its
+  // value from.
+  if disablingNode = origin then ending = "." else ending = " by the value from $@."
+select request, "This request may run without certificate validation because it is $@" + ending,
+  disablingNode, "disabled here", origin, "here"

python/ql/src/change-notes/2022-06-08-request-without-certificate-validation-modeling.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* Improved library modeling for the query "Request without certificate validation" (`py/request-without-cert-validation`), so it now also covers `httpx`, `aiohttp.client`, and `urllib3`.

python/ql/test/library-tests/frameworks/aiohttp/client_request.py

@@ -1,5 +1,6 @@
 import aiohttp
 import asyncio
+import ssl
 
 s = aiohttp.ClientSession()
 resp = s.request("method", "url") # $ clientRequestUrlPart="url"
@@ -13,4 +14,22 @@
 s = aiohttp.ClientSession()
 resp = s.post("url") # $ clientRequestUrlPart="url"
 resp = s.patch("url") # $ clientRequestUrlPart="url"
-resp = s.options("url") # $ clientRequestUrlPart="url"
+resp = s.options("url") # $ clientRequestUrlPart="url"
+
+# disabling of SSL validation
+# see https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientSession.request
+s.get("url", ssl=False) # $ clientRequestUrlPart="url" clientRequestCertValidationDisabled
+s.get("url", ssl=0) # $ clientRequestUrlPart="url" clientRequestCertValidationDisabled
+# None is treated as default and so does _not_ disable the check
+s.get("url", ssl=None) # $ clientRequestUrlPart="url"
+
+# deprecated since 3.0, but still supported
+s.get("url", verify_ssl=False) # $ clientRequestUrlPart="url" clientRequestCertValidationDisabled
+
+# A manually constructed SSLContext does not have safe defaults, so is effectively the
+# same as turning off SSL validation
+context = ssl.SSLContext()
+assert context.check_hostname == False
+assert context.verify_mode == ssl.VerifyMode.CERT_NONE
+
+s.get("url", ssl=context) # $ clientRequestUrlPart="url" MISSING: clientRequestCertValidationDisabled

python/ql/test/library-tests/frameworks/httpx/test.py

@@ -1,4 +1,5 @@
 import httpx
+import ssl
 
 httpx.get("url") # $ clientRequestUrlPart="url"
 httpx.post("url") # $ clientRequestUrlPart="url"
@@ -15,10 +16,30 @@
 response = client.request("method", url="url") # $ clientRequestUrlPart="url"
 response = client.stream("method", url="url") # $ clientRequestUrlPart="url"
 
-client = httpx.AsyncClient()
-response = client.get("url") # $ clientRequestUrlPart="url"
-response = client.post("url") # $ clientRequestUrlPart="url"
-response = client.patch("url") # $ clientRequestUrlPart="url"
-response = client.options("url") # $ clientRequestUrlPart="url"
-response = client.request("method", url="url") # $ clientRequestUrlPart="url"
-response = client.stream("method", url="url") # $ clientRequestUrlPart="url"
+async def async_test():
+    client = httpx.AsyncClient()
+    response = await client.get("url") # $ clientRequestUrlPart="url"
+    response = await client.post("url") # $ clientRequestUrlPart="url"
+    response = await client.patch("url") # $ clientRequestUrlPart="url"
+    response = await client.options("url") # $ clientRequestUrlPart="url"
+    response = await client.request("method", url="url") # $ clientRequestUrlPart="url"
+    response = await client.stream("method", url="url") # $ clientRequestUrlPart="url"
+
+# ==============================================================================
+# Disabling certificate validation
+# ==============================================================================
+
+httpx.get("url", verify=False) # $ clientRequestUrlPart="url" clientRequestCertValidationDisabled
+httpx.get("url", verify=0) # $ clientRequestUrlPart="url" clientRequestCertValidationDisabled
+httpx.get("url", verify=None) # $ clientRequestUrlPart="url" clientRequestCertValidationDisabled
+
+# A manually constructed SSLContext does not have safe defaults, so is effectively the
+# same as turning off SSL validation
+context = ssl.SSLContext()
+assert context.check_hostname == False
+assert context.verify_mode == ssl.VerifyMode.CERT_NONE
+
+httpx.get("url", verify=context) # $ clientRequestUrlPart="url" MISSING: clientRequestCertValidationDisabled
+
+client = httpx.Client(verify=False)
+client.get("url") # $ clientRequestUrlPart="url" clientRequestCertValidationDisabled