Merge pull request #9340 from geoffw0/nocheckbeforeunsafeputuser

C++: Improve cpp/linux-kernel-no-check-before-unsafe-put-user

Author: MathiasVP

cpp/ql/src/experimental/Security/CWE/CWE-020/NoCheckBeforeUnsafePutUser.ql

@@ -17,6 +17,36 @@
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
 
+/**
+ * A Linux system call.
+ */
+class SystemCallFunction extends Function {
+  SystemCallFunction() {
+    exists(MacroInvocation m |
+      m.getMacro().getName().matches("SYSCALL\\_DEFINE%") and
+      this = m.getEnclosingFunction()
+    )
+  }
+}
+
+/**
+ * A value that comes from a Linux system call (sources).
+ */
+class SystemCallSource extends DataFlow::Node {
+  SystemCallSource() {
+    exists(FunctionCall fc |
+      fc.getTarget() instanceof SystemCallFunction and
+      (
+        this.asDefiningArgument() = fc.getAnArgument().getAChild*() or
+        this.asExpr() = fc
+      )
+    )
+  }
+}
+
+/**
+ * Macros used to check the value (barriers).
+ */
 class WriteAccessCheckMacro extends Macro {
   VariableAccess va;
 
@@ -28,6 +58,9 @@ class WriteAccessCheckMacro extends Macro {
   VariableAccess getArgument() { result = va }
 }
 
+/**
+ * The `unsafe_put_user` macro and its uses (sinks).
+ */
 class UnSafePutUserMacro extends Macro {
   PointerDereferenceExpr writeUserPtr;
 
@@ -42,15 +75,13 @@ class UnSafePutUserMacro extends Macro {
   }
 }
 
-class ExploitableUserModePtrParam extends Parameter {
+class ExploitableUserModePtrParam extends SystemCallSource {
   ExploitableUserModePtrParam() {
-    not exists(WriteAccessCheckMacro writeAccessCheck |
-      DataFlow::localFlow(DataFlow::parameterNode(this),
-        DataFlow::exprNode(writeAccessCheck.getArgument()))
-    ) and
     exists(UnSafePutUserMacro unsafePutUser |
-      DataFlow::localFlow(DataFlow::parameterNode(this),
-        DataFlow::exprNode(unsafePutUser.getUserModePtr()))
+      DataFlow::localFlow(this, DataFlow::exprNode(unsafePutUser.getUserModePtr()))
+    ) and
+    not exists(WriteAccessCheckMacro writeAccessCheck |
+      DataFlow::localFlow(this, DataFlow::exprNode(writeAccessCheck.getArgument()))
     )
   }
 }

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-020/NoCheckBeforeUnsafePutUser/NoCheckBeforeUnsafePutUser.expected

@@ -1 +1,3 @@
-| test.cpp:14:16:14:16 | p | unsafe_put_user write user-mode pointer $@ without check. | test.cpp:14:16:14:16 | p | p |
+| test.cpp:20:21:20:22 | ref arg & ... | unsafe_put_user write user-mode pointer $@ without check. | test.cpp:20:21:20:22 | ref arg & ... | ref arg & ... |
+| test.cpp:41:21:41:22 | ref arg & ... | unsafe_put_user write user-mode pointer $@ without check. | test.cpp:41:21:41:22 | ref arg & ... | ref arg & ... |
+| test.cpp:69:21:69:27 | ref arg & ... | unsafe_put_user write user-mode pointer $@ without check. | test.cpp:69:21:69:27 | ref arg & ... | ref arg & ... |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-020/NoCheckBeforeUnsafePutUser/test.cpp

@@ -1,7 +1,11 @@
 
 typedef unsigned long size_t;
 
-void SYSC_SOMESYSTEMCALL(void *param);
+#define SYSCALL_DEFINE(name, ...) \
+	void do_sys_##name(); \
+	void sys_##name(...) { do_sys_##name(); } \
+	void do_sys_##name()
+SYSCALL_DEFINE(somesystemcall, void *param) {};
 
 bool user_access_begin_impl(const void *where, size_t sz);
 void user_access_end_impl();
@@ -13,14 +17,14 @@ void unsafe_put_user_impl(int what, const void *where, size_t sz);
 
 void test1(int p)
 {
-	SYSC_SOMESYSTEMCALL(&p);
+	sys_somesystemcall(&p);
 
 	unsafe_put_user(123, &p); // BAD
 }
 
 void test2(int p)
 {
-	SYSC_SOMESYSTEMCALL(&p);
+	sys_somesystemcall(&p);
 
 	if (user_access_begin(&p, sizeof(p)))
 	{
@@ -34,16 +38,16 @@ void test3()
 {
 	int v;
 
-	SYSC_SOMESYSTEMCALL(&v);
+	sys_somesystemcall(&v);
 
-	unsafe_put_user(123, &v); // BAD [NOT DETECTED]
+	unsafe_put_user(123, &v); // BAD
 }
 
 void test4()
 {
 	int v;
 
-	SYSC_SOMESYSTEMCALL(&v);
+	sys_somesystemcall(&v);
 
 	if (user_access_begin(&v, sizeof(v)))
 	{
@@ -62,16 +66,16 @@ void test5()
 {
 	data myData;
 
-	SYSC_SOMESYSTEMCALL(&myData);
+	sys_somesystemcall(&myData);
 
-	unsafe_put_user(123, &(myData.x)); // BAD [NOT DETECTED]
+	unsafe_put_user(123, &(myData.x)); // BAD
 }
 
 void test6()
 {
 	data myData;
 
-	SYSC_SOMESYSTEMCALL(&myData);
+	sys_somesystemcall(&myData);
 
 	if (user_access_begin(&myData, sizeof(myData)))
 	{