Merge pull request github#11709 from github/jhelie/add-shell-command-injection

ATM: add boosted version for `ShellCommandInjectionFromEnvironment` query

Author: jhelie

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll

@@ -278,6 +278,28 @@ private class NosqlInjectionSinkCharacteristic extends EndpointCharacteristic {
   }
 }
 
+/**
+ * Endpoints identified as "ShellCommandInjectionFromEnvironmentSink" by the standard JavaScript libraries are
+ * ShellCommandInjectionFromEnvironment sinks with maximal confidence.
+ */
+private class ShellCommandInjectionFromEnvironmentSinkCharacteristic extends EndpointCharacteristic {
+  ShellCommandInjectionFromEnvironmentSinkCharacteristic() {
+    this = "ShellCommandInjectionFromEnvironmentSink"
+  }
+
+  override predicate appliesToEndpoint(DataFlow::Node n) {
+    n instanceof ShellCommandInjectionFromEnvironment::Sink
+  }
+
+  override predicate hasImplications(
+    EndpointType endpointClass, boolean isPositiveIndicator, float confidence
+  ) {
+    endpointClass instanceof ShellCommandInjectionFromEnvironmentSinkType and
+    isPositiveIndicator = true and
+    confidence = maximalConfidence()
+  }
+}
+
 /*
  * Characteristics that are indicative of not being a sink of any type, and have historically been used to select
  * negative samples for training.

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointTypes.qll

@@ -10,7 +10,8 @@ newtype TEndpointType =
   TXssSinkType() or
   TNosqlInjectionSinkType() or
   TSqlInjectionSinkType() or
-  TTaintedPathSinkType()
+  TTaintedPathSinkType() or
+  TShellCommandInjectionFromEnvironmentSinkType()
 
 /** A class that can be predicted by endpoint scoring models. */
 abstract class EndpointType extends TEndpointType {
@@ -60,3 +61,11 @@ class TaintedPathSinkType extends EndpointType, TTaintedPathSinkType {
 
   override int getEncoding() { result = 4 }
 }
+
+/** The `ShellCommandInjectionFromEnvironmentSink` class that can be predicted by endpoint scoring models. */
+class ShellCommandInjectionFromEnvironmentSinkType extends EndpointType,
+  TShellCommandInjectionFromEnvironmentSinkType {
+  override string getDescription() { result = "ShellCommandInjectionFromEnvironmentSink" }
+
+  override int getEncoding() { result = 5 }
+}

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ShellCommandInjectionFromEnvironmentATM.qll

@@ -0,0 +1,30 @@
+/**
+ * For internal use only.
+ *
+ * A taint-tracking configuration for reasoning about command-injection
+ * vulnerabilities.
+ * Defines shared code used by the ShellCommandInjectionFromEnvironment boosted query.
+ */
+
+private import semmle.javascript.heuristics.SyntacticHeuristics
+private import semmle.javascript.security.dataflow.ShellCommandInjectionFromEnvironmentCustomizations::ShellCommandInjectionFromEnvironment as ShellCommandInjectionFromEnvironment
+import AdaptiveThreatModeling
+
+class ShellCommandInjectionFromEnvironmentAtmConfig extends AtmConfig {
+  ShellCommandInjectionFromEnvironmentAtmConfig() {
+    this = "ShellCommandInjectionFromEnvironmentAtmConfig"
+  }
+
+  override predicate isKnownSource(DataFlow::Node source) {
+    source instanceof ShellCommandInjectionFromEnvironment::Source
+  }
+
+  override EndpointType getASinkEndpointType() {
+    result instanceof ShellCommandInjectionFromEnvironmentSinkType
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    super.isSanitizer(node) or
+    node instanceof ShellCommandInjectionFromEnvironment::Sanitizer
+  }
+}

javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql

@@ -17,6 +17,7 @@ private import experimental.adaptivethreatmodeling.SqlInjectionATM as SqlInjecti
 private import experimental.adaptivethreatmodeling.TaintedPathATM as TaintedPathAtm
 private import experimental.adaptivethreatmodeling.XssATM as XssAtm
 private import experimental.adaptivethreatmodeling.XssThroughDomATM as XssThroughDomAtm
+private import experimental.adaptivethreatmodeling.ShellCommandInjectionFromEnvironmentATM as ShellCommandInjectionFromEnvironmentAtm
 
 string getAReasonSinkExcluded(DataFlow::Node sinkCandidate, Query query) {
   query instanceof NosqlInjectionQuery and
@@ -33,6 +34,11 @@ string getAReasonSinkExcluded(DataFlow::Node sinkCandidate, Query query) {
   or
   query instanceof XssThroughDomQuery and
   result = any(XssThroughDomAtm::XssThroughDomAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate)
+  or
+  query instanceof ShellCommandInjectionFromEnvironmentQuery and
+  result =
+    any(ShellCommandInjectionFromEnvironmentAtm::ShellCommandInjectionFromEnvironmentAtmConfig cfg)
+        .getAReasonSinkExcluded(sinkCandidate)
 }
 
 pragma[inline]

javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll

@@ -15,6 +15,7 @@ private import experimental.adaptivethreatmodeling.SqlInjectionATM as SqlInjecti
 private import experimental.adaptivethreatmodeling.TaintedPathATM as TaintedPathAtm
 private import experimental.adaptivethreatmodeling.XssATM as XssAtm
 private import experimental.adaptivethreatmodeling.XssThroughDomATM as XssThroughDomAtm
+private import experimental.adaptivethreatmodeling.ShellCommandInjectionFromEnvironmentATM as ShellCommandInjectionFromEnvironmentAtm
 
 /**
  * Gets the set of featureName-featureValue pairs for each endpoint in the training set.
@@ -217,6 +218,10 @@ DataFlow::Configuration getDataFlowCfg(Query query) {
   query instanceof XssQuery and result instanceof XssAtm::DomBasedXssAtmConfig
   or
   query instanceof XssThroughDomQuery and result instanceof XssThroughDomAtm::XssThroughDomAtmConfig
+  or
+  query instanceof ShellCommandInjectionFromEnvironmentQuery and
+  result instanceof
+    ShellCommandInjectionFromEnvironmentAtm::ShellCommandInjectionFromEnvironmentAtmConfig
 }
 
 // TODO: Delete this once we are no longer surfacing `hasFlowFromSource`.

javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql

@@ -9,6 +9,7 @@ import experimental.adaptivethreatmodeling.NosqlInjectionATM as NosqlInjectionAt
 import experimental.adaptivethreatmodeling.TaintedPathATM as TaintedPathAtm
 import experimental.adaptivethreatmodeling.XssATM as XssAtm
 import experimental.adaptivethreatmodeling.XssThroughDomATM as XssThroughDomAtm
+import experimental.adaptivethreatmodeling.ShellCommandInjectionFromEnvironmentATM as ShellCommandInjectionFromEnvironmentAtm
 import experimental.adaptivethreatmodeling.AdaptiveThreatModeling
 
 from string queryName, AtmConfig c, EndpointType e
@@ -26,6 +27,10 @@ where
     queryName = "Xss" and c instanceof XssAtm::DomBasedXssAtmConfig
     or
     queryName = "XssThroughDom" and c instanceof XssThroughDomAtm::XssThroughDomAtmConfig
+    or
+    queryName = "ShellCommandInjectionFromEnvironment" and
+    c instanceof
+      ShellCommandInjectionFromEnvironmentAtm::ShellCommandInjectionFromEnvironmentAtmConfig
   ) and
   e = c.getASinkEndpointType()
 select queryName, e.getEncoding() as label

javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/Queries.qll

@@ -9,7 +9,8 @@ newtype TQuery =
   TSqlInjectionQuery() or
   TTaintedPathQuery() or
   TXssQuery() or
-  TXssThroughDomQuery()
+  TXssThroughDomQuery() or
+  TShellCommandInjectionFromEnvironmentQuery()
 
 abstract class Query extends TQuery {
   abstract string getName();
@@ -36,3 +37,8 @@ class XssQuery extends Query, TXssQuery {
 class XssThroughDomQuery extends Query, TXssThroughDomQuery {
   override string getName() { result = "XssThroughDom" }
 }
+
+class ShellCommandInjectionFromEnvironmentQuery extends Query,
+  TShellCommandInjectionFromEnvironmentQuery {
+  override string getName() { result = "ShellCommandInjectionFromEnvironment" }
+}

javascript/ql/experimental/adaptivethreatmodeling/src/ShellCommandInjectionFromEnvironmentATM.md

@@ -0,0 +1,75 @@
+
+# Shell command built from environment values (experimental)
+
+Dynamically constructing a shell command with values from the
+local environment, such as file paths, may inadvertently
+change the meaning of the shell command.
+
+Such changes can occur when an environment value contains
+characters that the shell interprets in a special way, for instance
+quotes and spaces.
+
+This can result in the shell command misbehaving, or even
+allowing a malicious user to execute arbitrary commands on the system.
+
+Note: This CodeQL query is an experimental query. Experimental queries generate alerts using machine learning. They might include more false positives but they will improve over time.
+
+## Recommendation
+
+If possible, use hard-coded string literals to specify the
+shell command to run, and provide the dynamic arguments to the shell
+command separately to avoid interpretation by the shell.
+
+Alternatively, if the shell command must be constructed
+dynamically, then add code to ensure that special characters in
+environment values do not alter the shell command unexpectedly.
+
+## Example
+
+The following example shows a dynamically constructed shell
+command that recursively removes a temporary directory that is located
+next to the currently executing JavaScript file. Such utilities are
+often found in custom build scripts.
+
+```javascript
+var cp = require("child_process"),
+  path = require("path");
+function cleanupTemp() {
+  let cmd = "rm -rf " + path.join(__dirname, "temp");
+  cp.execSync(cmd); // BAD
+}
+```
+
+The shell command will, however, fail to work as intended if the
+absolute path of the script's directory contains spaces. In that
+case, the shell command will interpret the absolute path as multiple
+paths, instead of a single path.
+
+For instance, if the absolute path of
+the temporary directory is "`/home/username/important project/temp`", then the shell command will recursively delete
+`"/home/username/important"` and `"project/temp"`,
+where the latter path gets resolved relative to the working directory
+of the JavaScript process.
+
+Even worse, although less likely, a malicious user could
+provide the path `"/home/username/; cat /etc/passwd #/important
+project/temp"` in order to execute the command `"cat
+/etc/passwd"`.
+
+To avoid such potentially catastrophic behaviors, provide the
+directory as an argument that does not get interpreted by a
+shell:
+
+```javascript
+var cp = require("child_process"),
+  path = require("path");
+function cleanupTemp() {
+  let cmd = "rm",
+    args = ["-rf", path.join(__dirname, "temp")];
+  cp.execFileSync(cmd, args); // GOOD
+}
+```
+
+
+## References
+* OWASP: [Command Injection](https://www.owasp.org/index.php/Command_Injection)

javascript/ql/experimental/adaptivethreatmodeling/src/ShellCommandInjectionFromEnvironmentATM.ql

@@ -0,0 +1,29 @@
+/**
+ * For internal use only.
+ *
+ * @name Shell command built from environment values
+ * @description Building a shell command string with values from the enclosing
+ *              environment may cause subtle bugs or vulnerabilities.
+ * @kind path-problem
+ * @scored
+ * @problem.severity warning
+ * @security-severity 6.3
+ * @precision high
+ * @id js/ml-powered/shell-command-injection-from-environment
+ * @tags experimental security
+ *       correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ */
+
+import javascript
+import experimental.adaptivethreatmodeling.ShellCommandInjectionFromEnvironmentATM
+import ATM::ResultsInfo
+import DataFlow::PathGraph
+
+from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score
+where cfg.hasBoostedFlowPath(source, sink, score)
+select sink.getNode(), source, sink,
+  "(Experimental) This shell command depends on $@. Identified using machine learning.",
+  source.getNode(), "an uncontrolled value", score

javascript/ql/experimental/adaptivethreatmodeling/src/XssThroughDomATM.md

@@ -0,0 +1,48 @@
+# DOM text reinterpreted as HTML (experimental)
+
+Extracting text from a DOM node and interpreting it as HTML can lead to a cross-site scripting vulnerability.
+
+A webpage with this vulnerability reads text from the DOM, and afterwards adds the text as HTML to the DOM. Using text from the DOM as HTML effectively unescapes the text, and thereby invalidates any escaping done on the text. If an attacker is able to control the safe sanitized text, then this vulnerability can be exploited to perform a cross-site scripting attack. 
+
+Note: This CodeQL query is an experimental query. Experimental queries generate alerts using machine learning. They might include more false positives but they will improve over time.
+
+## Recommendation
+
+To guard against cross-site scripting, consider using contextual output encoding/escaping before writing text to the page, or one of the other solutions that are mentioned in the References section below.
+
+## Example
+
+The following example shows a webpage using a `data-target` attribute 
+to select and manipulate a DOM element using the JQuery library. In the example, the 
+`data-target` attribute is read into the `target` variable, and the 
+`$` function is then supposed to use the `target` variable as a CSS 
+selector to determine which element should be manipulated.
+
+```javascript
+$("button").click(function () {
+    var target = $(this).attr("data-target");
+    $(target).hide();
+});
+```
+
+However, if an attacker can control the `data-target` attribute, 
+then the value of `target` can be used to cause the `$` function
+to execute arbitrary JavaScript. 
+
+The above vulnerability can be fixed by using `$.find` instead of `$`. 
+The `$.find` function will only interpret `target` as a CSS selector 
+and never as HTML, thereby preventing an XSS attack. 
+
+```javascript
+$("button").click(function () {
+    var target = $(this).attr("data-target");
+	$.find(target).hide();
+});
+```
+
+## References
+* OWASP: [DOM based XSS Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html)
+* OWASP: [(Cross Site Scripting) Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html)
+* OWASP [DOM Based XSS](https://owasp.org/www-community/attacks/DOM_Based_XSS)
+* OWASP [Types of Cross-Site Scripting](https://owasp.org/www-community/Types_of_Cross-Site_Scripting)
+* Wikipedia: [Cross-site scripting](http://en.wikipedia.org/wiki/Cross-site_scripting)