Merge pull request github#3732 from erik-krogh/priv-file-polish

Approved by mchammer01

Author: semmle-qlci

change-notes/1.25/analysis-javascript.md

@@ -36,6 +36,7 @@
 | Incomplete HTML attribute sanitization (`js/incomplete-html-attribute-sanitization`) | security, external/cwe/cwe-20, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities due to incomplete sanitization of HTML meta-characters. Results are shown on LGTM by default. |
 | Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
 | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. |
+| Exposure of private files (`js/exposure-of-private-files`) | security, external/cwe/cwe-200 | Highlights servers that serve private files. Results are shown on LGTM by default. |
 | Creating biased random numbers from a cryptographically secure source (`js/biased-cryptographic-random`) | security, external/cwe/cwe-327 | Highlights mathematical operations on cryptographically secure numbers that can create biased results. Results are shown on LGTM by default. |
 | Storage of sensitive information in build artifact (`js/build-artifact-leak`) | security, external/cwe/cwe-312 | Highlights storage of sensitive information in build artifacts. Results are shown on LGTM by default. |
 | Improper code sanitization (`js/bad-code-sanitization`) | security, external/cwe/cwe-094, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights string concatenation where code is constructed without proper sanitization. Results are shown on LGTM by default. |

javascript/ql/src/Security/CWE-200/PrivateFileExposure.qhelp

@@ -5,20 +5,33 @@
 
 <overview>
   <p>
-    Placeholder
+    Libraries like <code>express</code> provide easy methods for serving entire 
+    directories of static files from a web server.
+    However, using these can sometimes lead to accidental information exposure. 
+    If for example the <code>node_modules</code> folder is served, then an attacker
+    can access the <code>_where</code> field from a <code>package.json</code> file, 
+    which gives access to the absolute path of the file. 
   </p>
 </overview>
 
 <recommendation>
   <p>
-    Placeholder
+    Limit which folders of static files are served from a web server.
   </p>
 </recommendation>
 
 <example>
   <p>
-    Placeholder
+    In the example below, all the files from the <code>node_modules</code> are served. 
+    This allows clients to easily access all the files inside that folder,
+    which includes potentially private information inside <code>package.json</code> files.
   </p>
+  <sample src="examples/PrivateFileExposure.js"/>
+  <p>
+    The issue has been fixed below by only serving specific folders within the
+    <code>node_modules</code> folder.
+  </p>
+  <sample src="examples/PrivateFileExposureFixed.js"/>
 </example>
 
 <references>

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

@@ -69,19 +69,33 @@ pragma[noinline]
 Folder getAPackageJSONFolder() { result = any(PackageJSON json).getFile().getParentContainer() }
 
 /**
- * Gets a reference to `dirname` that might cause information to be leaked.
- * That can happen if there is a `package.json` file in the same folder.
- * (It is assumed that the presence of a `package.json` file means that a `node_modules` folder can also exist.
+ * Gets a reference to `dirname`, the home folder, the current working folder, or the root folder.
+ * All of these might cause information to be leaked.
+ *
+ * For `dirname` that can happen if there is a `package.json` file in the same folder.
+ * It is assumed that the presence of a `package.json` file means that a `node_modules` folder can also exist.
+ *
+ * For the root/home/working folder, they contain so much information that they must leak information somehow (e.g. ssh keys in the `~/.ssh` folder).
  */
