Merge pull request #7933 from geoffw0/cwe497

C++: Improve cpp/system-data-exposure
2026-04-30 03:05:15 +02:00 · 2022-03-02 10:18:01 +00:00
parent 71cd507f89 2962b125af
commit 3681a1b736
11 changed files with 173 additions and 125 deletions
--- a/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql
+++ b/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.ir.dataflow.TaintTracking
+import semmle.code.cpp.models.interfaces.FlowSource
+import DataFlow::PathGraph

 /**
 * An element that should not be exposed to an adversary.
@@ -24,42 +26,19 @@ 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
-  }
 }

 /**
 * Data originating from the environment.
 */
 class EnvData extends SystemData {
-  EnvData() { this instanceof EnvironmentRead }
+  EnvData() {
+    // identify risky looking environment variables only
+    this.(EnvironmentRead)
+        .getEnvironmentVariable()
+        .toLowerCase()
+        .regexpMatch(".*(user|host|admin|root|home|path|http|ssl|snmp|sock|port|proxy|pass|token|crypt|key).*")
+  }

  override Expr getAnExpr() { result = this }
 }
@@ -91,11 +70,6 @@ class SQLConnectInfo extends SystemData {
 }

 private predicate posixSystemInfo(FunctionCall source, Element use) {
-  // long sysconf(int name)
-  //  - various OS / system values and limits
-  source.getTarget().hasName("sysconf") and
-  use = source
-  or
  // size_t confstr(int name, char *buf, size_t len)
  //  - various OS / system strings, such as the libc version
  // int statvfs(const char *__path, struct statvfs *__buf)
@@ -311,70 +285,31 @@ 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.asConvertedExpr() = 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).getAChild*() = 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
+from ExposedSystemDataConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where
-  sd.getAnExprIndirect() = ow.getASource() or
-  sd.getAnExprIndirect() = ow.getASource().getAChild*()
-select ow, "This operation exposes system data from $@.", sd, sd.toString()
+  config.hasFlowPath(source, sink) and
+  not exists(
+    DataFlow::Node alt // remove duplicate results on conversions
+  |
+    config.hasFlow(source.getNode(), alt) and
+    alt.asConvertedExpr() = sink.getNode().asExpr() and
+    alt != sink.getNode()
+  )
+select sink, source, sink, "This operation exposes system data from $@.", source,
+  source.getNode().toString()
--- a/cpp/ql/src/change-notes/2022-02-10-system-data-exposure.md
+++ b/cpp/ql/src/change-notes/2022-02-10-system-data-exposure.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/system-data-exposure` query has been modernized and has converted to a `path-problem` query. There are now fewer false positive results.
--- a/cpp/ql/src/codeql-suites/exclude-slow-queries.yml
+++ b/cpp/ql/src/codeql-suites/exclude-slow-queries.yml
@@ -2,7 +2,6 @@
 # These queries are infeasible to compute on large projects:
 - exclude:
    query path:
-      - Security/CWE/CWE-497/ExposedSystemData.ql
      - Critical/DescriptorMayNotBeClosed.ql
      - Critical/DescriptorNeverClosed.ql
      - Critical/FileMayNotBeClosed.ql
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/ExposedSystemData.expected
+++ b/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
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/OutputWrite.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/OutputWrite.expected
@@ -1,7 +0,0 @@
-| tests.c:29:9:29:14 | call to printf | tests.c:29:16:29:21 | %s\n |
-| tests.c:29:9:29:14 | call to printf | tests.c:29:24:29:27 | line |
-| tests.c:43:13:43:21 | call to printLine | tests.c:43:23:43:38 | fgets() failed |
-| tests.c:62:13:62:21 | call to printLine | tests.c:62:23:62:52 | User logged in successfully. |
-| tests.c:67:13:67:21 | call to printLine | tests.c:67:23:67:40 | Unable to login. |
-| tests.c:70:9:70:15 | call to fprintf | tests.c:70:25:70:67 | User attempted access with password: %s\n |
-| tests.c:70:9:70:15 | call to fprintf | tests.c:70:70:70:77 | password |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/OutputWrite.ql
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/OutputWrite.ql
@@ -1,4 +0,0 @@
-import semmle.code.cpp.security.OutputWrite
-
-from OutputWrite ow
-select ow, ow.getASource()
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/SAMATE/tests.c
+++ b/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]
    }
 }
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/ExposedSystemData.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/ExposedSystemData.expected
@@ -1,2 +1,47 @@
-| tests2.cpp:27:12:27:12 | call to operator<< | This operation exposes system data from $@. | tests2.cpp:27:15:27:20 | call to getenv | call to getenv |
-| tests2.cpp:28:25:28:25 | call to operator<< | This operation exposes system data from $@. | tests2.cpp:28:28:28:33 | call to getenv | call to getenv |
+edges
+| tests2.cpp:63:13:63:18 | call to getenv | tests2.cpp:63:13:63:26 | (const char *)... |
+| tests2.cpp:64:13:64:18 | call to getenv | tests2.cpp:64:13:64:26 | (const char *)... |
+| tests2.cpp:65:13:65:18 | call to getenv | tests2.cpp:65:13:65:30 | (const char *)... |
+| tests2.cpp:76:18:76:38 | call to mysql_get_client_info | tests2.cpp:79:14:79:19 | (const char *)... |
+| 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 | tests2.cpp:78:14:78:34 | call to mysql_get_client_info |
+| tests2.cpp:89:42:89:45 | str1 | tests2.cpp:91:14:91:17 | str1 |
+| tests2.cpp:99:8:99:15 | call to getpwuid | tests2.cpp:100:14:100:15 | pw |
+| tests2.cpp:107:3:107:4 | c1 [post update] [ptr] | tests2.cpp:109:14:109:15 | c1 [read] [ptr] |
+| tests2.cpp:107:6:107:8 | ptr [post update] | tests2.cpp:107:3:107:4 | c1 [post update] [ptr] |
+| tests2.cpp:107:12:107:17 | call to getenv | tests2.cpp:107:6:107:8 | ptr [post update] |
+| tests2.cpp:109:14:109:15 | c1 [read] [ptr] | tests2.cpp:109:14:109:19 | (const char *)... |
+nodes
+| tests2.cpp:63:13:63:18 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:63:13:63:18 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:63:13:63:26 | (const char *)... | semmle.label | (const char *)... |
+| tests2.cpp:64:13:64: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:64:13:64:26 | (const char *)... | semmle.label | (const char *)... |
+| tests2.cpp:65:13:65: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:65:13:65:30 | (const char *)... | semmle.label | (const char *)... |
+| 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:78:14:78:34 | call to mysql_get_client_info | semmle.label | call to mysql_get_client_info |
+| tests2.cpp:79:14:79:19 | (const char *)... | semmle.label | (const char *)... |
+| tests2.cpp:89:42:89:45 | str1 | semmle.label | str1 |
+| tests2.cpp:91:14:91:17 | str1 | semmle.label | str1 |
+| tests2.cpp:99:8:99:15 | call to getpwuid | semmle.label | call to getpwuid |
+| tests2.cpp:100:14:100:15 | pw | semmle.label | pw |
+| tests2.cpp:107:3:107:4 | c1 [post update] [ptr] | semmle.label | c1 [post update] [ptr] |
+| tests2.cpp:107:6:107:8 | ptr [post update] | semmle.label | ptr [post update] |
+| tests2.cpp:107:12:107:17 | call to getenv | semmle.label | call to getenv |
+| tests2.cpp:109:14:109:15 | c1 [read] [ptr] | semmle.label | c1 [read] [ptr] |
+| tests2.cpp:109:14:109:19 | (const char *)... | semmle.label | (const char *)... |
+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: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: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 | (const char *)... | tests2.cpp:76:18:76:38 | call to mysql_get_client_info | tests2.cpp:79:14:79:19 | (const char *)... | 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:91:14:91:17 | str1 | tests2.cpp:89:42:89:45 | str1 | tests2.cpp:91:14:91:17 | str1 | This operation exposes system data from $@. | tests2.cpp:89:42:89:45 | str1 | str1 |
+| tests2.cpp:100:14:100:15 | pw | tests2.cpp:99:8:99:15 | call to getpwuid | tests2.cpp:100:14:100:15 | pw | This operation exposes system data from $@. | tests2.cpp:99:8:99:15 | call to getpwuid | call to getpwuid |
+| tests2.cpp:109:14:109:19 | (const char *)... | tests2.cpp:107:12:107:17 | call to getenv | tests2.cpp:109:14:109:19 | (const char *)... | This operation exposes system data from $@. | tests2.cpp:107:12:107:17 | call to getenv | call to getenv |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/OutputWrite.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/OutputWrite.expected
@@ -1,5 +0,0 @@
-| tests2.cpp:27:12:27:12 | call to operator<< | tests2.cpp:27:15:27:20 | call to getenv |
-| tests2.cpp:28:12:28:12 | call to operator<< | tests2.cpp:28:15:28:23 | PATH =  |
-| tests2.cpp:28:25:28:25 | call to operator<< | tests2.cpp:28:28:28:33 | call to getenv |
-| tests2.cpp:28:43:28:43 | call to operator<< | tests2.cpp:28:46:28:48 | . |
-| tests2.cpp:29:12:29:12 | call to operator<< | tests2.cpp:29:15:29:28 | PATHPATHPATH |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/OutputWrite.ql
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/OutputWrite.ql
@@ -1,4 +0,0 @@
-import semmle.code.cpp.security.OutputWrite
-
-from OutputWrite ow
-select ow, ow.getASource()
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/tests2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/tests2.cpp
@@ -3,6 +3,7 @@
 // library functions etc

 char *getenv(const char *name);
+char *strcpy(char *s1, const char *s2);

 namespace std
 {
@@ -20,11 +21,92 @@ namespace std
 	extern ostream cout;
 }

+int socket(int p1, int p2, int p3);
+void send(int sock, const char *buffer, int p3, int p4);
+
+const char *mysql_get_client_info();
+void mysql_real_connect(int p1, int p2, int p3, const char *password, int p5, int p6, int p7, int p8);
+
+struct container
+{
+	char *ptr;
+};
+
+struct passwd
+{
+	// ...
+
+	char *pw_passwd;
+
+	// ...
+};
+
+passwd *getpwuid(int uid);
+
+int val();
+
 // test cases

+const char *global1 = mysql_get_client_info();
+const char *global2 = "abc";
+
 void test1()
 {
-	std::cout << getenv("HOME"); // BAD: outputs HOME environment variable
-	std::cout << "PATH = " << getenv("PATH") << "."; // BAD: outputs PATH environment variable
+	int sock = socket(val(), val(), val());
+
+	// tests for a strict implementation of CWE-497
+	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
+	send(sock, getenv("HOME"), val(), val()); // BAD
+	send(sock, getenv("PATH"), val(), val()); // BAD
+	send(sock, getenv("USERNAME"), val(), val()); // BAD
+	send(sock, getenv("HARMLESS"), val(), val()); // GOOD: harmless information
+	send(sock, "HOME", val(), val()); // GOOD: not system data
+	send(sock, "PATH", val(), val()); // GOOD: not system data
+	send(sock, "USERNAME", val(), val()); // GOOD: not system data
+	send(sock, "HARMLESS", val(), val()); // GOOD: not system data
+
+	// tests for `mysql_get_client_info`, including via a global
+	{
+		char buffer[256];
+
+		strcpy(buffer, mysql_get_client_info());
+
+		send(sock, mysql_get_client_info(), 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
+	}
+
+	// tests for `mysql_real_connect`
+	{
+		const char *str1 = "123456";
+		const char *str2 = "abcdef";
+
+		mysql_real_connect(sock, val(), val(), str1, val(), val(), val(), val());
+
+		send(sock, str1, val(), val()); // BAD
+		send(sock, str2, val(), val()); // GOOD: not system data
+	}
+
+	// tests for `getpwuid`
+	{
+		passwd *pw;
+
+		pw = getpwuid(val());
+		send(sock, pw->pw_passwd, val(), val()); // BAD
+	}
+
+	// tests for containers
+	{
+		container c1, c2;
+
+		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
+	}
 }