Merge pull request #6948 from ihsinme/ihsinme-patch-076

CPP: Add query for CWE-243 Creation of chroot Jail Without Changing Working Directory

Author: MathiasVP

cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.cpp

@@ -0,0 +1,24 @@
+...
+  chroot("/myFold/myTmp"); // BAD
+...
+  chdir("/myFold/myTmp"); // BAD
+...
+  int fd = open("/myFold/myTmp", O_RDONLY | O_DIRECTORY);
+  fchdir(fd); // BAD
+...
+  if (chdir("/myFold/myTmp") == -1) {
+    exit(-1);
+  }
+  if (chroot("/myFold/myTmp") == -1) {  // GOOD
+    exit(-1);
+  }
+...
+  if (chdir("/myFold/myTmp") == -1) { // GOOD
+    exit(-1);
+  }
+...
+  int fd = open("/myFold/myTmp", O_RDONLY | O_DIRECTORY);
+  if(fchdir(fd) == -1) { // GOOD
+    exit(-1);
+  }
+...

cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.qhelp

@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Working with changing directories, without checking the return value or pinning the directory, may not be safe. Requires the attention of developers.</p>
+
+</overview>
+
+<example>
+<p>The following example demonstrates erroneous and corrected work with changing working directories.</p>
+<sample src="IncorrectChangingWorkingDirectory.cpp" />
+
+</example>
+<references>
+
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail">POS05-C. Limit access to files by creating a jail.</a>
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql

@@ -0,0 +1,69 @@
+/**
+ * @name Find work with changing working directories, with security errors.
+ * @description Not validating the return value or pinning the directory can be unsafe.
+ * @kind problem
+ * @id cpp/work-with-changing-working-directories
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-243
+ *       external/cwe/cwe-252
+ */
+
+import cpp
+import semmle.code.cpp.commons.Exclusions
+
+/** Holds if a `fc` function call is available before or after a `chdir` function call. */
+predicate inExistsChdir(FunctionCall fcp) {
+  exists(FunctionCall fctmp |
+    (
+      fctmp.getTarget().hasGlobalOrStdName("chdir") or
+      fctmp.getTarget().hasGlobalOrStdName("fchdir")
+    ) and
+    (
+      fcp.getBasicBlock().getASuccessor*() = fctmp.getBasicBlock() or
+      fctmp.getBasicBlock().getASuccessor*() = fcp.getBasicBlock()
+    )
+  )
+}
+
+/** Holds if a `fc` function call is available before or after a function call containing a `chdir` call. */
+predicate outExistsChdir(FunctionCall fcp) {
+  exists(FunctionCall fctmp |
+    exists(FunctionCall fctmp2 |
+      (
+        fctmp2.getTarget().hasGlobalOrStdName("chdir") or
+        fctmp2.getTarget().hasGlobalOrStdName("fchdir")
+      ) and
+      // we are looking for a call containing calls chdir and fchdir
+      fctmp2.getEnclosingStmt().getParentStmt*() = fctmp.getTarget().getEntryPoint().getChildStmt*()
+    ) and
+    (
+      fcp.getBasicBlock().getASuccessor*() = fctmp.getBasicBlock() or
+      fctmp.getBasicBlock().getASuccessor*() = fcp.getBasicBlock()
+    )
+  )
+}
+
+from FunctionCall fc, string msg
+where
+  fc.getTarget().hasGlobalOrStdName("chroot") and
+  not inExistsChdir(fc) and
+  not outExistsChdir(fc) and
+  // in this section I want to exclude calls to functions containing chroot that have a direct path to chdir, or to a function containing chdir
+  exists(FunctionCall fctmp |
+    fc.getEnclosingStmt().getParentStmt*() = fctmp.getTarget().getEntryPoint().getChildStmt*() and
+    not inExistsChdir(fctmp) and
+    not outExistsChdir(fctmp)
+  ) and
+  msg = "Creation of 'chroot' jail without changing the working directory"
+  or
+  (
+    fc.getTarget().hasGlobalOrStdName("chdir") or
+    fc.getTarget().hasGlobalOrStdName("fchdir")
+  ) and
+  fc instanceof ExprInVoidContext and
+  not isFromMacroDefinition(fc) and
+  msg = "Unchecked return value for call to '" + fc.getTarget().getName() + "'."
+select fc, msg

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-243/semmle/tests/IncorrectChangingWorkingDirectory.expected

@@ -0,0 +1,2 @@
+| test.cpp:12:7:12:12 | call to chroot | Creation of 'chroot' jail without changing the working directory |
+| test.cpp:29:3:29:7 | call to chdir | Unchecked return value for call to 'chdir'. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-243/semmle/tests/IncorrectChangingWorkingDirectory.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-243/semmle/tests/test.cpp

@@ -0,0 +1,46 @@
+typedef int FILE;
+#define size_t int
+size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
+FILE *fopen(const char *filename, const char *mode);
+int fread(char *buf, int size, int count, FILE *fp);
+int fclose(FILE *fp);
+int chroot(char *path);
+int chdir(char *path);
+void exit(int status);
+
+int funTest1(){
+  if (chroot("/myFold/myTmp") == -1) {  // BAD
+    exit(-1);
+  }
+  return 0;
+}
+
+int funTest2(){  
+  if (chdir("/myFold/myTmp") == -1) { // GOOD
+    exit(-1);
+  }
+  if (chroot("/myFold/myTmp") == -1) {  // GOOD
+    exit(-1);
+  }
+  return 0;
+}
+
+int funTest3(){  
+  chdir("/myFold/myTmp"); // BAD
+  return 0;
+}
+int main(int argc, char *argv[])
+{
+  if(argc = 0) {
+    funTest3();
+    return 2;
+  }
+  if(argc = 1)
+    funTest1();
+  else
+    funTest2();
+  FILE *fp = fopen(argv[1], "w");
+  fwrite("12345", 5, 1, fp);
+  fclose(fp);
+  return 0;
+}