JS: Factor RequestHeaderAccess into separate class

javascript/ql/src/Security/CWE-640/HostHeaderPoisoningInEmailGeneration.ql

@@ -14,7 +14,7 @@ class TaintedHostHeader extends TaintTracking::Configuration {
   TaintedHostHeader() { this = "TaintedHostHeader" }
 
   override predicate isSource(DataFlow::Node node) {
-    exists (HTTP::RequestInputAccess input | node = input |
+    exists (HTTP::RequestHeaderAccess input | node = input |
       input.getKind() = "header" and
       input.getAHeaderName() = "host")
   }

javascript/ql/src/semmle/javascript/frameworks/Express.qll

@@ -471,29 +471,14 @@ module Express {
           propName = "originalUrl"
         )
         or
-        exists (string methodName |
-          // `req.get(...)` or `req.header(...)`
-          kind = "header" and
-          this.(DataFlow::MethodCallNode).calls(request, methodName) |
-          methodName = "get" or
-          methodName = "header"
-        )
-        or
-        exists (DataFlow::PropRead headers |
-          // `req.headers.name`
-          kind = "header" and
-          headers.accesses(request, "headers") and
-          this = headers.getAPropertyRead())
-        or
-        exists (string propName | propName = "host" or propName = "hostname" |
-          // `req.host` and `req.hostname` are derived from headers
-          kind = "header" and
-          this.(DataFlow::PropRead).accesses(request, propName))
-        or
         // `req.cookies`
         kind = "cookie" and
         this.(DataFlow::PropRef).accesses(request, "cookies")
       )
+      or
+      exists (RequestHeaderAccess access | this = access |
+        rh = access.getRouteHandler() and
+        kind = "header")
     }
 
     override RouteHandler getRouteHandler() {
@@ -503,9 +488,35 @@ module Express {
     override string getKind() {
       result = kind
     }
+  }
+
+  /**
+   * An access to a header on an Express request.
+   */
+  private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
+    RouteHandler rh;
+
+    RequestHeaderAccess() {
+      exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
+        exists (string methodName |
+          // `req.get(...)` or `req.header(...)`
+          this.(DataFlow::MethodCallNode).calls(request, methodName) |
+          methodName = "get" or
+          methodName = "header"
+        )
+        or
+        exists (DataFlow::PropRead headers |
+          // `req.headers.name`
+          headers.accesses(request, "headers") and
+          this = headers.getAPropertyRead())
+        or
+        exists (string propName | propName = "host" or propName = "hostname" |
+          // `req.host` and `req.hostname` are derived from headers
+          this.(DataFlow::PropRead).accesses(request, propName))
+      )
+    }
 
     override string getAHeaderName() {
-      kind = "header" and
       exists (string name |
         name = this.(DataFlow::PropRead).getPropertyName()
         or
@@ -516,6 +527,14 @@ module Express {
         else
           result = name.toLowerCase())
     }
+
+    override RouteHandler getRouteHandler() {
+      result = rh
+    }
+
+    override string getKind() {
+      result = "header"
+    }
   }
 
   /**
@@ -613,9 +632,9 @@ module Express {
       astNode.getMethodName() = any(string n | n = "set" or n = "header") and
       astNode.getNumArgument() = 1
     }
-    
+
     /**
-     * Gets a reference to the multiple headers object that is to be set. 
+     * Gets a reference to the multiple headers object that is to be set.
      */
     private DataFlow::SourceNode getAHeaderSource() {
       result.flowsToExpr(astNode.getArgument(0))
@@ -631,12 +650,12 @@ module Express {
     override RouteHandler getRouteHandler() {
       result = rh
     }
-    
+
     override Expr getNameExpr() {
-      exists (DataFlow::PropWrite write  | 
+      exists (DataFlow::PropWrite write  |
         getAHeaderSource().flowsTo(write.getBase()) and
         result = write.getPropertyNameExpr()
-      )      
+      )
     }
   }
 

javascript/ql/src/semmle/javascript/frameworks/HTTP.qll

@@ -72,9 +72,9 @@ module HTTP {
      * Holds if the header with (lower-case) name `headerName` is set to the value of `headerValue`.
      */
     abstract predicate definesExplicitly(string headerName, Expr headerValue);
-    
+
     /**
-     * Returns the expression used to compute the header name. 
+     * Returns the expression used to compute the header name.
      */
     abstract Expr getNameExpr();
   }
@@ -354,9 +354,9 @@ module HTTP {
         headerName = getNameExpr().getStringValue().toLowerCase() and
         headerValue = astNode.getArgument(1)
       }
-      
+
       override Expr getNameExpr() {
-      	 result = astNode.getArgument(0)
+         result = astNode.getArgument(0)
       }
 
     }
