Add query for missing mode argument in open/openat calls

Author: jketema

cpp/ql/src/Security/CWE/CWE-732/FilePermissions.qll

@@ -83,16 +83,22 @@ abstract class FileCreationExpr extends FunctionCall {
   abstract int getMode();
 }
 
-class OpenCreationExpr extends FileCreationExpr {
+abstract class FileCreationWithOptionalModeExpr extends FileCreationExpr {
+  abstract predicate hasModeArgument();
+}
+
+class OpenCreationExpr extends FileCreationWithOptionalModeExpr {
   OpenCreationExpr() {
     this.getTarget().getName() = ["open", "_open", "_wopen"] and
     sets(this.getArgument(1).getValue().toInt(), o_creat())
   }
 
   override Expr getPath() { result = this.getArgument(0) }
 
+  override predicate hasModeArgument() { exists(this.getArgument(2)) }
+
   override int getMode() {
-    if exists(this.getArgument(2))
+    if hasModeArgument()
     then result = this.getArgument(2).getValue().toInt()
     else
       // assume anything is permitted
@@ -108,16 +114,18 @@ class CreatCreationExpr extends FileCreationExpr {
   override int getMode() { result = this.getArgument(1).getValue().toInt() }
 }
 
-class OpenatCreationExpr extends FileCreationExpr {
+class OpenatCreationExpr extends FileCreationWithOptionalModeExpr {
   OpenatCreationExpr() {
     this.getTarget().getName() = "openat" and
     sets(this.getArgument(2).getValue().toInt(), o_creat())
   }
 
   override Expr getPath() { result = this.getArgument(1) }
 
+  override predicate hasModeArgument() { exists(this.getArgument(3)) }
+
   override int getMode() {
-    if exists(this.getArgument(3))
+    if hasModeArgument()
     then result = this.getArgument(3).getValue().toInt()
     else
       // assume anything is permitted

cpp/ql/src/Security/CWE/CWE-732/OpenCallMissingModeArgument.c

@@ -0,0 +1,9 @@
+int open_file_bad() {
+	// BAD - this uses arbitrary bytes from the stack as mode argument
+        return open(FILE, O_CREAT)
+}
+
+int open_file_good() {
+	// GOOD - the mode argument is supplied
+        return open(FILE, O_CREAT, S_IRUSR | S_IWUSR)
+}

cpp/ql/src/Security/CWE/CWE-732/OpenCallMissingModeArgument.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+When opening a file with the <code>O_CREAT</code> flag, the <code>mode</code> must be supplied. If the
+<code>mode</code> argument is omitted, some arbitrary bytes from the stack will be used as the file mode.
+This leaks some bits from the stack into the permissions of the file.
+</p>
+</overview>
+
+<recommendation>
+<p>
+The <code>mode</code> must be supplied when <code>O_CREAT</code> is specified.
+</p>
+</recommendation>
+
+<example>
+<p>
+The first example opens a file with the <code>O_CREAT</code> flag without supplying the <code>mode</code>
+argument. In this case arbitrary bytes from the stack will be used as <code>mode</code> argument. The
+second example correctly supplies the <code>mode</code> argument and creates a file that is user readable
+and writable.
+</p>
+
+<sample src="OpenCallMissingModeArgument.c" />
+
+</example>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-732/OpenCallMissingModeArgument.ql

@@ -0,0 +1,19 @@
+/**
+ * @name File opened with O_CREAT flag but without mode argument
+ * @description Opening a file with the O_CREAT flag but without mode argument reads arbitrary bytes from the stack.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 7.8
+ * @precision medium
+ * @id cpp/open-call-with-mode-argument
+ * @tags security
+ *       external/cwe/cwe-732
+ */
+
+import cpp
+import FilePermissions
+
+from FileCreationWithOptionalModeExpr fc
+where not fc.hasModeArgument()
+select fc,
+  "A file is created here without providing a mode argument, which may leak bits from the stack"

cpp/ql/test/query-tests/Security/CWE/CWE-732/OpenCallMissingModeArgument.c

@@ -0,0 +1,16 @@
+typedef unsigned int mode_t;
+
+#define O_CREAT 0100
+
+int open(const char *pathname, int flags, ...);
+
+int openat(int dirfd, const char *pathname, int flags, ...);
+
+const char *a_file = "/a_file";
+
+void test_open() {
+  open(a_file, O_CREAT);
+  open(a_file, O_CREAT, 0);
+  openat(0, a_file, O_CREAT);
+  openat(0, a_file, O_CREAT, 0);
+}

cpp/ql/test/query-tests/Security/CWE/CWE-732/OpenCallMissingModeArgument.expected

@@ -0,0 +1,2 @@
+| OpenCallMissingModeArgument.c:12:3:12:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack |
+| OpenCallMissingModeArgument.c:14:3:14:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack |

cpp/ql/test/query-tests/Security/CWE/CWE-732/OpenCallMissingModeArgument.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-732/OpenCallMissingModeArgument.ql