Merge pull request #300 from esben-semmle/js/http-file-access-polish

Approved by asger-semmle

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -16,10 +16,12 @@
 | **Query**                                     | **Tags**                                             | **Purpose**                                                                                                                                                                 |
 |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094  | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
+| File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. |
 | Host header poisoning in email generation |  security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default.  |
 | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
 | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
 | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |
+| User-controlled data in file | security, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file. Results are not shown on LGTM by default. |
 
 ## Changes to existing queries
 

javascript/ql/src/Security/CWE-200/FileAccessToHttp.ql

@@ -1,6 +1,6 @@
 /**
- * @name File Access data flows to Http POST/PUT 
- * @description Writing data from file directly to http body or request header can be an indication to data exfiltration or unauthorized information disclosure.
+ * @name File data in outbound network request
+ * @description Directly sending file data in an outbound network request can indicate unauthorized information disclosure.
  * @kind problem
  * @problem.severity warning
  * @id js/file-access-to-http
@@ -11,6 +11,6 @@
 import javascript
 import semmle.javascript.security.dataflow.FileAccessToHttp
 
-from FileAccessToHttpDataFlow::Configuration config, DataFlow::Node src, DataFlow::Node sink
+from FileAccessToHttp::Configuration config, DataFlow::Node src, DataFlow::Node sink
 where config.hasFlow (src, sink)
-select src, "$@ flows directly to Http request body", sink, "File access"
+select sink, "$@ flows directly to outbound network request", src, "File data"

javascript/ql/src/Security/CWE-912/HttpToFileAccess.ql

@@ -1,6 +1,6 @@
 /**
- * @name Http response data flows to File Access 
- * @description Writing data from an HTTP request directly to the file system allows arbitrary file upload and might indicate a backdoor.
+ * @name User-controlled data written to file
+ * @description Writing user-controlled data directly to the file system allows arbitrary file upload and might indicate a backdoor.
  * @kind problem
  * @problem.severity warning
  * @id js/http-to-file-access
@@ -11,6 +11,6 @@
 import javascript
 import semmle.javascript.security.dataflow.HttpToFileAccess
 
-from HttpToFileAccessFlow::Configuration configuration, DataFlow::Node src, DataFlow::Node sink
+from HttpToFileAccess::Configuration configuration, DataFlow::Node src, DataFlow::Node sink
 where configuration.hasFlow(src, sink)
-select sink, "$@ flows to file system", src, "Untrusted data received from Http response"
+select sink, "$@ flows to file system", src, "Untrusted data"

javascript/ql/src/Security/CWE-918/RequestForgery.qhelp

@@ -25,8 +25,8 @@
 		<p>
 
 			To guard against request forgery, it is advisable to avoid
-			putting user input directly into a remote request. If a flexible
-			remote request mechanism is required, it is recommended to maintain a
+			putting user input directly into a network request. If a flexible
+			network request mechanism is required, it is recommended to maintain a
 			list of authorized request targets and choose from that list based on
 			the user input provided.
 

javascript/ql/src/Security/CWE-918/RequestForgery.ql

@@ -1,6 +1,6 @@
 /**
- * @name Uncontrolled data used in remote request
- * @description Sending remote requests with user-controlled data allows for request forgery attacks.
+ * @name Uncontrolled data used in network request
+ * @description Sending network requests with user-controlled data allows for request forgery attacks.
  * @kind problem
  * @problem.severity error
  * @precision medium

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -29,19 +29,27 @@ abstract class FileSystemAccess extends DataFlow::Node {
   /** Gets an argument to this file system access that is interpreted as a path. */
   abstract DataFlow::Node getAPathArgument();
 
-  /** Gets a node that represents file system access data, such as buffer the data is copied to. */
-  abstract DataFlow::Node getDataNode();
 }
 
 /**
- * A data flow node that performs read file system access.
+ * A data flow node that reads data from the file system.
  */
-abstract class FileSystemReadAccess extends FileSystemAccess { }
+abstract class FileSystemReadAccess extends FileSystemAccess {
+
+  /** Gets a node that represents data from the file system. */
+  abstract DataFlow::Node getADataNode();
+
+}
 
 /**
- * A data flow node that performs write file system access.
+ * A data flow node that writes data to the file system.
  */