@@ -399,15 +399,19 @@ module HTTP {
      * Note that this predicate is functional.
      */
     abstract string getKind();
+  }
 
+  /**
+   * An access to a header on an incoming HTTP request.
+   */
+  abstract class RequestHeaderAccess extends RequestInputAccess {
     /**
      * Gets the lower-case name of an HTTP header from which this input is derived,
      * if this can be determined.
      *
-     * When the input is not derived from a header, or the header name is
-     * unknown, this has no result.
+     * When the name of the header is unknown, this has no result.
      */
-    string getAHeaderName() { none() }
+    abstract string getAHeaderName();
   }
 
   /**

javascript/ql/src/semmle/javascript/frameworks/Hapi.qll

@@ -121,13 +121,6 @@ module Hapi {
           this.asExpr().(PropAccess).accesses(url, "path")
         )
         or
-        exists (PropAccess headers |
-          // `request.headers.<name>`
-          kind = "header" and
-          headers.accesses(request, "headers")  and
-          this.asExpr().(PropAccess).accesses(headers, _)
-        )
-        or
         exists (PropAccess state |
           // `request.state.<name>`
           kind = "cookie" and
@@ -135,6 +128,10 @@ module Hapi {
           this.asExpr().(PropAccess).accesses(state, _)
         )
       )
+      or
+      exists (RequestHeaderAccess access | this = access |
+        rh = access.getRouteHandler() and
+        kind = "header")
     }
 
     override RouteHandler getRouteHandler() {
@@ -144,11 +141,35 @@ module Hapi {
     override string getKind() {
       result = kind
     }
+  }
+
+  /**
+   * An access to an HTTP header on a Hapi request.
+   */
+  private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
+    RouteHandler rh;
+
+    RequestHeaderAccess() {
+      exists (Expr request | request = rh.getARequestExpr() |
+        exists (PropAccess headers |
+          // `request.headers.<name>`
+          headers.accesses(request, "headers")  and
+          this.asExpr().(PropAccess).accesses(headers, _)
+        )
+      )
+    }
 
     override string getAHeaderName() {
-      kind = "header" and
       result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
     }
+
+    override RouteHandler getRouteHandler() {
+      result = rh
+    }
+
+    override string getKind() {
+      result = "header"
+    }
   }
 
   /**

javascript/ql/src/semmle/javascript/frameworks/Koa.qll

@@ -182,19 +182,6 @@ module Koa {
           propName = "originalUrl" or
           propName = "href"
         )
-        or
-        exists (string propName, PropAccess headers |
-          // `ctx.request.header.<name>`, `ctx.request.headers.<name>`
-          kind = "header" and
-          headers.accesses(request, propName) and
-          this.asExpr().(PropAccess).accesses(headers, _) |
-          propName = "header" or
-          propName = "headers"
-        )
-        or
-        // `ctx.request.get(<name>)`
-        kind = "header" and
-        this.asExpr().(MethodCallExpr).calls(request, "get")
       )
       or
       exists (PropAccess cookies |
@@ -203,6 +190,10 @@ module Koa {
         cookies.accesses(rh.getAContextExpr(), "cookies") and
         this.asExpr().(MethodCallExpr).calls(cookies, "get")
       )
+      or
+      exists (RequestHeaderAccess access | access = this |
+        rh = access.getRouteHandler() and
+        kind = "header")
     }
 
     override RouteHandler getRouteHandler() {
@@ -212,16 +203,43 @@ module Koa {
     override string getKind() {
       result = kind
     }
+  }
+
+  /**
+   * An access to an HTTP header on a Koa request.
+   */
+  private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
+    RouteHandler rh;
+
+    RequestHeaderAccess() {
+      exists (Expr request | request = rh.getARequestExpr() |
+        exists (string propName, PropAccess headers |
+          // `ctx.request.header.<name>`, `ctx.request.headers.<name>`
+          headers.accesses(request, propName) and
+          this.asExpr().(PropAccess).accesses(headers, _) |
+          propName = "header" or
+          propName = "headers"
+        )
+        or
+        // `ctx.request.get(<name>)`
+        this.asExpr().(MethodCallExpr).calls(request, "get")
+      )
+    }
 
     override string getAHeaderName() {
-      kind = "header" and
-      (
-        result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
-        or
-        exists (string name |
-          this.(DataFlow::CallNode).getArgument(0).mayHaveStringValue(name) and
-          result = name.toLowerCase())
-      )
+      result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
+      or
+      exists (string name |
+        this.(DataFlow::CallNode).getArgument(0).mayHaveStringValue(name) and
+        result = name.toLowerCase())
+    }
+
+    override RouteHandler getRouteHandler() {
+      result = rh
+    }
+
+    override string getKind() {
+      result = "header"
     }
   }
 

javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll

@@ -146,12 +146,16 @@ module NodeJSLib {
       kind = "url" and
       this.asExpr().(PropAccess).accesses(request, "url")
       or
-      exists (PropAccess headers, string name |
-        // `req.headers.<name>`
-        if name = "cookie" then kind = "cookie" else kind= "header" |
+      exists (PropAccess headers |
+        // `req.headers.cookie`
+        kind = "cookie" and
         headers.accesses(request, "headers") and
-        this.asExpr().(PropAccess).accesses(headers, name)
+        this.asExpr().(PropAccess).accesses(headers, "cookie")
       )
+      or
+      exists (RequestHeaderAccess access | this = access |
+        request = access.getRequest() and
+        kind = "header")
     }
 
     override RouteHandler getRouteHandler() {
@@ -161,11 +165,38 @@ module NodeJSLib {
     override string getKind() {
       result = kind
     }
+  }
+
+  /**
+   * An access to an HTTP header (other than "Cookie") on an incoming Node.js request object.
+   */
+  private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
+    RequestExpr request;
+
+    RequestHeaderAccess() {
+      exists (PropAccess headers, string name |
+        // `req.headers.<name>`
+        name != "cookie" and
+        headers.accesses(request, "headers") and
+        this.asExpr().(PropAccess).accesses(headers, name)
+      )
+    }
 
     override string getAHeaderName() {
-      kind = "header" and
       result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
     }
+
+    override RouteHandler getRouteHandler() {
+      result = request.getRouteHandler()
+    }
+
+    override string getKind() {
+      result = "header"
+    }
+
+    RequestExpr getRequest() {
+      result = request
+    }
   }
 
   class RouteSetup extends CallExpr, HTTP::Servers::StandardRouteSetup {
@@ -385,7 +416,7 @@ module NodeJSLib {
     }
 
   }
-  
+
   /**
    * A call to a method from module `child_process`.
    */
@@ -481,21 +512,21 @@ module NodeJSLib {
     }
 
   }
-  
+
   /**
    * A call to a method from module `vm`
    */
   class VmModuleMethodCall extends DataFlow::CallNode {
     string methodName;
-    
+
     VmModuleMethodCall() {
       this = DataFlow::moduleMember("vm", methodName).getACall()
     }
-    
+
     /**
      * Gets the code to be executed as part of this call.
      */
-    DataFlow::Node getACodeArgument() {      
+    DataFlow::Node getACodeArgument() {
       (
         methodName = "runInContext" or
         methodName = "runInNewContext" or
@@ -548,7 +579,7 @@ module NodeJSLib {
     }
 
   }
-  
+
   /**
    * A model of a URL request in the Node.js `http` library.
    */
@@ -574,7 +605,7 @@ module NodeJSLib {
     }
 
   }
-  
+
   /**
    * A data flow node that is the parameter of a result callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `https.request(url, (res) => {})`.
    */
@@ -584,12 +615,12 @@ module NodeJSLib {
         this = req.(DataFlow::MethodCallNode).getCallback(1).getParameter(0)
       )
     }
-    
+
     override string getSourceType() {
       result = "NodeJSClientRequest callback parameter"
     }
   }
-  
+
   /**
    * A data flow node that is the parameter of a data callback for an HTTP or HTTPS request made by a Node.js process, for example `body` in `http.request(url, (res) => {res.on('data', (body) => {})})`.
    */
@@ -601,20 +632,20 @@ module NodeJSLib {
         this = mcn.getCallback(1).getParameter(0)
       )
     }
-    
+
     override string getSourceType() {
       result = "http.request data parameter"
     }
   }
