Python: Model CookieWrites from HeaderWrites

python/ql/lib/change-notes/2024-06-24-cookie-header-writes.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Additional modelling has been added to detect cookie writes from direct writes to the `Set-Cookie` header have been added for several web frameworks.

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

@@ -1134,6 +1134,54 @@ module Http {
       }
     }
 
+    /** A key-value pair in a literal for a bulk header update, considered as a single header update. */
+    private class HeaderBulkWriteDictLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
+    {
+      KeyValuePair item;
+
+      HeaderBulkWriteDictLiteral() {
+        exists(Dict dict | DataFlow::localFlow(DataFlow::exprNode(dict), super.getBulkArg()) |
+          item = dict.getAnItem()
+        )
+      }
+
+      override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
+
+      override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
+
+      override predicate nameAllowsNewline() {
+        Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
+      }
+
+      override predicate valueAllowsNewline() {
+        Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
+      }
+    }
+
+    /** A tuple in a list for a bulk header update, considered as a single header update. */
+    private class HeaderBulkWriteListLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
+    {
+      Tuple item;
+
+      HeaderBulkWriteListLiteral() {
+        exists(List list | DataFlow::localFlow(DataFlow::exprNode(list), super.getBulkArg()) |
+          item = list.getAnElt()
+        )
+      }
+
+      override DataFlow::Node getNameArg() { result.asExpr() = item.getElt(0) }
+
+      override DataFlow::Node getValueArg() { result.asExpr() = item.getElt(1) }
+
+      override predicate nameAllowsNewline() {
+        Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
+      }
+
+      override predicate valueAllowsNewline() {
+        Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
+      }
+    }
+
     /**
      * A data-flow node that sets a cookie in an HTTP response.
      *
@@ -1186,6 +1234,27 @@ module Http {
       }
     }
 
+    /** A write to a `Set-Cookie` header that sets a cookie directly. */
+    private class CookieHeaderWrite extends CookieWrite::Range instanceof Http::Server::ResponseHeaderWrite
+    {
+      CookieHeaderWrite() {
+        exists(StringLiteral str |
+          str.getText().toLowerCase() = "set-cookie" and
+          DataFlow::exprNode(str)
+              .(DataFlow::LocalSourceNode)
+              .flowsTo(this.(Http::Server::ResponseHeaderWrite).getNameArg())
+        )
+      }
+
+      override DataFlow::Node getNameArg() { none() }
+
+      override DataFlow::Node getHeaderArg() {
+        result = this.(Http::Server::ResponseHeaderWrite).getValueArg()
+      }
+
+      override DataFlow::Node getValueArg() { none() }
+    }
+
     /**
      * A data-flow node that enables or disables Cross-site request forgery protection
      * in a global manner.

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

@@ -706,6 +706,33 @@ module AiohttpWebModel {
 
     override DataFlow::Node getValueArg() { result = value }
   }
+
+  /**
+   * A dict-like write to an item of the `headers` attribute on a HTTP response, such as
+   * `response.headers[name] = value`.
+   */
+  class AiohttpResponseHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
+    DataFlow::Node index;
+    DataFlow::Node value;
+
+    AiohttpResponseHeaderSubscriptWrite() {
+      exists(API::Node i |
+        value = aiohttpResponseInstance().getMember("headers").getSubscriptAt(i).asSink() and
+        index = i.asSink() and
+        // To give `this` a value, we need to choose between either LHS or RHS,
+        // and just go with the RHS as it is readily available
+        this = value
+      )
+    }
+
+    override DataFlow::Node getNameArg() { result = index }
+
+    override DataFlow::Node getValueArg() { result = value }
+
+    override predicate nameAllowsNewline() { none() }
+
+    override predicate valueAllowsNewline() { none() }
+  }
 }
 
 /**

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

@@ -2239,6 +2239,71 @@ module PrivateDjango {
 
           override DataFlow::Node getValueArg() { result = value }
         }
+
+        /**
+         * A dict-like write to an item of the `headers` attribute on a HTTP response, such as
+         * `response.headers[name] = value`.
+         */
+        class DjangoResponseHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
+          DataFlow::Node index;
+          DataFlow::Node value;
+
+          DjangoResponseHeaderSubscriptWrite() {
+            exists(SubscriptNode subscript, DataFlow::AttrRead headerLookup |
+              // To give `this` a value, we need to choose between either LHS or RHS,
+              // and just go with the LHS
+              this.asCfgNode() = subscript
+            |
+              headerLookup
+                  .accesses(DjangoImpl::DjangoHttp::Response::HttpResponse::instance(), "headers") and
+              exists(DataFlow::Node subscriptObj |
+                subscriptObj.asCfgNode() = subscript.getObject()
+              |
+                headerLookup.flowsTo(subscriptObj)
+              ) and
+              value.asCfgNode() = subscript.(DefinitionNode).getValue() and
+              index.asCfgNode() = subscript.getIndex()
+            )
+          }
+
+          override DataFlow::Node getNameArg() { result = index }
+
+          override DataFlow::Node getValueArg() { result = value }
+
+          override predicate nameAllowsNewline() { none() }
+
+          override predicate valueAllowsNewline() { none() }
+        }
+
+        /**
+         * A dict-like write to an item of an HTTP response, which is treated as a header write,
+         * such as `response[headerName] = value`
+         */
+        class DjangoResponseSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
+          DataFlow::Node index;
+          DataFlow::Node value;
+
+          DjangoResponseSubscriptWrite() {
+            exists(SubscriptNode subscript |
+              // To give `this` a value, we need to choose between either LHS or RHS,
+              // and just go with the LHS
+              this.asCfgNode() = subscript
+            |
+              subscript.getObject() =
+                DjangoImpl::DjangoHttp::Response::HttpResponse::instance().asCfgNode() and
+              value.asCfgNode() = subscript.(DefinitionNode).getValue() and
+              index.asCfgNode() = subscript.getIndex()
+            )
+          }
+
+          override DataFlow::Node getNameArg() { result = index }
+
+          override DataFlow::Node getValueArg() { result = value }
+
+          override predicate nameAllowsNewline() { none() }
+
+          override predicate valueAllowsNewline() { none() }
+        }
       }
     }
 

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