-abstract class FileSystemWriteAccess extends FileSystemAccess { }
+abstract class FileSystemWriteAccess extends FileSystemAccess {
+
+  /** Gets a node that represents data to be written to the file system. */
+  abstract DataFlow::Node getADataNode();
+
+}
 
 /**
  * A data flow node that contains a file name or an array of file names from the local file system.

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

@@ -9,17 +9,31 @@ import javascript
 
 /**
  * A call that performs a request to a URL.
+ *
+ * Example: An HTTP POST request is a client request that sends some
+ * `data` to a `url`, where both the headers and the body of the request
+ * contribute to the `data`.
  */
 abstract class CustomClientRequest extends DataFlow::InvokeNode {
 
   /**
    * Gets the URL of the request.
    */
   abstract DataFlow::Node getUrl();
+
+  /**
+   * Gets a node that contributes to the data-part this request.
+   */
+  abstract DataFlow::Node getADataNode();
+
 }
 
 /**
  * A call that performs a request to a URL.
+ *
+ * Example: An HTTP POST request is client request that sends some
+ * `data` to a `url`, where both the headers and the body of the request
+ * contribute to the `data`.
  */
 class ClientRequest extends DataFlow::InvokeNode {
 
@@ -35,6 +49,14 @@ class ClientRequest extends DataFlow::InvokeNode {
   DataFlow::Node getUrl() {
     result = custom.getUrl()
   }
+
+  /**
+   * Gets a node that contributes to the data-part this request.
+   */
+  DataFlow::Node getADataNode() {
+    result = custom.getADataNode()
+  }
+
 }
 
 /**
@@ -83,6 +105,10 @@ private class RequestUrlRequest extends CustomClientRequest {
     result = url
   }
 
+  override DataFlow::Node getADataNode() {
+    result = getArgument(1)
+  }
+
 }
 
 /**
@@ -113,6 +139,10 @@ private class AxiosUrlRequest extends CustomClientRequest {
     result = url
   }
 
+  override DataFlow::Node getADataNode() {
+    none()
+  }
+
 }
 
 /**
@@ -144,6 +174,10 @@ private class FetchUrlRequest extends CustomClientRequest {
     result = url
   }
 
+  override DataFlow::Node getADataNode() {
+    none()
+  }
+
 }
 
 /**
@@ -169,6 +203,10 @@ private class GotUrlRequest extends CustomClientRequest {
     result = url
   }
 
+  override DataFlow::Node getADataNode() {
+    none()
+  }
+
 }
 
 /**
@@ -191,4 +229,8 @@ private class SuperAgentUrlRequest extends CustomClientRequest {
     result = url
   }
 
+  override DataFlow::Node getADataNode() {
+    none()
+  }
+
 }

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

@@ -63,6 +63,10 @@ module Electron {
       result = getOptionArgument(0, "url")
     }
 
+    override DataFlow::Node getADataNode() {
+      none()
+    }
+
   }
 
   /**
@@ -78,6 +82,10 @@ module Electron {
       result = getOptionArgument(0, "url")
     }
 
+    override DataFlow::Node getADataNode() {
+      none()
+    }
+
   }
 
 

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

@@ -824,16 +824,16 @@ module Express {
   }
 
   /** A call to `response.sendFile`, considered as a file system access. */
-  private class ResponseSendFileAsFileSystemAccess extends FileSystemAccess, DataFlow::ValueNode {
+  private class ResponseSendFileAsFileSystemAccess extends FileSystemReadAccess, DataFlow::ValueNode {
     override MethodCallExpr astNode;
 
     ResponseSendFileAsFileSystemAccess() {
       exists (string name | name = "sendFile" or name = "sendfile" |
         asExpr().(MethodCallExpr).calls(any(ResponseExpr res), name))
     }
 
-    override DataFlow::Node getDataNode() {
-      result = DataFlow::valueNode(astNode)
+    override DataFlow::Node getADataNode() {
+      none()
     }
 
     override DataFlow::Node getAPathArgument() {

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

@@ -132,11 +132,6 @@ module HTTP {
     result = "http" or result = "https"
   }
 
-  /**
-   * An expression whose value is sent as (part of) the body of an HTTP request (POST, PUT).
-   */
-  abstract class RequestBody extends DataFlow::Node {}
-  
   /**
    * An expression whose value is sent as (part of) the body of an HTTP response.
    */