-  
-  
+
+
   /**
    * A data flow node that is registered as a callback for an HTTP or HTTPS request made by a Node.js process, for example the function `handler` in `http.request(url).on(message, handler)`.
    */
   class ClientRequestHandler extends DataFlow::FunctionNode {
     string handledEvent;
     NodeJSClientRequest clientRequest;
-    
+
     ClientRequestHandler() {
       exists(DataFlow::MethodCallNode mcn |
         clientRequest.getAMethodCall("on") = mcn and
@@ -622,14 +653,14 @@ module NodeJSLib {
         flowsTo(mcn.getArgument(1))
       )
     }
-    
+
     /**
      * Gets the name of an event this callback is registered for.
      */
     string getAHandledEvent() {
       result = handledEvent
     }
-    
+
     /**
      * Gets a request this callback is registered for.
      */
@@ -637,7 +668,7 @@ module NodeJSLib {
       result = clientRequest
     }
   }
-  
+
   /**
    * A data flow node that is the parameter of a response callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `http.request(url).on('response', (res) => {})`.
    */
@@ -648,12 +679,12 @@ module NodeJSLib {
         handler.getAHandledEvent() = "response"
       )
     }
-    
+
     override string getSourceType() {
       result = "NodeJSClientRequest response event"
     }
   }
-  
+
   /**
    * A data flow node that is the parameter of a data callback for an HTTP or HTTPS request made by a Node.js process, for example `chunk` in `http.request(url).on('response', (res) => {res.on('data', (chunk) => {})})`.
    */
@@ -665,12 +696,12 @@ module NodeJSLib {
         this = mcn.getCallback(1).getParameter(0)
       )
     }
-    
+
     override string getSourceType() {
       result = "NodeJSClientRequest data event"
     }
   }
-  
+
   /**
    * A data flow node that is a login callback for an HTTP or HTTPS request made by a Node.js process.
    */
@@ -679,7 +710,7 @@ module NodeJSLib {
       getAHandledEvent() = "login"
     }
   }
-  
+
   /**
    * A data flow node that is a parameter of a login callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `http.request(url).on('login', (res, callback) => {})`.
    */
@@ -689,12 +720,12 @@ module NodeJSLib {
         this = handler.getParameter(0)
       )
     }
-    
+
     override string getSourceType() {
       result = "NodeJSClientRequest login event"
     }
   }
-  
+
   /**
    * A data flow node that is the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `callback` in `http.request(url).on('login', (res, callback) => {})`.
    */
@@ -705,7 +736,7 @@ module NodeJSLib {
       )
     }
   }
-  
+
   /**
    * A data flow node that is the username passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `username` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`.
    */
@@ -715,14 +746,14 @@ module NodeJSLib {
         this = callback.getACall().getArgument(0).asExpr()
       )
     }
-    
+
     override string getCredentialsKind() {
       result = "Node.js http(s) client login username"
     }
   }
-  
+
   /**
-   * A data flow node that is the password passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `password` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`. 
+   * A data flow node that is the password passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `password` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`.
    */
   private class ClientRequestLoginPassword extends CredentialsExpr {
     ClientRequestLoginPassword() {
@@ -730,13 +761,13 @@ module NodeJSLib {
         this = callback.getACall().getArgument(1).asExpr()
       )
     }
-    
+
     override string getCredentialsKind() {
       result = "Node.js http(s) client login password"
     }
   }
 
-  
+
   /**
    * A data flow node that is the parameter of an error callback for an HTTP or HTTPS request made by a Node.js process, for example `err` in `http.request(url).on('error', (err) => {})`.
    */
@@ -747,7 +778,7 @@ module NodeJSLib {
         handler.getAHandledEvent() = "error"
       )
     }
-    
+
     override string getSourceType() {
       result = "NodeJSClientRequest error event"
     }

javascript/ql/test/library-tests/frameworks/Express/HeaderAccess.ql

@@ -1,4 +1,4 @@
 import javascript
 
-from HTTP::RequestInputAccess access
+from HTTP::RequestHeaderAccess access
 select access, access.getAHeaderName()

javascript/ql/test/library-tests/frameworks/NodeJSLib/HeaderAccess.ql

@@ -1,4 +1,4 @@
 import javascript
 
-from HTTP::RequestInputAccess access
+from HTTP::RequestHeaderAccess access
 select access, access.getAHeaderName()

javascript/ql/test/library-tests/frameworks/hapi/HeaderAccess.ql

@@ -1,4 +1,4 @@
 import javascript
 
-from HTTP::RequestInputAccess access
+from HTTP::RequestHeaderAccess access
 select access, access.getAHeaderName()

javascript/ql/test/library-tests/frameworks/koa/HeaderAccess.ql

@@ -1,4 +1,4 @@
 import javascript
 
-from HTTP::RequestInputAccess access
+from HTTP::RequestHeaderAccess access
 select access, access.getAHeaderName()