Merge pull request github#7703 from geoffw0/getslocal

Author: MathiasVP

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

@@ -11,15 +11,14 @@ import semmle.code.cpp.models.interfaces.SideEffect
 import semmle.code.cpp.models.interfaces.FlowSource
 
 /**
- * The standard functions `gets` and `fgets`.
+ * The standard functions `fgets` and `fgetws`.
  */
-private class GetsFunction extends DataFlowFunction, TaintFunction, ArrayFunction, AliasFunction,
+private class FgetsFunction extends DataFlowFunction, TaintFunction, ArrayFunction, AliasFunction,
   SideEffectFunction, RemoteFlowSourceFunction {
-  GetsFunction() {
-    // gets(str)
+  FgetsFunction() {
     // fgets(str, num, stream)
     // fgetws(wstr, num, stream)
-    this.hasGlobalOrStdOrBslName(["gets", "fgets", "fgetws"])
+    this.hasGlobalOrStdOrBslName(["fgets", "fgetws"])
   }
 
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
@@ -51,20 +50,61 @@ private class GetsFunction extends DataFlowFunction, TaintFunction, ArrayFunctio
   override predicate hasRemoteFlowSource(FunctionOutput output, string description) {
     output.isParameterDeref(0) and
     description = "String read by " + this.getName()
+    or
+    output.isReturnValue() and
+    description = "String read by " + this.getName()
   }
 
   override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
-    not this.hasName("gets") and
     bufParam = 0 and
     countParam = 1
   }
 
-  override predicate hasArrayWithUnknownSize(int bufParam) {
-    this.hasName("gets") and
-    bufParam = 0
+  override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
+
+  override predicate hasSocketInput(FunctionInput input) { input.isParameterDeref(2) }
+}
+
+/**
+ * The standard functions `gets`.
+ */
+private class GetsFunction extends DataFlowFunction, ArrayFunction, AliasFunction,
+  SideEffectFunction, LocalFlowSourceFunction {
+  GetsFunction() {
+    // gets(str)
+    this.hasGlobalOrStdOrBslName("gets")
   }
 
-  override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
+  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
+    input.isParameter(0) and
+    output.isReturnValue()
+  }
+
+  override predicate parameterNeverEscapes(int index) { none() }
+
+  override predicate parameterEscapesOnlyViaReturn(int index) { index = 0 }
 
-  override predicate hasSocketInput(FunctionInput input) { input.isParameter(2) }
+  override predicate parameterIsAlwaysReturned(int index) { index = 0 }
+
+  override predicate hasOnlySpecificReadSideEffects() { any() }
+
+  override predicate hasOnlySpecificWriteSideEffects() { any() }
+
+  override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
+    i = 0 and
+    buffer = true and
+    mustWrite = true
+  }
+
+  override predicate hasLocalFlowSource(FunctionOutput output, string description) {
+    output.isParameterDeref(0) and
+    description = "String read by " + this.getName()
+    or
+    output.isReturnValue() and
+    description = "String read by " + this.getName()
+  }
+
+  override predicate hasArrayWithUnknownSize(int bufParam) { bufParam = 0 }
+
+  override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
 }

cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql

@@ -58,7 +58,10 @@ class Send extends SendRecv instanceof RemoteFlowSinkFunction {
     call.getTarget() = this and
     exists(FunctionInput input, int arg |
       super.hasSocketInput(input) and
-      input.isParameter(arg) and
+      (
+        input.isParameter(arg) or
+        input.isParameterDeref(arg)
+      ) and
       result = call.getArgument(arg)
     )
   }
@@ -81,7 +84,10 @@ class Recv extends SendRecv instanceof RemoteFlowSourceFunction {
     call.getTarget() = this and
     exists(FunctionInput input, int arg |
       super.hasSocketInput(input) and
-      input.isParameter(arg) and
+      (
+        input.isParameter(arg) or
+        input.isParameterDeref(arg)
+      ) and
       result = call.getArgument(arg)
     )
   }

cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/CleartextBufferWrite.expected

@@ -1,8 +1,10 @@
 edges
 | test.cpp:54:17:54:20 | argv | test.cpp:58:25:58:29 | input |
 nodes
+| test2.cpp:110:3:110:6 | call to gets | semmle.label | call to gets |
 | test.cpp:54:17:54:20 | argv | semmle.label | argv |
 | test.cpp:58:25:58:29 | input | semmle.label | input |
 subpaths
 #select
+| test2.cpp:110:3:110:6 | call to gets | test2.cpp:110:3:110:6 | call to gets | test2.cpp:110:3:110:6 | call to gets | This write into buffer 'password' may contain unencrypted data from $@ | test2.cpp:110:3:110:6 | call to gets | user input (String read by gets) |
 | test.cpp:58:3:58:9 | call to sprintf | test.cpp:54:17:54:20 | argv | test.cpp:58:25:58:29 | input | This write into buffer 'passwd' may contain unencrypted data from $@ | test.cpp:54:17:54:20 | argv | user input (a command-line argument) |

cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/test2.cpp

@@ -99,3 +99,14 @@ void tests(FILE *log, myStruct &s)
 		fprintf(log, "log: %s", buffer); // BAD
 	}
 }
+
+char *gets(char *s);
+
+void test_gets()
+{
+	{
+		char password[1024];
+
+		gets(password); // BAD
+	}
+}