Merge pull request github#10077 from intrigus-lgtm/cpp/wexpand-commmand-injection

Add query for tainted `wordexp` calls.

Author: geoffw0

cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.c

@@ -0,0 +1,19 @@
+
+int main(int argc, char** argv) {
+  char *filePath = argv[2];
+
+  {
+    // BAD: the user-controlled string is injected
+    // directly into `wordexp` which performs command substitution
+
+    wordexp_t we;
+    wordexp(filePath, &we, 0);
+  }
+
+  {
+    // GOOD: command substitution is disabled
+
+    wordexp_t we;
+    wordexp(filePath, &we, WRDE_NOCMD);
+  }
+}

cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The code passes user input to <code>wordexp</code>. This leaves the code
+vulnerable to attack by command injection, because <code>wordexp</code> performs command substitution.
+Command substitution is a feature that replaces <code>$(command)</code> or <code>`command`</code> with the
+output of the given command, allowing the user to run arbitrary code on the system.
+</p>
+
+</overview>
+<recommendation>
+
+<p>When calling <code>wordexp</code>, pass the <code>WRDE_NOCMD</code> flag to prevent command substitution.</p>
+
+</recommendation>
+<example>
+<p>The following example passes a user-supplied file path to <code>wordexp</code> in two ways. The
+first way uses <code>wordexp</code> with no specified flags. As such, it is vulnerable to command
+injection.
+The second way uses <code>wordexp</code> with the <code>WRDE_NOCMD</code> flag. As such, no command substitution
+is performed, making this safe from command injection.</p>
+<sample src="WordexpTainted.c" />
+
+</example>
+<references>
+
+<li>CERT C Coding Standard:
+<a href="https://www.securecoding.cert.org/confluence/display/c/STR02-C.+Sanitize+data+passed+to+complex+subsystems">STR02-C.
+Sanitize data passed to complex subsystems</a>.</li>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+
+
+<!--  LocalWords: CWE STR
+ -->
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql

@@ -0,0 +1,57 @@
+/**
+ * @name Uncontrolled data used in `wordexp` command
+ * @description Using user-supplied data in a `wordexp` command, without
+ *              disabling command substitution, can make code vulnerable
+ *              to command injection.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/wordexp-injection
+ * @tags security
+ *       external/cwe/cwe-078
+ */
+
+import cpp
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import semmle.code.cpp.security.FlowSources
+import DataFlow::PathGraph
+
+/**
+ * The `wordexp` function, which can perform command substitution.
+ */
+private class WordexpFunction extends Function {
+  WordexpFunction() { hasGlobalName("wordexp") }
+}
+
+/**
+ * Holds if `fc` disables command substitution by containing `WRDE_NOCMD` as a flag argument.
+ */
+private predicate isCommandSubstitutionDisabled(FunctionCall fc) {
+  fc.getArgument(2).getValue().toInt().bitAnd(4) = 4
+  /* 4 = WRDE_NOCMD. Check whether the flag is set.  */
+}
+
+/**
+ * A configuration to track user-supplied data to the `wordexp` function.
+ */
+class WordexpTaintConfiguration extends TaintTracking::Configuration {
+  WordexpTaintConfiguration() { this = "WordexpTaintConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof FlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(FunctionCall fc | fc.getTarget() instanceof WordexpFunction |
+      fc.getArgument(0) = sink.asExpr() and
+      not isCommandSubstitutionDisabled(fc)
+    )
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node.asExpr().getUnspecifiedType() instanceof IntegralType
+  }
+}
+
+from WordexpTaintConfiguration conf, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode
+where conf.hasFlowPath(sourceNode, sinkNode)
+select sinkNode.getNode(), sourceNode, sinkNode,
+  "Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection."

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/WordexpTainted.expected

@@ -0,0 +1,11 @@
+edges
+| test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | (const char *)... |
+| test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | filePath |
+nodes
+| test.cpp:23:20:23:23 | argv | semmle.label | argv |
+| test.cpp:29:13:29:20 | (const char *)... | semmle.label | (const char *)... |
+| test.cpp:29:13:29:20 | filePath | semmle.label | filePath |
+subpaths
+#select
+| test.cpp:29:13:29:20 | (const char *)... | test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | (const char *)... | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. |
+| test.cpp:29:13:29:20 | filePath | test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | filePath | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/WordexpTainted.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-078/WordexpTainted.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/test.cpp

@@ -0,0 +1,45 @@
+#ifdef _MSC_VER
+#define restrict __restrict
+#else
+#define restrict __restrict__
+#endif
+
+typedef unsigned long size_t;
+
+typedef struct {
+    size_t we_wordc;
+    char **we_wordv;
+    size_t we_offs;
+} wordexp_t;
+
+enum {
+    WRDE_APPEND = (1 << 1),
+    WRDE_NOCMD = (1 << 2)
+};
+
+int wordexp(const char *restrict s, wordexp_t *restrict p, int flags);
+
+int main(int argc, char** argv) {
+  char *filePath = argv[2];
+
+  {
+    // BAD: the user string is injected directly into `wordexp` which performs command substitution
+
+    wordexp_t we;
+    wordexp(filePath, &we, 0);
+  }
+
+  {
+    // GOOD: command substitution is disabled
+
+    wordexp_t we;
+    wordexp(filePath, &we, WRDE_NOCMD);
+  }
+
+  {
+    // GOOD: command substitution is disabled
+
+    wordexp_t we;
+    wordexp(filePath, &we, WRDE_NOCMD | WRDE_APPEND);
+  }
+}