Address review comments

Author: jketema

cpp/ql/lib/change-notes/2022-02-02-detect-creat-in-openat.md

@@ -1,4 +1,4 @@
 ---
 category: minorAnalysis
 ---
-* The `cpp/world-writable-file-creation` query now only detects `openat` calls with the `O_CREAT` flag, irrespective of whether the `mode` argument is present.
+* The `cpp/world-writable-file-creation` query now only detects `open` and `openat` calls with both the `O_CREAT` and `O_TMPFILE` flag, irrespective of whether the `mode` argument is present.

cpp/ql/lib/change-notes/2022-02-02-detect-missing-mode-argument-for-open.md

@@ -1,4 +1,4 @@
 ---
 category: newQuery
 ---
-* Added a new query, `open-call-with-mode-argument`, to detect when `open` or `openat` is called with the `O_CREAT` flag but when the `mode` argument is omitted.
+* Added a new query, `cpp/open-call-with-mode-argument`, to detect when `open` or `openat` is called with the `O_CREAT` or `O_TMPFILE` flag but when the `mode` argument is omitted.

cpp/ql/lib/semmle/code/cpp/commons/unix/Constants.qll

@@ -11,10 +11,10 @@ import cpp
  */
 bindingset[input]
 int parseOctal(string input) {
-  input.charAt(0) = "0" and
+  input.regexpMatch("0[0-7]+") and
   result =
-    strictsum(int ix |
-      ix in [0 .. input.length()]
+    sum(int ix |
+      ix in [1 .. input.length()]
     |
       8.pow(input.length() - (ix + 1)) * input.charAt(ix).toInt()
     )

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

@@ -1,27 +1,29 @@
 import cpp
-import semmle.code.cpp.commons.unix.Constants
+import semmle.code.cpp.commons.unix.Constants as UnixConstants
 
 bindingset[input]
 int parseHex(string input) {
-  input.matches("0x%") and
-  result =
-    strictsum(int ix |
-      ix in [2 .. input.length()]
-    |
-      16.pow(input.length() - (ix + 1)) * "0123456789abcdef".indexOf(input.charAt(ix).toLowerCase())
-    )
+  exists(string lowerCaseInput | lowerCaseInput = input.toLowerCase() |
+    lowerCaseInput.regexpMatch("0x[0-9a-f]+") and
+    result =
+      sum(int ix |
+        ix in [2 .. input.length()]
+      |
+        16.pow(input.length() - (ix + 1)) * "0123456789abcdef".indexOf(lowerCaseInput.charAt(ix))
+      )
+  )
 }
 
-int o_creat_new() {
-  exists(Macro m | m.getName() = "O_CREAT" | result = parseHex(m.getBody()))
-  or
-  exists(Macro m | m.getName() = "O_CREAT" | result = parseOctal(m.getBody()))
+int o_creat() {
+  exists(Macro m | m.getName() = "O_CREAT" |
+    result = parseHex(m.getBody()) or result = UnixConstants::parseOctal(m.getBody())
+  )
 }
 
 int o_tmpfile() {
-  exists(Macro m | m.getName() = "O_TMPFILE" | result = parseHex(m.getBody()))
-  or
-  exists(Macro m | m.getName() = "O_TMPFILE" | result = parseOctal(m.getBody()))
+  exists(Macro m | m.getName() = "O_TMPFILE" |
+    result = parseHex(m.getBody()) or result = UnixConstants::parseOctal(m.getBody())
+  )
 }
 
 bindingset[n, digit]
@@ -114,7 +116,7 @@ class OpenCreationExpr extends FileCreationWithOptionalModeExpr {
   OpenCreationExpr() {
     this.getTarget().getName() = ["open", "_open", "_wopen"] and
     exists(int flag | flag = this.getArgument(1).getValue().toInt() |
-      sets(flag, o_creat_new()) or sets(flag, o_tmpfile())
+      sets(flag, o_creat()) or sets(flag, o_tmpfile())
     )
   }
 
@@ -143,7 +145,7 @@ class OpenatCreationExpr extends FileCreationWithOptionalModeExpr {
   OpenatCreationExpr() {
     this.getTarget().getName() = "openat" and
     exists(int flag | flag = this.getArgument(2).getValue().toInt() |
-      sets(flag, o_creat_new()) or sets(flag, o_tmpfile())
+      sets(flag, o_creat()) or sets(flag, o_tmpfile())
     )
   }
 
@@ -162,7 +164,12 @@ class OpenatCreationExpr extends FileCreationWithOptionalModeExpr {
 
 private int fopenMode() {
   result =
-    s_irusr().bitOr(s_irgrp()).bitOr(s_iroth()).bitOr(s_iwusr()).bitOr(s_iwgrp()).bitOr(s_iwoth())
+    UnixConstants::s_irusr()
+        .bitOr(UnixConstants::s_irgrp())
+        .bitOr(UnixConstants::s_iroth())
+        .bitOr(UnixConstants::s_iwusr())
+        .bitOr(UnixConstants::s_iwgrp())
+        .bitOr(UnixConstants::s_iwoth())
 }
 
 class FopenCreationExpr extends FileCreationExpr {
@@ -194,6 +201,6 @@ class FopensCreationExpr extends FileCreationExpr {
     // fopen_s has restrictive permissions unless you have "u" in the mode
     if this.getArgument(2).getValue().charAt(_) = "u"
     then result = fopenMode()
-    else result = s_irusr().bitOr(s_iwusr())
+    else result = UnixConstants::s_irusr().bitOr(UnixConstants::s_iwusr())
   }
 }

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

@@ -1,8 +1,11 @@
 typedef unsigned int mode_t;
 
-#define O_APPEND  0x0020
-#define O_CREAT   0x0200
-#define O_TMPFILE 0x2000
+#define O_RDWR     0x0002
+#define O_CLOEXEC  0x0040
+#define O_NONBLOCK 0x0080
+#define O_CREAT    0x0200
+#define O_APPEND   0x0800
+#define O_TMPFILE  0x2000
 
 int open(const char *pathname, int flags, ...);
 
@@ -11,6 +14,8 @@ int openat(int dirfd, const char *pathname, int flags, ...);
 const char *a_file = "/a_file";
 
 void test_open() {
+  open(a_file, O_NONBLOCK); // GOOD
+  open(a_file, O_RDWR | O_CLOEXEC); // GOOD
   open(a_file, O_APPEND); // GOOD
   open(a_file, O_CREAT); // BAD
   open(a_file, O_CREAT, 0); // GOOD

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

@@ -1,4 +1,4 @@
-| OpenCallMissingModeArgument.c:15:3:15:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
-| OpenCallMissingModeArgument.c:17:3:17:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
-| OpenCallMissingModeArgument.c:20:3:20:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |
-| OpenCallMissingModeArgument.c:22:3:22:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |
+| OpenCallMissingModeArgument.c:20:3:20:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
+| OpenCallMissingModeArgument.c:22:3:22:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
+| OpenCallMissingModeArgument.c:25:3:25:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |
+| OpenCallMissingModeArgument.c:27:3:27:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |