C++: Split 'gets' model and make it a local source.

Author: geoffw0

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) {
@@ -54,17 +53,60 @@ private class GetsFunction extends DataFlowFunction, TaintFunction, ArrayFunctio
   }
 
   override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
-    not this.hasName("gets") and
     bufParam = 0 and
     countParam = 1
   }
 
+  override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
+
+  override predicate hasSocketInput(FunctionInput input) { input.isParameter(2) }
+}
+
+
+/**
+ * The standard functions `gets`.
+ */
+private class GetsFunction extends DataFlowFunction, TaintFunction, ArrayFunction, AliasFunction,
+  SideEffectFunction, LocalFlowSourceFunction {
+  GetsFunction() {
+    // gets(str)
+    this.hasGlobalOrStdOrBslName("gets")
+  }
+
+  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
+    input.isParameter(0) and
+    output.isReturnValue()
+  }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    input.isParameter(2) and
+    output.isParameterDeref(0)
+  }
+
+  override predicate parameterNeverEscapes(int index) { none() }
+
+  override predicate parameterEscapesOnlyViaReturn(int index) { index = 0 }
+
+  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()
+  }
+
   override predicate hasArrayWithUnknownSize(int bufParam) {
-    this.hasName("gets") and
     bufParam = 0
   }
 
   override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
-
-  override predicate hasSocketInput(FunctionInput input) { input.isParameter(2) }
 }

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

@@ -1,5 +1,4 @@
 edges
-| test2.cpp:101:8:101:15 | password | test2.cpp:103:8:103:15 | password |
 | test3.cpp:17:28:17:36 | password1 | test3.cpp:22:15:22:23 | password1 |
 | test3.cpp:17:51:17:59 | password2 | test3.cpp:26:15:26:23 | password2 |
 | test3.cpp:45:8:45:15 | password | test3.cpp:47:15:47:22 | password |
@@ -91,8 +90,6 @@ edges
 | test.cpp:66:23:66:43 | cleartext password! | test.cpp:76:21:76:27 | call to encrypt |
 | test.cpp:66:23:66:43 | cleartext password! | test.cpp:76:29:76:39 | thePassword |
 nodes
-| test2.cpp:101:8:101:15 | password | semmle.label | password |
-| test2.cpp:103:8:103:15 | password | semmle.label | password |
 | test3.cpp:17:28:17:36 | password1 | semmle.label | password1 |
 | test3.cpp:17:51:17:59 | password2 | semmle.label | password2 |
 | test3.cpp:22:15:22:23 | password1 | semmle.label | password1 |
@@ -213,7 +210,6 @@ subpaths
 | test3.cpp:316:11:316:19 | password1 | test3.cpp:283:20:283:23 | data | test3.cpp:283:20:283:23 | data | test3.cpp:316:11:316:19 | ref arg password1 |
 | test3.cpp:324:11:324:14 | data | test3.cpp:293:20:293:23 | data | test3.cpp:293:20:293:23 | data | test3.cpp:324:11:324:14 | ref arg data |
 #select
-| test2.cpp:103:3:103:6 | call to gets | test2.cpp:101:8:101:15 | password | test2.cpp:103:8:103:15 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test2.cpp:101:8:101:15 | password | password |
 | test3.cpp:22:3:22:6 | call to send | test3.cpp:17:28:17:36 | password1 | test3.cpp:22:15:22:23 | password1 | This operation transmits 'password1', which may contain unencrypted sensitive data from $@ | test3.cpp:17:28:17:36 | password1 | password1 |
 | test3.cpp:26:3:26:6 | call to send | test3.cpp:17:51:17:59 | password2 | test3.cpp:26:15:26:23 | password2 | This operation transmits 'password2', which may contain unencrypted sensitive data from $@ | test3.cpp:17:51:17:59 | password2 | password2 |
 | test3.cpp:47:3:47:6 | call to recv | test3.cpp:45:8:45:15 | password | test3.cpp:47:15:47:22 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:45:8:45:15 | password | password |

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

@@ -100,6 +100,6 @@ void test_gets()
 	{
 		char password[1024];
 
-		gets(password); // BAD [but FALSE POSITIVE from cpp/cleartext-transmission]
+		gets(password); // BAD
 	}
 }