Merge pull request #14744 from jketema/fgets

C++: Fix `hasRemoteFlowSource` for `fgets`

Author: jketema

cpp/ql/lib/semmle/code/cpp/models/implementations/Gets.qll

@@ -49,10 +49,11 @@ private class FgetsFunction extends DataFlowFunction, TaintFunction, ArrayFuncti
   }
 
   override predicate hasRemoteFlowSource(FunctionOutput output, string description) {
-    output.isParameterDeref(0) and
-    description = "string read by " + this.getName()
-    or
-    output.isReturnValue() and
+    (
+      output.isParameterDeref(0) or
+      output.isReturnValue() or
+      output.isReturnValueDeref()
+    ) and
     description = "string read by " + this.getName()
   }
 

cpp/ql/test/query-tests/Security/CWE/CWE-114/semmle/UncontrolledProcessOperation/UncontrolledProcessOperation.expected

@@ -52,6 +52,10 @@ edges
 | test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
 | test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | buffer |
 | test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | buffer |
+| test.cpp:113:8:113:12 | call to fgets | test.cpp:114:9:114:11 | ptr |
+| test.cpp:113:8:113:12 | call to fgets | test.cpp:114:9:114:11 | ptr |
+| test.cpp:113:8:113:12 | call to fgets | test.cpp:114:9:114:11 | ptr |
+| test.cpp:113:8:113:12 | call to fgets | test.cpp:114:9:114:11 | ptr |
 subpaths
 nodes
 | test.cpp:24:30:24:36 | command | semmle.label | command |
@@ -91,6 +95,10 @@ nodes
 | test.cpp:106:17:106:22 | recv output argument | semmle.label | recv output argument |
 | test.cpp:107:15:107:20 | buffer | semmle.label | buffer |
 | test.cpp:107:15:107:20 | buffer | semmle.label | buffer |
+| test.cpp:113:8:113:12 | call to fgets | semmle.label | call to fgets |
+| test.cpp:113:8:113:12 | call to fgets | semmle.label | call to fgets |
+| test.cpp:114:9:114:11 | ptr | semmle.label | ptr |
+| test.cpp:114:9:114:11 | ptr | semmle.label | ptr |
 #select
 | test.cpp:26:10:26:16 | command | test.cpp:42:18:42:23 | call to getenv | test.cpp:26:10:26:16 | command | The value of this argument may come from $@ and is being passed to system. | test.cpp:42:18:42:23 | call to getenv | call to getenv |
 | test.cpp:31:10:31:16 | command | test.cpp:43:18:43:23 | call to getenv | test.cpp:31:10:31:16 | command | The value of this argument may come from $@ and is being passed to system. | test.cpp:43:18:43:23 | call to getenv | call to getenv |
@@ -101,3 +109,4 @@ nodes
 | test.cpp:78:10:78:15 | buffer | test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer | The value of this argument may come from $@ and is being passed to system. | test.cpp:76:12:76:17 | buffer | buffer |
 | test.cpp:99:15:99:20 | buffer | test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer | The value of this argument may come from $@ and is being passed to LoadLibrary. | test.cpp:98:17:98:22 | buffer | buffer |
 | test.cpp:107:15:107:20 | buffer | test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer | The value of this argument may come from $@ and is being passed to LoadLibrary. | test.cpp:106:17:106:22 | buffer | buffer |
+| test.cpp:114:9:114:11 | ptr | test.cpp:113:8:113:12 | call to fgets | test.cpp:114:9:114:11 | ptr | The value of this argument may come from $@ and is being passed to system. | test.cpp:113:8:113:12 | call to fgets | call to fgets |

cpp/ql/test/query-tests/Security/CWE/CWE-114/semmle/UncontrolledProcessOperation/test.cpp

@@ -107,3 +107,9 @@ void testAcceptRecv(int socket1, int socket2)
 		LoadLibrary(buffer); // BAD: using data from recv
 	}
 }
+
+void argumentUse(char *ptr, FILE *stream) {
+	char buffer[80];
+	ptr = fgets(buffer, sizeof(buffer), stream);
+	system(ptr); // BAD
+}