Merge pull request #6949 from ihsinme/ihsinme-patch-073

CPP: Add query for CWE-266 Incorrect Privilege Assignment

Author: geoffw0

cpp/ql/src/experimental/Security/CWE/CWE-266/IncorrectPrivilegeAssignment.cpp

@@ -0,0 +1,16 @@
+...
+  umask(0); // BAD
+...
+  maskOut = S_IRWXG | S_IRWXO;
+  umask(maskOut); // GOOD
+  ...
+  fchmod(fileno(fp), 0555 - maskOut); // BAD 
+  ...
+  fchmod(fileno(fp), 0555 & ~maskOut); // GOOD
+...
+  umask(0666);
+  chmod(pathname, 0666); // BAD
+...
+  umask(0022);
+  chmod(pathname, 0666); // GOOD
+...

cpp/ql/src/experimental/Security/CWE/CWE-266/IncorrectPrivilegeAssignment.qhelp

@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Finding  for function calls that set file permissions that may have errors in use. Incorrect arithmetic for calculating the resolution mask, using the same mask in opposite functions, using a mask that is too wide.</p>
+
+</overview>
+
+<example>
+<p>The following example demonstrates erroneous and fixed ways to use functions.</p>
+<sample src="IncorrectPrivilegeAssignment.cpp" />
+
+</example>
+<references>
+
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/FIO06-C.+Create+files+with+appropriate+access+permissions">FIO06-C. Create files with appropriate access permissions</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-266/IncorrectPrivilegeAssignment.ql

@@ -0,0 +1,87 @@
+/**
+ * @name Find the wrong use of the umask function.
+ * @description Incorrectly evaluated argument to the umask function may have security implications.
+ * @kind problem
+ * @id cpp/wrong-use-of-the-umask
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       maintainability
+ *       security
+ *       external/cwe/cwe-266
+ *       external/cwe/cwe-264
+ *       external/cwe/cwe-200
+ *       external/cwe/cwe-560
+ *       external/cwe/cwe-687
+ */
+
+import cpp
+import semmle.code.cpp.exprs.BitwiseOperation
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/**
+ * An expression that is either a `BinaryArithmeticOperation` or the result of one or more `BinaryBitwiseOperation`s on a `BinaryArithmeticOperation`. For example `1 | (2 + 3)`.
+ */
+class ContainsArithmetic extends Expr {
+  ContainsArithmetic() {
+    this instanceof BinaryArithmeticOperation
+    or
+    // recursive search into `Operation`s
+    this.(BinaryBitwiseOperation).getAnOperand() instanceof ContainsArithmetic
+  }
+}
+
+/** Holds for a function `f` that has an argument at index `apos` used to set file permissions. */
+predicate numberArgumentModFunctions(Function f, int apos) {
+  f.hasGlobalOrStdName("umask") and apos = 0
+  or
+  f.hasGlobalOrStdName("fchmod") and apos = 1
+  or
+  f.hasGlobalOrStdName("chmod") and apos = 1
+}
+
+from FunctionCall fc, string msg, FunctionCall fcsnd
+where
+  fc.getTarget().hasGlobalOrStdName("umask") and
+  fc.getArgument(0).getValue() = "0" and
+  not exists(FunctionCall fctmp |
+    fctmp.getTarget().hasGlobalOrStdName("umask") and
+    not fctmp.getArgument(0).getValue() = "0"
+  ) and
+  exists(FunctionCall fctmp |
+    (
+      fctmp.getTarget().hasGlobalOrStdName("fopen") or
+      fctmp.getTarget().hasGlobalOrStdName("open")
+    ) and
+    not fctmp.getArgument(1).getValue().matches("r%") and
+    fctmp.getNumberOfArguments() = 2 and
+    not fctmp.getArgument(0).getValue() = "/dev/null" and
+    fcsnd = fctmp
+  ) and
+  not exists(FunctionCall fctmp |
+    fctmp.getTarget().hasGlobalOrStdName("chmod") or
+    fctmp.getTarget().hasGlobalOrStdName("fchmod")
+  ) and
+  msg = "Using umask(0) may not be safe with call $@."
+  or
+  fc.getTarget().hasGlobalOrStdName("umask") and
+  exists(FunctionCall fctmp |
+    (
+      fctmp.getTarget().hasGlobalOrStdName("chmod") or
+      fctmp.getTarget().hasGlobalOrStdName("fchmod")
+    ) and
+    (
+      globalValueNumber(fc.getArgument(0)) = globalValueNumber(fctmp.getArgument(1)) and
+      fc.getArgument(0).getValue() != "0"
+    ) and
+    msg = "Not use equal argument in umask and $@ functions." and
+    fcsnd = fctmp
+  )
+  or
+  exists(ContainsArithmetic exptmp, int i |
+    numberArgumentModFunctions(fc.getTarget(), i) and
+    globalValueNumber(exptmp) = globalValueNumber(fc.getArgument(i)) and
+    msg = "Using arithmetic to compute the mask in $@ may not be safe." and
+    fcsnd = fc
+  )
+select fc, msg, fcsnd, fcsnd.getTarget().getName()

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-266/semmle/tests/IncorrectPrivilegeAssignment.expected

@@ -0,0 +1,2 @@
+| test.cpp:9:3:9:7 | call to umask | Not use equal argument in umask and $@ functions. | test.cpp:13:3:13:7 | call to chmod | chmod |
+| test.cpp:30:3:30:7 | call to chmod | Using arithmetic to compute the mask in $@ may not be safe. | test.cpp:30:3:30:7 | call to chmod | chmod |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-266/semmle/tests/IncorrectPrivilegeAssignment.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-266/IncorrectPrivilegeAssignment.ql

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

@@ -0,0 +1,49 @@
+typedef int FILE;
+FILE *fopen(const char *filename, const char *mode);
+int umask(int pmode);
+int chmod(char * filename,int pmode);
+int fclose(FILE *stream);
+
+void funcTest1()
+{
+  umask(0666); // BAD
+  FILE *fe;
+  fe = fopen("myFile.txt", "wt");
+  fclose(fe);
+  chmod("myFile.txt",0666);
+}
+void funcTest1g()
+{
+  umask(0022);
+  FILE *fe;
+  fe = fopen("myFile.txt", "wt");
+  fclose(fe);
+  chmod("myFile.txt",0666); // GOOD
+}
+
+void funcTest2(int mode)
+{
+  umask(mode);
+  FILE *fe;
+  fe = fopen("myFile.txt", "wt");
+  fclose(fe);
+  chmod("myFile.txt",0555-mode); // BAD
+}
+
+void funcTest2g(int mode)
+{
+  umask(mode);
+  FILE *fe;
+  fe = fopen("myFile.txt", "wt");
+  fclose(fe);
+  chmod("myFile.txt",0555&~mode); // GOOD
+}
+
+int main(int argc, char *argv[])
+{
+  funcTest1();
+  funcTest2(27);
+  funcTest1g();
+  funcTest2g(27);
+  return 0;
+}