@@ -361,28 +361,59 @@ module FastApi {
     }
 
     /**
-     * A call to `append` on a `headers` of a FastAPI Response, with the `Set-Cookie`
-     * header-key.
+     * A call to `append` on a `headers` of a FastAPI Response.
      */
-    private class HeadersAppendCookie extends Http::Server::CookieWrite::Range,
+    private class HeadersAppend extends Http::Server::ResponseHeaderWrite::Range,
       DataFlow::MethodCallNode
     {
-      HeadersAppendCookie() {
-        exists(DataFlow::AttrRead headers, DataFlow::Node keyArg |
+      HeadersAppend() {
+        exists(DataFlow::AttrRead headers |
           headers.accesses(instance(), "headers") and
-          this.calls(headers, "append") and
-          keyArg in [this.getArg(0), this.getArgByName("key")] and
-          keyArg.getALocalSource().asExpr().(StringLiteral).getText().toLowerCase() = "set-cookie"
+          this.calls(headers, "append")
         )
       }
 
-      override DataFlow::Node getHeaderArg() {
+      override DataFlow::Node getNameArg() { result = [this.getArg(0), this.getArgByName("key")] }
+
+      override DataFlow::Node getValueArg() {
         result in [this.getArg(1), this.getArgByName("value")]
       }
 
-      override DataFlow::Node getNameArg() { none() }
+      override predicate nameAllowsNewline() { none() }
+
+      override predicate valueAllowsNewline() { none() }
+    }
+
+    /**
+     * A dict-like write to an item of the `headers` attribute on a HTTP response, such as
+     * `response.headers[name] = value`.
+     */
+    class HeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
+      DataFlow::Node index;
+      DataFlow::Node value;
+
+      HeaderSubscriptWrite() {
+        exists(SubscriptNode subscript, DataFlow::AttrRead headerLookup |
+          // To give `this` a value, we need to choose between either LHS or RHS,
+          // and just go with the LHS
+          this.asCfgNode() = subscript
+        |
+          headerLookup.accesses(instance(), "headers") and
+          exists(DataFlow::Node subscriptObj | subscriptObj.asCfgNode() = subscript.getObject() |
+            headerLookup.flowsTo(subscriptObj)
+          ) and
+          value.asCfgNode() = subscript.(DefinitionNode).getValue() and
+          index.asCfgNode() = subscript.getIndex()
+        )
+      }
+
+      override DataFlow::Node getNameArg() { result = index }
+
+      override DataFlow::Node getValueArg() { result = value }
+
+      override predicate nameAllowsNewline() { none() }
 
-      override DataFlow::Node getValueArg() { none() }
+      override predicate valueAllowsNewline() { none() }
     }
   }
 }

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

