Merge remote-tracking branch 'upstream/master' into typeAheadSink

Author: erik-krogh

change-notes/1.24/analysis-javascript.md

@@ -12,9 +12,9 @@
 
 ## New queries
 
-| **Query**                                                                 | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
-|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-
+| **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
+|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Cross-site scripting through exception (`js/xss-through-exception`) | security, external/cwe/cwe-079, external/cwe/cwe-116              | Highlights potential XSS vulnerabilities where an exception is written to the DOM. Results are not shown on LGTM by default. |
 
 ## Changes to existing queries
 
@@ -25,3 +25,6 @@
 | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | 
 
 ## Changes to libraries
+
+* The predicates `RegExpTerm.getSuccessor` and `RegExpTerm.getPredecessor` have been changed to reflect textual, not operational, matching order. This only makes a difference in lookbehind assertions, which are operationally matched backwards. Previously, `getSuccessor` would mimick this, so in an assertion `(?<=ab)` the term `b` would be considered the predecessor, not the successor, of `a`. Textually, however, `a` is still matched before `b`, and this is the order we now follow.
+

cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql

@@ -84,18 +84,18 @@ predicate hasZeroParamDecl(Function f) {
 }
 
 // True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(Function f) {
-  exists(File file | file.compiledAsC() |
-    file = f.getFile() or file.getAnIncludedFile+() = f.getFile()
-  )
+predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
 }
 
 from FunctionCall fc, Function f, Parameter p
 where
   f = fc.getTarget() and
   p = f.getAParameter() and
   hasZeroParamDecl(f) and
-  isCompiledAsC(f) and
+  isCompiledAsC(f.getFile()) and
   not f.isVarargs() and
   not f instanceof BuiltInFunction and
   p.getIndex() < fc.getNumberOfArguments() and

cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql

@@ -24,10 +24,10 @@ predicate hasZeroParamDecl(Function f) {
 }
 
 // True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(Function f) {
-  exists(File file | file.compiledAsC() |
-    file = f.getFile() or file.getAnIncludedFile+() = f.getFile()
-  )
+predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
 }
 
 from FunctionCall fc, Function f
@@ -36,7 +36,7 @@ where
   not f.isVarargs() and
   not f instanceof BuiltInFunction and
   hasZeroParamDecl(f) and
-  isCompiledAsC(f) and
+  isCompiledAsC(f.getFile()) and
   // There is an explicit declaration of the function whose parameter count is larger
   // than the number of call arguments
   exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |

cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql

@@ -25,18 +25,18 @@ predicate hasZeroParamDecl(Function f) {
 }
 
 // True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(Function f) {
-  exists(File file | file.compiledAsC() |
-    file = f.getFile() or file.getAnIncludedFile+() = f.getFile()
-  )
+predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
 }
 
 from FunctionCall fc, Function f
 where
   f = fc.getTarget() and
   not f.isVarargs() and
   hasZeroParamDecl(f) and
-  isCompiledAsC(f) and
+  isCompiledAsC(f.getFile()) and
   exists(f.getBlock()) and
   // There must not exist a declaration with the number of parameters
   // at least as large as the number of call arguments

javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

@@ -21,8 +21,10 @@
 import com.semmle.util.process.Env;
 import com.semmle.util.projectstructure.ProjectLayout;
 import com.semmle.util.trap.TrapWriter;
+import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
+import java.io.InputStreamReader;
 import java.io.Reader;
 import java.lang.ProcessBuilder.Redirect;
 import java.net.URI;
@@ -195,6 +197,11 @@ public class AutoBuild {
   private final String defaultEncoding;
   private ExecutorService threadPool;
   private volatile boolean seenCode = false;
+  private boolean installDependencies = false;
+  private int installDependenciesTimeout;
+
+  /** The default timeout when running <code>yarn</code>, in milliseconds. */
+  public static final int INSTALL_DEPENDENCIES_DEFAULT_TIMEOUT = 10 * 60 * 1000; // 10 minutes
 
   public AutoBuild() {
     this.LGTM_SRC = toRealPath(getPathFromEnvVar("LGTM_SRC"));
@@ -204,6 +211,11 @@ public AutoBuild() {
     this.typeScriptMode =
         getEnumFromEnvVar("LGTM_INDEX_TYPESCRIPT", TypeScriptMode.class, TypeScriptMode.FULL);
     this.defaultEncoding = getEnvVar("LGTM_INDEX_DEFAULT_ENCODING");
+    this.installDependencies = Boolean.valueOf(getEnvVar("LGTM_INDEX_TYPESCRIPT_INSTALL_DEPS"));
+    this.installDependenciesTimeout =
+        Env.systemEnv()
+            .getInt(
+                "LGTM_INDEX_TYPESCRIPT_INSTALL_DEPS_TIMEOUT", INSTALL_DEPENDENCIES_DEFAULT_TIMEOUT);
     setupFileTypes();
     setupXmlMode();
     setupMatchers();
@@ -533,6 +545,10 @@ private void extractSource() throws IOException {
     List<Path> tsconfigFiles = new ArrayList<>();
     findFilesToExtract(defaultExtractor, filesToExtract, tsconfigFiles);
 
+    if (!tsconfigFiles.isEmpty() && this.installDependencies) {
+      this.installDependencies(filesToExtract);
+    }
+
     // extract TypeScript projects and files
     Set<Path> extractedFiles = extractTypeScript(defaultExtractor, filesToExtract, tsconfigFiles);
 
@@ -549,6 +565,61 @@ private void extractSource() throws IOException {
     }
   }
 
+  /** Returns true if yarn is installed, otherwise prints a warning and returns false. */
+  private boolean verifyYarnInstallation() {
+    ProcessBuilder pb = new ProcessBuilder(Arrays.asList("yarn", "-v"));
+    try {
+      Process process = pb.start();
+      boolean completed = process.waitFor(this.installDependenciesTimeout, TimeUnit.MILLISECONDS);
+      if (!completed) {
+        System.err.println("Yarn could not be launched. Timeout during 'yarn -v'.");
+        return false;
+      }
+      BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
+      String version = reader.readLine();
+      System.out.println("Found yarn version: " + version);
+      return true;
+    } catch (IOException | InterruptedException ex) {
+      System.err.println(
+          "Yarn not found. Please put 'yarn' on the PATH for automatic dependency installation.");
+      Exceptions.ignore(ex, "Continue without dependency installation");
+      return false;
+    }
+  }
+
+  protected void installDependencies(Set<Path> filesToExtract) {
+    if (!verifyYarnInstallation()) {
+      return;
+    }
+    for (Path file : filesToExtract) {
+      if (file.getFileName().toString().equals("package.json")) {
+        System.out.println("Installing dependencies from " + file);
+        ProcessBuilder pb =
+            new ProcessBuilder(
+                Arrays.asList(
+                    "yarn",
+                    "install",
+                    "--verbose",
+                    "--non-interactive",
+                    "--ignore-scripts",
+                    "--ignore-platform",
+                    "--ignore-engines",
+                    "--ignore-optional",
+                    "--no-default-rc",
+                    "--no-bin-links",
+                    "--pure-lockfile"));
+        pb.directory(file.getParent().toFile());
+        pb.redirectOutput(Redirect.INHERIT);
+        pb.redirectError(Redirect.INHERIT);
+        try {
+          pb.start().waitFor(this.installDependenciesTimeout, TimeUnit.MILLISECONDS);
+        } catch (IOException | InterruptedException ex) {
+          throw new ResourceError("Could not install dependencies from " + file, ex);
+        }
+      }
+    }
+  }
+
   private ExtractorConfig mkExtractorConfig() {
     ExtractorConfig config = new ExtractorConfig(true);
     config = config.withSourceType(getSourceType());

javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java

@@ -128,6 +128,11 @@ public void extractTypeScriptFiles(
           }
         }
 
+        @Override
+        protected void installDependencies(Set<Path> filesToExtract) {
+          // never install dependencies during testing
+        }
+
         @Override
         protected void extractXml() throws IOException {
           Files.walkFileTree(

javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp

@@ -0,0 +1,54 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Directly writing exceptions to a webpage without sanitization allows for a cross-site scripting
+vulnerability if the value of the exception can be influenced by a user.
+</p>
+</overview>
+
+<recommendation>
+<p>
+To guard against cross-site scripting, consider using contextual output encoding/escaping before
+writing user input to the page, or one of the other solutions that are mentioned in the
+references.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows an exception being written directly to the document, 
+and this exception can potentially be influenced by the page URL, 
+leaving the website vulnerable to cross-site scripting.
+</p>
+<sample src="examples/ExceptionXss.js" />
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html">DOM based
+XSS Prevention Cheat Sheet</a>.
+</li>
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html">XSS
+(Cross Site Scripting) Prevention Cheat Sheet</a>.
+</li>
+<li>
+OWASP
+<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
+</li>
+<li>
+OWASP
+<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
+Scripting</a>.
+</li>
+<li>
+Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
+</li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-079/ExceptionXss.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Cross-site scripting through exception
+ * @description Inserting data from an exception containing user
+ *              input into the DOM may enable cross-site scripting.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision medium
+ * @id js/xss-through-exception
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.ExceptionXss::ExceptionXss
+import DataFlow::PathGraph
+
+from
+  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where
+  cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  sink.getNode().(Xss::Shared::Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),
+  "user-provided value"

javascript/ql/src/Security/CWE-079/examples/ExceptionXss.js

@@ -0,0 +1,10 @@
+function setLanguageOptions() {
+    var href = document.location.href,
+        deflt = href.substring(href.indexOf("default=")+8);
+    
+    try {
+        var parsed = unknownParseFunction(deflt); 
+    } catch(e) {
+        document.write("Had an error: " + e + ".");
+    }
+}

javascript/ql/src/semmle/javascript/Expr.qll

@@ -245,6 +245,21 @@ class Expr extends @expr, ExprOrStmt, ExprOrType, AST::ValueNode {
       ctx.(ConditionalExpr).inNullSensitiveContext()
     )
   }
+
+  /**
+   * Gets the data-flow node where exceptions thrown by this expression will
+   * propagate if this expression causes an exception to be thrown.
+   */
+  DataFlow::Node getExceptionTarget() {
+    if exists(this.getEnclosingStmt().getEnclosingTryCatchStmt())
+    then
+      result = DataFlow::parameterNode(this
+              .getEnclosingStmt()
+              .getEnclosingTryCatchStmt()
+              .getACatchClause()
+              .getAParameter())
+    else result = any(DataFlow::FunctionNode f | f.getFunction() = this.getContainer()).getExceptionalReturn()
+  }
 }
 
 /**