Merge pull request github#3765 from asger-semmle/js-team-sprint-merge2

JS: Merge js-team-sprint

Author: asgerf

change-notes/1.25/analysis-javascript.md

@@ -11,6 +11,7 @@
   - [jGrowl](https://github.com/stanlemon/jGrowl)
   - [jQuery](https://jquery.com/)
   - [marsdb](https://www.npmjs.com/package/marsdb)
+  - [micro](https://www.npmjs.com/package/micro/)
   - [minimongo](https://www.npmjs.com/package/minimongo/)
   - [mssql](https://www.npmjs.com/package/mssql)
   - [mysql](https://www.npmjs.com/package/mysql)
@@ -20,6 +21,7 @@
   - [sqlite](https://www.npmjs.com/package/sqlite)
   - [ssh2-streams](https://www.npmjs.com/package/ssh2-streams)
   - [ssh2](https://www.npmjs.com/package/ssh2)
+  - [yargs](https://www.npmjs.com/package/yargs)
   - [webpack-dev-server](https://www.npmjs.com/package/webpack-dev-server)
 
 * TypeScript 3.9 is now supported.
@@ -35,6 +37,13 @@
 | 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. |
+| Download of sensitive file through insecure connection (`js/insecure-download`) | security, external/cwe/cwe-829 | Highlights downloads of sensitive files through an unencrypted protocol. 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. |
+| Disabling certificate validation (`js/disabling-certificate-validation`) | security, external/cwe-295 | Highlights locations where SSL certificate validation is disabled. Results are shown on LGTM by default. | 
+| Incomplete multi-character sanitization (`js/incomplete-multi-character-sanitization`) | correctness, security, external/cwe/cwe-20, external/cwe/cwe-116 | Highlights sanitizers that fail to remove dangerous substrings completely. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

javascript/config/suites/javascript/security

@@ -13,27 +13,33 @@
 + semmlecode-javascript-queries/Security/CWE-078/ShellCommandInjectionFromEnvironment.ql: /Security/CWE/CWE-078
 + semmlecode-javascript-queries/Security/CWE-079/ReflectedXss.ql: /Security/CWE/CWE-079
 + semmlecode-javascript-queries/Security/CWE-079/StoredXss.ql: /Security/CWE/CWE-079
-+ semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079
 + semmlecode-javascript-queries/Security/CWE-079/UnsafeJQueryPlugin.ql: /Security/CWE/CWE-079
++ semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079
 + semmlecode-javascript-queries/Security/CWE-089/SqlInjection.ql: /Security/CWE/CWE-089
 + semmlecode-javascript-queries/Security/CWE-094/CodeInjection.ql: /Security/CWE/CWE-094
++ semmlecode-javascript-queries/Security/CWE-094/ImproperCodeSanitization.ql: /Security/CWE/CWE-094
 + semmlecode-javascript-queries/Security/CWE-094/UnsafeDynamicMethodAccess.ql: /Security/CWE/CWE-094
-+ semmlecode-javascript-queries/Security/CWE-116/IncompleteSanitization.ql: /Security/CWE/CWE-116
-+ semmlecode-javascript-queries/Security/CWE-116/IncompleteHtmlAttributeSanitization.ql: /Security/CWE/CWE-116
 + semmlecode-javascript-queries/Security/CWE-116/DoubleEscaping.ql: /Security/CWE/CWE-116
++ semmlecode-javascript-queries/Security/CWE-116/IncompleteHtmlAttributeSanitization.ql: /Security/CWE/CWE-116
++ semmlecode-javascript-queries/Security/CWE-116/IncompleteMultiCharacterSanitization.ql: /Security/CWE/CWE-116
++ semmlecode-javascript-queries/Security/CWE-116/IncompleteSanitization.ql: /Security/CWE/CWE-116
 + semmlecode-javascript-queries/Security/CWE-134/TaintedFormatString.ql: /Security/CWE/CWE-134
++ semmlecode-javascript-queries/Security/CWE-200/PrivateFileExposure.ql: /Security/CWE/CWE-200
 + semmlecode-javascript-queries/Security/CWE-201/PostMessageStar.ql: /Security/CWE/CWE-201
 + semmlecode-javascript-queries/Security/CWE-209/StackTraceExposure.ql: /Security/CWE/CWE-209
-+ semmlecode-javascript-queries/Security/CWE-312/CleartextStorage.ql: /Security/CWE/CWE-312
++ semmlecode-javascript-queries/Security/CWE-295/DisablingCertificateValidation.ql: /Security/CWE/CWE-295
++ semmlecode-javascript-queries/Security/CWE-312/BuildArtifactLeak.ql: /Security/CWE/CWE-312
 + semmlecode-javascript-queries/Security/CWE-312/CleartextLogging.ql: /Security/CWE/CWE-312
++ semmlecode-javascript-queries/Security/CWE-312/CleartextStorage.ql: /Security/CWE/CWE-312
 + semmlecode-javascript-queries/Security/CWE-313/PasswordInConfigurationFile.ql: /Security/CWE/CWE-313
++ semmlecode-javascript-queries/Security/CWE-327/BadRandomness.ql: /Security/CWE/CWE-327
 + semmlecode-javascript-queries/Security/CWE-327/BrokenCryptoAlgorithm.ql: /Security/CWE/CWE-327
 + semmlecode-javascript-queries/Security/CWE-338/InsecureRandomness.ql: /Security/CWE/CWE-338
 + semmlecode-javascript-queries/Security/CWE-346/CorsMisconfigurationForCredentials.ql: /Security/CWE/CWE-346
 + semmlecode-javascript-queries/Security/CWE-352/MissingCsrfMiddleware.ql: /Security/CWE/CWE-352
-+ semmlecode-javascript-queries/Security/CWE-400/RemotePropertyInjection.ql: /Security/CWE/CWE-400
 + semmlecode-javascript-queries/Security/CWE-400/PrototypePollution.ql: /Security/CWE/CWE-400
 + semmlecode-javascript-queries/Security/CWE-400/PrototypePollutionUtility.ql: /Security/CWE/CWE-400
++ semmlecode-javascript-queries/Security/CWE-400/RemotePropertyInjection.ql: /Security/CWE/CWE-400
 + semmlecode-javascript-queries/Security/CWE-502/UnsafeDeserialization.ql: /Security/CWE/CWE-502
 + semmlecode-javascript-queries/Security/CWE-506/HardcodedDataInterpretedAsCode.ql: /Security/CWE/CWE-506
 + semmlecode-javascript-queries/Security/CWE-601/ClientSideUrlRedirect.ql: /Security/CWE/CWE-601
@@ -48,6 +54,7 @@
 + semmlecode-javascript-queries/Security/CWE-798/HardcodedCredentials.ql: /Security/CWE/CWE-798
 + semmlecode-javascript-queries/Security/CWE-807/ConditionalBypass.ql: /Security/CWE/CWE-807
 + semmlecode-javascript-queries/Security/CWE-807/DifferentKindsComparisonBypass.ql: /Security/CWE/CWE-807
++ semmlecode-javascript-queries/Security/CWE-829/InsecureDownload.ql: /Security/CWE/CWE-829
 + semmlecode-javascript-queries/Security/CWE-834/LoopBoundInjection.ql: /Security/CWE/CWE-834
 + semmlecode-javascript-queries/Security/CWE-843/TypeConfusionThroughParameterTampering.ql: /Security/CWE/CWE-834
 + semmlecode-javascript-queries/Security/CWE-916/InsufficientPasswordHash.ql: /Security/CWE/CWE-916

javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.qhelp

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+  <overview>
+    <p>
+        Using string concatenation to construct JavaScript code can be error-prone, or in the worst
+        case, enable code injection if an input is constructed by an attacker.  
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+      If using <code>JSON.stringify</code> or a HTML sanitizer to sanitize a string inserted into 
+      JavaScript code, then make sure to perform additional sanitization or remove potentially
+      dangerous characters. 
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+      The example below constructs a function that assigns the number 42 to the property <code>key</code>
+      on an object <code>obj</code>. However, if <code>key</code> contains <code>&lt;/script&gt;</code>, then 
+      the generated code will break out of a <code>&lt;/script&gt;</code> if inserted into a 
+      <code>&lt;/script&gt;</code> tag.
+    </p>
+    <sample src="examples/ImproperCodeSanitization.js" />
+    <p>
+      The issue has been fixed by escaping potentially dangerous characters, as shown below.
+    </p>
+    <sample src="examples/ImproperCodeSanitizationFixed.js" />
+  </example>
+
+  <references>
+    <li>OWASP: <a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.</li>
+  </references>
+</qhelp>

javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql

@@ -0,0 +1,71 @@
+/**
+ * @name Improper code sanitization
+ * @description Escaping code as HTML does not provide protection against code injection.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/bad-code-sanitization
+ * @tags security
+ *       external/cwe/cwe-094
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.ImproperCodeSanitization::ImproperCodeSanitization
+import DataFlow::PathGraph
+private import semmle.javascript.heuristics.HeuristicSinks
+private import semmle.javascript.security.dataflow.CodeInjectionCustomizations
+
+/**
+ * Gets a type-tracked instance of `RemoteFlowSource` using type-tracker `t`.
+ */
+private DataFlow::Node remoteFlow(DataFlow::TypeTracker t) {
+  t.start() and
+  result instanceof RemoteFlowSource
+  or
+  exists(DataFlow::TypeTracker t2, DataFlow::Node prev | prev = remoteFlow(t2) |
+    t2 = t.smallstep(prev, result)
+    or
+    any(TaintTracking::AdditionalTaintStep dts).step(prev, result) and
+    t = t2
+  )
+}
+
+/**
+ * Gets a type-tracked reference to a `RemoteFlowSource`.
+ */
+private DataFlow::Node remoteFlow() { result = remoteFlow(DataFlow::TypeTracker::end()) }
+
+/**
+ * Gets a type-back-tracked instance of a code injection sink using type-tracker `t`.
+ */
+private DataFlow::Node endsInCodeInjectionSink(DataFlow::TypeBackTracker t) {
+  t.start() and
+  (
+    result instanceof CodeInjection::Sink
+    or
+    result instanceof HeuristicCodeInjectionSink and
+    not result instanceof StringOps::ConcatenationRoot // the heuristic CodeInjection sink looks for string-concats, we are not interrested in those here.
+  )
+  or
+  exists(DataFlow::TypeBackTracker t2 | t = t2.smallstep(result, endsInCodeInjectionSink(t2)))
+}
+
+/**
+ * Gets a reference to to a data-flow node that ends in a code injection sink.
+ */
+private DataFlow::Node endsInCodeInjectionSink() {
+  result = endsInCodeInjectionSink(DataFlow::TypeBackTracker::end())
+}
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where
+  cfg.hasFlowPath(source, sink) and
+  // Basic detection of duplicate results with `js/code-injection`.
+  not (
+    sink.getNode().(StringOps::ConcatenationLeaf).getRoot() = endsInCodeInjectionSink() and
+    remoteFlow() = source.getNode().(DataFlow::InvokeNode).getAnArgument()
+  )
+select sink.getNode(), source, sink, "$@ flows to here and is used to construct code.",
+  source.getNode(), "Improperly sanitized value"

javascript/ql/src/Security/CWE-094/examples/ImproperCodeSanitization.js

@@ -0,0 +1,4 @@
+function createObjectWrite() {
+    const assignment = `obj[${JSON.stringify(key)}]=42`;
+    return `(function(){${assignment}})` // NOT OK
+}

javascript/ql/src/Security/CWE-094/examples/ImproperCodeSanitizationFixed.js

@@ -0,0 +1,23 @@
+const charMap = {
+    '<': '\\u003C',
+    '>' : '\\u003E',
+    '/': '\\u002F',
+    '\\': '\\\\',
+    '\b': '\\b',
+    '\f': '\\f',
+    '\n': '\\n',
+    '\r': '\\r',
+    '\t': '\\t',
+    '\0': '\\0',
+    '\u2028': '\\u2028',
+    '\u2029': '\\u2029'
+};
+
+function escapeUnsafeChars(str) {
+    return str.replace(/[<>\b\f\n\r\t\0\u2028\u2029]/g, x => charMap[x])
+}
+
+function createObjectWrite() {
+    const assignment = `obj[${escapeUnsafeChars(JSON.stringify(key))}]=42`;
+    return `(function(){${assignment}})` // OK
+}

javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.qhelp

@@ -0,0 +1,8 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+	<include src="IncompleteSanitization.qhelp" />
+
+</qhelp>

javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.ql

@@ -0,0 +1,81 @@
+/**
+ * @name Incomplete multi-character sanitization
+ * @description A sanitizer that removes a sequence of characters may reintroduce the dangerous sequence.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/incomplete-multi-character-sanitization
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-116
+ *       external/cwe/cwe-20
+ */
+
+import javascript
+import semmle.javascript.security.IncompleteBlacklistSanitizer
+
+predicate isDangerous(RegExpTerm t) {
+  // path traversals
+  t.getAMatchedString() = ["..", "/..", "../"]
+  or
+  exists(RegExpTerm start |
+    start = t.(RegExpSequence).getAChild() and
+    start.getConstantValue() = "." and
+    start.getSuccessor().getConstantValue() = "." and
+    not [start.getPredecessor(), start.getSuccessor().getSuccessor()].getConstantValue() = "."
+  )
+  or
+  // HTML comments
+  t.getAMatchedString() = "<!--"
+  or
+  // HTML scripts
+  t.getAMatchedString().regexpMatch("(?i)<script.*")
+  or
+  exists(RegExpSequence seq | seq = t |
+    t.getChild(0).getConstantValue() = "<" and
+    // the `cript|scrip` case has been observed in the wild, not sure what the goal of that pattern is...
+    t
+        .getChild(0)
+        .getSuccessor+()
+        .getAMatchedString()
+        .regexpMatch("(?i)iframe|script|cript|scrip|style")
+  )
+  or
+  // HTML attributes
+  exists(string dangerousPrefix | dangerousPrefix = ["ng-", "on"] |
+    t.getAMatchedString().regexpMatch("(i?)" + dangerousPrefix + "[a-z]+")
+    or
+    exists(RegExpTerm start, RegExpTerm event | start = t.getAChild() |
+      start.getConstantValue().regexpMatch("(?i)[^a-z]*" + dangerousPrefix) and
+      event = start.getSuccessor() and
+      exists(RegExpTerm quantified | quantified = event.(RegExpQuantifier).getChild(0) |
+        quantified
+            .(RegExpCharacterClass)
+            .getAChild()
+            .(RegExpCharacterRange)
+            .isRange(["a", "A"], ["z", "Z"]) or
+        [quantified, quantified.(RegExpRange).getAChild()].(RegExpCharacterClassEscape).getValue() =
+          "w"
+      )
+    )
+  )
+}
+
+from StringReplaceCall replace, RegExpTerm regexp, RegExpTerm dangerous
+where
+  [replace.getRawReplacement(), replace.getCallback(1).getAReturn()].mayHaveStringValue("") and
+  replace.isGlobal() and
+  regexp = replace.getRegExp().getRoot() and
+  dangerous.getRootTerm() = regexp and
+  isDangerous(dangerous) and
+  // avoid anchored terms
+  not exists(RegExpAnchor a | a.getRootTerm() = regexp) and
+  // avoid flagging wrappers
+  not (
+    dangerous instanceof RegExpAlt or
+    dangerous instanceof RegExpGroup
+  ) and
+  // don't flag replace operations in a loop
+  not replace.getReceiver().getALocalSource() = replace
+select replace, "The replaced string may still contain a substring that starts matching at $@.",
+  dangerous, dangerous.toString()

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

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+<overview>
+  <p>
+    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>
+    Limit which folders of static files are served from a web server.
+  </p>
+</recommendation>
+
+<example>
+  <p>
+    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>
+  <li>OWASP: <a href="https://www.owasp.org/index.php/Top_10-2017_A3-Sensitive_Data_Exposure">Sensitive Data Exposure</a>.</li>
+</references>
+</qhelp>