C++: Modernize cpp/system-data-exposure as a path-problem using IR taint, RemoteFlowSinkFunction.

Author: geoffw0

cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql

@@ -3,7 +3,7 @@
  * @description Exposing system data or debugging information helps
  *              an adversary learn about the system and form an
  *              attack plan.
- * @kind problem
+ * @kind path-problem
  * @problem.severity warning
  * @security-severity 6.5
  * @precision medium
@@ -14,7 +14,9 @@
 
 import cpp
 import semmle.code.cpp.commons.Environment
-import semmle.code.cpp.security.OutputWrite
+import semmle.code.cpp.dataflow.TaintTracking
+import semmle.code.cpp.models.interfaces.FlowSource
+import DataFlow::PathGraph
 
 /**
  * An element that should not be exposed to an adversary.
@@ -24,35 +26,6 @@ abstract class SystemData extends Element {
    * Gets an expression that is part of this `SystemData`.
    */
   abstract Expr getAnExpr();
-
-  /**
-   * Gets an expression whose value originates from, or is used by,
-   * this `SystemData`.
-   */
-  Expr getAnExprIndirect() {
-    // direct SystemData
-    result = this.getAnExpr() or
-    // flow via global or member variable (conservative approximation)
-    result = this.getAnAffectedVar().getAnAccess() or
-    // flow via stack variable
-    definitionUsePair(_, this.getAnExprIndirect(), result) or
-    useUsePair(_, this.getAnExprIndirect(), result) or
-    useUsePair(_, result, this.getAnExprIndirect()) or
-    // flow from assigned value to assignment expression
-    result.(AssignExpr).getRValue() = this.getAnExprIndirect()
-  }
-
-  /**
-   * Gets a global or member variable that may be affected by this system
-   * data (conservative approximation).
-   */
-  private Variable getAnAffectedVar() {
-    (
-      result.getAnAssignedValue() = this.getAnExprIndirect() or
-      result.getAnAccess() = this.getAnExprIndirect()
-    ) and
-    not result instanceof LocalScopeVariable
-  }
 }
 
 /**
@@ -311,70 +284,23 @@ class RegQuery extends SystemData {
   override Expr getAnExpr() { regQuery(this, result) }
 }
 
-/**
- * Somewhere data is output.
- */
-abstract class DataOutput extends Element {
-  /**
-   * Get an expression containing data that is output.
-   */
-  abstract Expr getASource();
-}
+class ExposedSystemDataConfiguration extends TaintTracking::Configuration {
+  ExposedSystemDataConfiguration() { this = "ExposedSystemDataConfiguration" }
 
-/**
- * Data that is output via standard output or standard error.
- */
-class StandardOutput extends DataOutput instanceof OutputWrite {
-  override Expr getASource() { result = OutputWrite.super.getASource() }
-}
+  override predicate isSource(DataFlow::Node source) {
+    source.asExpr() = any(SystemData sd).getAnExpr()
+  }
 
-private predicate socketCallOrIndirect(FunctionCall call) {
-  // direct socket call
-  // int socket(int domain, int type, int protocol);
-  call.getTarget().getName() = "socket"
-  or
-  exists(ReturnStmt rtn |
-    // indirect socket call
-    call.getTarget() = rtn.getEnclosingFunction() and
-    (
-      socketCallOrIndirect(rtn.getExpr()) or
-      socketCallOrIndirect(rtn.getExpr().(VariableAccess).getTarget().getAnAssignedValue())
+  override predicate isSink(DataFlow::Node sink) {
+    exists(FunctionCall fc, FunctionInput input, int arg |
+      fc.getTarget().(RemoteFlowSinkFunction).hasRemoteFlowSink(input, _) and
+      input.isParameterDeref(arg) and
+      fc.getArgument(arg) = sink.asExpr()
     )
-  )
-}
-
-private predicate socketFileDescriptor(Expr e) {
-  exists(Variable var, FunctionCall socket |
-    socketCallOrIndirect(socket) and
-    var.getAnAssignedValue() = socket and
-    e = var.getAnAccess()
-  )
-}
-
-private predicate socketOutput(FunctionCall call, Expr data) {
-  (
-    // ssize_t send(int sockfd, const void *buf, size_t len, int flags);
-    // ssize_t sendto(int sockfd, const void *buf, size_t len, int flags,
-    //                const struct sockaddr *dest_addr, socklen_t addrlen);
-    // ssize_t sendmsg(int sockfd, const struct msghdr *msg, int flags);
-    // int write(int handle, void *buffer, int nbyte);
-    call.getTarget().hasGlobalName(["send", "sendto", "sendmsg", "write"]) and
-    data = call.getArgument(1) and
-    socketFileDescriptor(call.getArgument(0))
-  )
-}
-
-/**
- * Data that is output via a socket.
- */
-class SocketOutput extends DataOutput {
-  SocketOutput() { socketOutput(this, _) }
-
-  override Expr getASource() { socketOutput(this, result) }
+  }
 }
 
-from SystemData sd, DataOutput ow
-where
-  sd.getAnExprIndirect() = ow.getASource() or
-  sd.getAnExprIndirect() = ow.getASource().getAChild*()
-select ow, "This operation exposes system data from $@.", sd, sd.toString()
+from ExposedSystemDataConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink, source, sink, "This operation exposes system data from $@.", source,
+  source.getNode().toString()

cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/ExposedSystemData.expected

@@ -1 +1,4 @@
-| tests.c:70:9:70:15 | call to fprintf | This operation exposes system data from $@. | tests.c:54:13:54:22 | call to LogonUserA | call to LogonUserA |
+edges
+nodes
+subpaths
+#select

cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/tests.c

@@ -67,6 +67,6 @@ void CWE535_Info_Exposure_Shell_Error__w32_char_01_bad()
             printLine("Unable to login.");
         }
         /* FLAW: Write sensitive data to stderr */
-        fprintf(stderr, "User attempted access with password: %s\n", password);
+        fprintf(stderr, "User attempted access with password: %s\n", password); // [NOT DETECTED]
     }
 }

cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/ExposedSystemData.expected

@@ -1,12 +1,28 @@
-| tests2.cpp:58:12:58:12 | call to operator<< | This operation exposes system data from $@. | tests2.cpp:58:15:58:20 | call to getenv | call to getenv |
-| tests2.cpp:59:25:59:25 | call to operator<< | This operation exposes system data from $@. | tests2.cpp:59:28:59:33 | call to getenv | call to getenv |
-| tests2.cpp:63:2:63:5 | call to send | This operation exposes system data from $@. | tests2.cpp:63:13:63:18 | call to getenv | call to getenv |
-| tests2.cpp:64:2:64:5 | call to send | This operation exposes system data from $@. | tests2.cpp:64:13:64:18 | call to getenv | call to getenv |
-| tests2.cpp:65:2:65:5 | call to send | This operation exposes system data from $@. | tests2.cpp:65:13:65:18 | call to getenv | call to getenv |
-| tests2.cpp:66:2:66:5 | call to send | This operation exposes system data from $@. | tests2.cpp:66:13:66:18 | call to getenv | call to getenv |
-| tests2.cpp:78:3:78:6 | call to send | This operation exposes system data from $@. | tests2.cpp:78:14:78:34 | call to mysql_get_client_info | call to mysql_get_client_info |
-| tests2.cpp:80:3:80:6 | call to send | This operation exposes system data from $@. | tests2.cpp:50:23:50:43 | call to mysql_get_client_info | call to mysql_get_client_info |
-| tests2.cpp:91:3:91:6 | call to send | This operation exposes system data from $@. | tests2.cpp:89:3:89:20 | call to mysql_real_connect | call to mysql_real_connect |
-| tests2.cpp:100:3:100:6 | call to send | This operation exposes system data from $@. | tests2.cpp:99:8:99:15 | call to getpwuid | call to getpwuid |
-| tests2.cpp:109:3:109:6 | call to send | This operation exposes system data from $@. | tests2.cpp:107:12:107:17 | call to getenv | call to getenv |
-| tests2.cpp:110:3:110:6 | call to send | This operation exposes system data from $@. | tests2.cpp:107:12:107:17 | call to getenv | call to getenv |
+edges
+| tests2.cpp:76:18:76:38 | call to mysql_get_client_info | tests2.cpp:79:14:79:19 | buffer |
+| tests2.cpp:107:3:107:4 | c1 [post update] [ptr] | tests2.cpp:109:14:109:15 | c1 [ptr] |
+| tests2.cpp:107:3:107:36 | ... = ... | tests2.cpp:107:3:107:4 | c1 [post update] [ptr] |
+| tests2.cpp:107:12:107:17 | call to getenv | tests2.cpp:107:3:107:36 | ... = ... |
+| tests2.cpp:109:14:109:15 | c1 [ptr] | tests2.cpp:109:17:109:19 | ptr |
+nodes
+| tests2.cpp:63:13:63:18 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:64:13:64:18 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:65:13:65:18 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:66:13:66:18 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:76:18:76:38 | call to mysql_get_client_info | semmle.label | call to mysql_get_client_info |
+| tests2.cpp:78:14:78:34 | call to mysql_get_client_info | semmle.label | call to mysql_get_client_info |
+| tests2.cpp:79:14:79:19 | buffer | semmle.label | buffer |
+| tests2.cpp:107:3:107:4 | c1 [post update] [ptr] | semmle.label | c1 [post update] [ptr] |
+| tests2.cpp:107:3:107:36 | ... = ... | semmle.label | ... = ... |
+| tests2.cpp:107:12:107:17 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:109:14:109:15 | c1 [ptr] | semmle.label | c1 [ptr] |
+| tests2.cpp:109:17:109:19 | ptr | semmle.label | ptr |
+subpaths
+#select
+| tests2.cpp:63:13:63:18 | call to getenv | tests2.cpp:63:13:63:18 | call to getenv | tests2.cpp:63:13:63:18 | call to getenv | This operation exposes system data from $@. | tests2.cpp:63:13:63:18 | call to getenv | call to getenv |
+| tests2.cpp:64:13:64:18 | call to getenv | tests2.cpp:64:13:64:18 | call to getenv | tests2.cpp:64:13:64:18 | call to getenv | This operation exposes system data from $@. | tests2.cpp:64:13:64:18 | call to getenv | call to getenv |
+| tests2.cpp:65:13:65:18 | call to getenv | tests2.cpp:65:13:65:18 | call to getenv | tests2.cpp:65:13:65:18 | call to getenv | This operation exposes system data from $@. | tests2.cpp:65:13:65:18 | call to getenv | call to getenv |
+| tests2.cpp:66:13:66:18 | call to getenv | tests2.cpp:66:13:66:18 | call to getenv | tests2.cpp:66:13:66:18 | call to getenv | This operation exposes system data from $@. | tests2.cpp:66:13:66:18 | call to getenv | call to getenv |
+| tests2.cpp:78:14:78:34 | call to mysql_get_client_info | tests2.cpp:78:14:78:34 | call to mysql_get_client_info | tests2.cpp:78:14:78:34 | call to mysql_get_client_info | This operation exposes system data from $@. | tests2.cpp:78:14:78:34 | call to mysql_get_client_info | call to mysql_get_client_info |
+| tests2.cpp:79:14:79:19 | buffer | tests2.cpp:76:18:76:38 | call to mysql_get_client_info | tests2.cpp:79:14:79:19 | buffer | This operation exposes system data from $@. | tests2.cpp:76:18:76:38 | call to mysql_get_client_info | call to mysql_get_client_info |
+| tests2.cpp:109:17:109:19 | ptr | tests2.cpp:107:12:107:17 | call to getenv | tests2.cpp:109:17:109:19 | ptr | This operation exposes system data from $@. | tests2.cpp:107:12:107:17 | call to getenv | call to getenv |

cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/tests2.cpp

@@ -55,8 +55,8 @@ void test1()
 	int sock = socket(val(), val(), val());
 
 	// tests for a strict implementation of CWE-497
-	std::cout << getenv("HOME"); // BAD: outputs HOME environment variable
-	std::cout << "PATH = " << getenv("PATH") << "."; // BAD: outputs PATH environment variable
+	std::cout << getenv("HOME"); // BAD: outputs HOME environment variable [NOT DETECTED]
+	std::cout << "PATH = " << getenv("PATH") << "."; // BAD: outputs PATH environment variable [NOT DETECTED]
 	std::cout << "PATHPATHPATH"; // GOOD: not system data
 
 	// tests for a more pragmatic implementation of CWE-497
@@ -76,8 +76,8 @@ void test1()
 		strcpy(buffer, mysql_get_client_info());
 
 		send(sock, mysql_get_client_info(), val(), val()); // BAD
-		send(sock, buffer, val(), val()); // BAD [NOT DETECTED]
-		send(sock, global1, val(), val()); // BAD
+		send(sock, buffer, val(), val()); // BAD
+		send(sock, global1, val(), val()); // BAD [NOT DETECTED]
 		send(sock, global2, val(), val()); // GOOD: not system data
 	}
 
@@ -88,7 +88,7 @@ void test1()
 
 		mysql_real_connect(sock, val(), val(), str1, val(), val(), val(), val());
 
-		send(sock, str1, val(), val()); // BAD
+		send(sock, str1, val(), val()); // BAD [NOT DETECTED]
 		send(sock, str2, val(), val()); // GOOD: not system data
 	}
 
@@ -97,7 +97,7 @@ void test1()
 		passwd *pw;
 
 		pw = getpwuid(val());
-		send(sock, pw->pw_passwd, val(), val()); // BAD
+		send(sock, pw->pw_passwd, val(), val()); // BAD [NOT DETECTED]
 	}
 
 	// tests for containers
@@ -107,6 +107,6 @@ void test1()
 		c1.ptr = getenv("MY_SECRET_TOKEN");
 		c2.ptr = "";
 		send(sock, c1.ptr, val(), val()); // BAD
-		send(sock, c2.ptr, val(), val()); // GOOD: not system data [FALSE POSITIVE]
+		send(sock, c2.ptr, val(), val()); // GOOD: not system data
 	}
 }