@@ -63,6 +63,50 @@ module Tornado {
 
       override string getAsyncMethodName() { none() }
     }
+
+    /**
+     * A dict-like write to an item of an `HTTPHeaders` object.
+     */
+    private class TornadoHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
+      DataFlow::Node index;
+      DataFlow::Node value;
+
+      TornadoHeaderSubscriptWrite() {
+        exists(SubscriptNode subscript |
+          subscript.getObject() = instance().asCfgNode() and
+          value.asCfgNode() = subscript.(DefinitionNode).getValue() and
+          index.asCfgNode() = subscript.getIndex() and
+          this.asCfgNode() = subscript
+        )
+      }
+
+      override DataFlow::Node getNameArg() { result = index }
+
+      override DataFlow::Node getValueArg() { result = value }
+
+      override predicate nameAllowsNewline() { none() }
+
+      override predicate valueAllowsNewline() { none() }
+    }
+
+    /**
+     * A call to `HTTPHeaders.add`.
+     */
+    private class TornadoHeadersAppendCall extends Http::Server::ResponseHeaderWrite::Range,
+      DataFlow::MethodCallNode
+    {
+      TornadoHeadersAppendCall() { this.calls(instance(), "add") }
+
+      override DataFlow::Node getNameArg() { result = [this.getArg(0), this.getArgByName("name")] }
+
+      override DataFlow::Node getValueArg() {
+        result in [this.getArg(1), this.getArgByName("value")]
+      }
+
+      override predicate nameAllowsNewline() { none() }
+
+      override predicate valueAllowsNewline() { none() }
+    }
   }
 
   // ---------------------------------------------------------------------------
@@ -209,6 +253,25 @@ module Tornado {
             this.(DataFlow::AttrRead).getAttributeName() = "request"
           }
         }
+
+        /** A call to `RequestHandler.set_header` or `RequestHandler.add_header` */
+        private class TornadoSetHeaderCall extends Http::Server::ResponseHeaderWrite::Range,
+          DataFlow::MethodCallNode
+        {
+          TornadoSetHeaderCall() { this.calls(instance(), ["set_header", "add_header"]) }
+
+          override DataFlow::Node getNameArg() {
+            result = [this.getArg(0), this.getArgByName("name")]
+          }
+
+          override DataFlow::Node getValueArg() {
+            result in [this.getArg(1), this.getArgByName("value")]
+          }
+
+          override predicate nameAllowsNewline() { none() }
+
+          override predicate valueAllowsNewline() { none() }
+        }
       }
 
       /**

python/ql/lib/semmle/python/security/dataflow/HttpHeaderInjectionCustomizations.qll

@@ -51,56 +51,6 @@ module HttpHeaderInjection {
     }
   }
 
-  /** A key-value pair in a literal for a bulk header update, considered as a single header update. */
-  // TODO: We could instead consider bulk writes as sinks with an implicit read step of DictionaryKey/DictionaryValue content as needed.
-  private class HeaderBulkWriteDictLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
-  {
-    KeyValuePair item;
-
-    HeaderBulkWriteDictLiteral() {
-      exists(Dict dict | DataFlow::localFlow(DataFlow::exprNode(dict), super.getBulkArg()) |
-        item = dict.getAnItem()
-      )
-    }
-
-    override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
-
-    override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
-
-    override predicate nameAllowsNewline() {
-      Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
-    }
-
-    override predicate valueAllowsNewline() {
-      Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
-    }
-  }
-
-  /** A tuple in a list for a bulk header update, considered as a single header update. */
-  // TODO: We could instead consider bulk writes as sinks with implicit read steps as needed.
-  private class HeaderBulkWriteListLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
-  {
-    Tuple item;
-
-    HeaderBulkWriteListLiteral() {
-      exists(List list | DataFlow::localFlow(DataFlow::exprNode(list), super.getBulkArg()) |
-        item = list.getAnElt()
-      )
-    }
-
-    override DataFlow::Node getNameArg() { result.asExpr() = item.getElt(0) }
-
-    override DataFlow::Node getValueArg() { result.asExpr() = item.getElt(1) }
-
-    override predicate nameAllowsNewline() {
-      Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
-    }
-
-    override predicate valueAllowsNewline() {
-      Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
-    }
-  }
-
   /**
    * A call to replace line breaks, considered as a sanitizer.
    */