-DataFlow::Node dirname() {
+DataFlow::Node getALeakingFolder(string description) {
   exists(ModuleScope ms | result.asExpr() = ms.getVariable("__dirname").getAnAccess()) and
-  result.getFile().getParentContainer() = getAPackageJSONFolder()
+  result.getFile().getParentContainer() = getAPackageJSONFolder() and
+  description = "the folder " + result.getFile().getParentContainer().getRelativePath()
+  or
+  result = DataFlow::moduleImport("os").getAMemberCall("homedir") and
+  description = "the home folder "
+  or
+  result.mayHaveStringValue("/") and
+  description = "the root folder"
+  or
+  result.getStringValue() = [".", "./"] and
+  description = "the current working folder"
   or
-  result.getAPredecessor() = dirname()
+  result.getAPredecessor() = getALeakingFolder(description)
   or
   exists(StringOps::ConcatenationRoot root | root = result |
     root.getNumOperand() = 2 and
-    root.getOperand(0) = dirname() and
+    root.getOperand(0) = getALeakingFolder(description) and
     root.getOperand(1).getStringValue() = "/"
   )
 }
@@ -94,18 +108,17 @@ DataFlow::Node getAPrivateFolderPath(string description) {
     result = getANodeModulePath(path) and description = "the folder \"" + path + "\""
   )
   or
-  result = dirname() and
-  description = "the folder " + result.getFile().getParentContainer().getRelativePath()
-  or
-  result.getStringValue() = [".", "./"] and
-  description = "the current working folder"
+  result = getALeakingFolder(description)
 }
 
 /**
  * Gest a call that serves the folder `path` to the public.
  */
 DataFlow::CallNode servesAPrivateFolder(string description) {
-  result = DataFlow::moduleMember("express", "static").getACall() and
+  result = DataFlow::moduleMember(["express", "connect"], "static").getACall() and
+  result.getArgument(0) = getAPrivateFolderPath(description)
+  or
+  result = DataFlow::moduleImport("serve-static").getACall() and
   result.getArgument(0) = getAPrivateFolderPath(description)
 }
 

javascript/ql/src/Security/CWE-200/examples/PrivateFileExposure.js

@@ -0,0 +1,6 @@
+
+var express = require('express');
+
+var app = express();
+
+app.use('/node_modules', express.static(path.resolve(__dirname, '../node_modules')));

javascript/ql/src/Security/CWE-200/examples/PrivateFileExposureFixed.js

@@ -0,0 +1,7 @@
+
+var express = require('express');
+
+var app = express();
+
+app.use("jquery", express.static('./node_modules/jquery/dist'));
+app.use("bootstrap", express.static('./node_modules/bootstrap/dist'));

javascript/ql/test/query-tests/Security/CWE-200/PrivateFileExposure.expected

@@ -14,3 +14,8 @@
 | private-file-exposure.js:18:1:18:74 | app.use ... les"))) | Serves the folder "/node_modules", which can contain private information. |
 | private-file-exposure.js:19:1:19:88 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
 | private-file-exposure.js:22:1:22:58 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
+| private-file-exposure.js:40:1:40:88 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
+| private-file-exposure.js:41:1:41:97 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
+| private-file-exposure.js:42:1:42:66 | app.use ... dir())) | Serves the home folder , which can contain private information. |
+| private-file-exposure.js:43:1:43:46 | app.use ... )("/")) | Serves the root folder, which can contain private information. |
+| private-file-exposure.js:51:5:51:88 | app.use ... les'))) | Serves the folder "../node_modules", which can contain private information. |

javascript/ql/test/query-tests/Security/CWE-200/private-file-exposure.js

@@ -35,3 +35,28 @@ app.use('/js/', express.static('node_modules/bootstrap/dist/js'))
 app.use('/css/', express.static('node_modules/font-awesome/css'));
 app.use('basedir', express.static(__dirname)); // GOOD, because there is no package.json in the same folder.
 app.use('/monthly', express.static(__dirname + '/')); // GOOD, because there is no package.json in the same folder.
+
+const connect = require("connect");
+app.use('/angular', connect.static(path.join(__dirname, "/node_modules") + '/angular/')); // NOT OK
+app.use('/angular', require('serve-static')(path.join(__dirname, "/node_modules") + '/angular/')); // NOT OK
+app.use('/home', require('serve-static')(require("os").homedir())); // NOT OK
+app.use('/root', require('serve-static')("/")); // NOT OK
+
+// Bad documentation example
+function bad() {
+    var express = require('express');
+
+    var app = express();
+
+    app.use('/node_modules', express.static(path.resolve(__dirname, '../node_modules'))); // NOT OK
+}
+
+// Good documentation example
+function good() {
+    var express = require('express');
+
+    var app = express();
+
+    app.use("jquery", express.static('./node_modules/jquery/dist')); // OK
+    app.use("bootstrap", express.static('./node_modules/bootstrap/dist')); // OK
+}