Merge pull request github#13676 from RasmusWL/aiohttp-ssrf-sink

Python: Relax restriction of flow through `async with`

Author: RasmusWL

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

@@ -304,8 +304,12 @@ module EssaFlow {
       // see `with_flow` in `python/ql/src/semmle/python/dataflow/Implementation.qll`
       with.getContextExpr() = contextManager.getNode() and
       with.getOptionalVars() = var.getNode() and
-      not with.isAsync() and
       contextManager.strictlyDominates(var)
+      // note: we allow this for both `with` and `async with`, since some
+      // implementations do `async def __aenter__(self): return self`, so you can do
+      // both:
+      // * `foo = x.foo(); await foo.async_method(); foo.close()` and
+      // * `async with x.foo() as foo: await foo.async_method()`.
     )
     or
     // Async with var definition
@@ -314,6 +318,12 @@ module EssaFlow {
     //  nodeTo is `x`, essa var
     //
     // This makes the cfg node the local source of the awaited value.
+    //
+    // We have this step in addition to the step above, to handle cases where the QL
+    // modeling of `f(42)` requires a `.getAwaited()` step (in API graphs) when not
+    // using `async with`, so you can do both:
+    // * `foo = await x.foo(); await foo.async_method(); foo.close()` and
+    // * `async with x.foo() as foo: await foo.async_method()`.
     exists(With with, ControlFlowNode var |
       nodeFrom.(CfgNode).getNode() = var and
       nodeTo.(EssaNode).getVar().getDefinition().(WithDefinition).getDefiningNode() = var and

python/ql/src/change-notes/2023-07-06-aiohttp-clientsession-modeling.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed modeling of `aiohttp.ClientSession` so we properly handle `async with` uses. This can impact results of server-side request forgery queries (`py/full-ssrf`, `py/partial-ssrf`).

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

@@ -1,35 +1,35 @@
 import aiohttp
-import asyncio
 import ssl
 
-s = aiohttp.ClientSession()
-resp = s.request("method", "url") # $ clientRequestUrlPart="url"
-resp = s.request("method", url="url") # $ clientRequestUrlPart="url"
+async def test():
+    s = aiohttp.ClientSession()
+    resp = await s.request("method", "url") # $ clientRequestUrlPart="url"
+    resp = await s.request("method", url="url") # $ clientRequestUrlPart="url"
 
-with aiohttp.ClientSession() as session:
-    resp = session.get("url") # $ clientRequestUrlPart="url"
-    resp = session.request(method="GET", url="url") # $ clientRequestUrlPart="url"
+    async with aiohttp.ClientSession() as session:
+        resp = await session.get("url") # $ clientRequestUrlPart="url"
+        resp = await session.request(method="GET", url="url") # $ clientRequestUrlPart="url"
 
-# other methods than GET
-s = aiohttp.ClientSession()
-resp = s.post("url") # $ clientRequestUrlPart="url"
-resp = s.patch("url") # $ clientRequestUrlPart="url"
-resp = s.options("url") # $ clientRequestUrlPart="url"
+    # other methods than GET
+    s = aiohttp.ClientSession()
+    resp = await s.post("url") # $ clientRequestUrlPart="url"
+    resp = await s.patch("url") # $ clientRequestUrlPart="url"
+    resp = await 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"
+    # 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
+    # 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
+    # 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
+    s.get("url", ssl=context) # $ clientRequestUrlPart="url" MISSING: clientRequestCertValidationDisabled