Merge pull request github#12407 from MathiasVP/fix-internal-system-data-tests

MathiasVP · web-flow · commit aa09361a423f · 2023-03-06T15:45:25.000Z
diff --git a/cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql b/cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql
@@ -39,7 +39,17 @@ class PotentiallyExposedSystemDataConfiguration extends TaintTracking::Configura
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(OutputWrite ow | ow.getASource().getAChild*() = sink.asIndirectExpr())
+    exists(OutputWrite ow, Expr child | child = ow.getASource().getAChild*() |
+      // Most sinks receive a pointer as an argument (for example `printf`),
+      // and we use an indirect sink for those.
+      // However, some sinks (for example `puts`) receive receive a single
+      // character as an argument. For those we have to use a direct sink.
+      if
+        child.getUnspecifiedType() instanceof PointerType or
+        child.getUnspecifiedType() instanceof ArrayType
+      then child = sink.asIndirectExpr()
+      else child = sink.asExpr()
+    )
   }
 }
 
diff --git a/cpp/ql/src/Security/CWE/CWE-497/SystemData.qll b/cpp/ql/src/Security/CWE/CWE-497/SystemData.qll
@@ -72,7 +72,7 @@ private predicate sqlConnectInfo(FunctionCall source, Expr use) {
 class SqlConnectInfo extends SystemData {
   SqlConnectInfo() { sqlConnectInfo(this, _) }
 
-  override DataFlow::Node getAnExpr() { sqlConnectInfo(this, result.asExpr()) }
+  override DataFlow::Node getAnExpr() { sqlConnectInfo(this, result.asIndirectExpr(1)) }
 
   override predicate isSensitive() { any() }
 }
diff --git 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
@@ -7,7 +7,9 @@ edges
 | tests2.cpp:65:13:65:18 | call to getenv indirection | tests2.cpp:65:13:65:30 | call to getenv indirection |
 | tests2.cpp:66:13:66:18 | call to getenv indirection | tests2.cpp:66:13:66:34 | call to getenv indirection |
 | tests2.cpp:78:18:78:38 | call to mysql_get_client_info indirection | tests2.cpp:81:14:81:19 | buffer indirection |
-| tests2.cpp:91:42:91:45 | str1 | tests2.cpp:93:14:93:17 | str1 indirection |
+| tests2.cpp:91:42:91:45 | str1 indirection | tests2.cpp:93:14:93:17 | str1 indirection |
+| tests2.cpp:91:42:91:45 | str1 indirection | tests2.cpp:93:14:93:17 | str1 indirection |
+| tests2.cpp:91:42:91:45 | str1 indirection | tests2.cpp:93:14:93:17 | str1 indirection |
 | tests2.cpp:101:8:101:15 | call to getpwuid indirection | tests2.cpp:102:14:102:15 | pw indirection |
 | tests2.cpp:101:8:101:15 | call to getpwuid indirection | tests2.cpp:102:14:102:15 | pw indirection |
 | tests2.cpp:109:3:109:36 | ... = ... indirection | tests2.cpp:109:6:109:8 | c1 indirection [post update] [ptr indirection] |
@@ -52,7 +54,10 @@ nodes
 | tests2.cpp:81:14:81:19 | buffer indirection | semmle.label | buffer indirection |
 | tests2.cpp:82:14:82:20 | global1 indirection | semmle.label | global1 indirection |
 | tests2.cpp:82:14:82:20 | global1 indirection | semmle.label | global1 indirection |
-| tests2.cpp:91:42:91:45 | str1 | semmle.label | str1 |
+| tests2.cpp:91:42:91:45 | str1 indirection | semmle.label | str1 indirection |
+| tests2.cpp:91:42:91:45 | str1 indirection | semmle.label | str1 indirection |
+| tests2.cpp:93:14:93:17 | str1 indirection | semmle.label | str1 indirection |
+| tests2.cpp:93:14:93:17 | str1 indirection | semmle.label | str1 indirection |
 | tests2.cpp:93:14:93:17 | str1 indirection | semmle.label | str1 indirection |
 | tests2.cpp:101:8:101:15 | call to getpwuid indirection | semmle.label | call to getpwuid indirection |
 | tests2.cpp:102:14:102:15 | pw indirection | semmle.label | pw indirection |
@@ -96,7 +101,9 @@ subpaths
 | tests2.cpp:81:14:81:19 | buffer indirection | tests2.cpp:78:18:78:38 | call to mysql_get_client_info indirection | tests2.cpp:81:14:81:19 | buffer indirection | This operation exposes system data from $@. | tests2.cpp:78:18:78:38 | call to mysql_get_client_info indirection | call to mysql_get_client_info indirection |
 | tests2.cpp:82:14:82:20 | global1 indirection | tests2.cpp:50:23:50:43 | call to mysql_get_client_info indirection | tests2.cpp:82:14:82:20 | global1 indirection | This operation exposes system data from $@. | tests2.cpp:50:23:50:43 | call to mysql_get_client_info indirection | call to mysql_get_client_info indirection |
 | tests2.cpp:82:14:82:20 | global1 indirection | tests2.cpp:50:23:50:43 | call to mysql_get_client_info indirection | tests2.cpp:82:14:82:20 | global1 indirection | This operation exposes system data from $@. | tests2.cpp:50:23:50:43 | call to mysql_get_client_info indirection | call to mysql_get_client_info indirection |
-| tests2.cpp:93:14:93:17 | str1 indirection | tests2.cpp:91:42:91:45 | str1 | tests2.cpp:93:14:93:17 | str1 indirection | This operation exposes system data from $@. | tests2.cpp:91:42:91:45 | str1 | str1 |
+| tests2.cpp:93:14:93:17 | str1 indirection | tests2.cpp:91:42:91:45 | str1 indirection | tests2.cpp:93:14:93:17 | str1 indirection | This operation exposes system data from $@. | tests2.cpp:91:42:91:45 | str1 indirection | str1 indirection |
+| tests2.cpp:93:14:93:17 | str1 indirection | tests2.cpp:91:42:91:45 | str1 indirection | tests2.cpp:93:14:93:17 | str1 indirection | This operation exposes system data from $@. | tests2.cpp:91:42:91:45 | str1 indirection | str1 indirection |
+| tests2.cpp:93:14:93:17 | str1 indirection | tests2.cpp:91:42:91:45 | str1 indirection | tests2.cpp:93:14:93:17 | str1 indirection | This operation exposes system data from $@. | tests2.cpp:91:42:91:45 | str1 indirection | str1 indirection |
 | tests2.cpp:102:14:102:15 | pw indirection | tests2.cpp:101:8:101:15 | call to getpwuid indirection | tests2.cpp:102:14:102:15 | pw indirection | This operation exposes system data from $@. | tests2.cpp:101:8:101:15 | call to getpwuid indirection | call to getpwuid indirection |
 | tests2.cpp:102:14:102:15 | pw indirection | tests2.cpp:101:8:101:15 | call to getpwuid indirection | tests2.cpp:102:14:102:15 | pw indirection | This operation exposes system data from $@. | tests2.cpp:101:8:101:15 | call to getpwuid indirection | call to getpwuid indirection |
 | tests2.cpp:111:14:111:19 | ptr indirection | tests2.cpp:109:12:109:17 | call to getenv indirection | tests2.cpp:111:14:111:19 | ptr indirection | This operation exposes system data from $@. | tests2.cpp:109:12:109:17 | call to getenv indirection | call to getenv indirection |

Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,17 @@ class PotentiallyExposedSystemDataConfiguration extends TaintTracking::Configura`
`39`	`39`	`}`
`40`	`40`
`41`	`41`	`override predicate isSink(DataFlow::Node sink) {`
`42`		`- exists(OutputWrite ow \| ow.getASource().getAChild*() = sink.asIndirectExpr())`
	`42`	`+ exists(OutputWrite ow, Expr child \| child = ow.getASource().getAChild*() \|`
	`43`	+ // Most sinks receive a pointer as an argument (for example `printf`),
	`44`	`+ // and we use an indirect sink for those.`
	`45`	+ // However, some sinks (for example `puts`) receive receive a single
	`46`	`+ // character as an argument. For those we have to use a direct sink.`
	`47`	`+ if`
	`48`	`+ child.getUnspecifiedType() instanceof PointerType or`
	`49`	`+ child.getUnspecifiedType() instanceof ArrayType`
	`50`	`+ then child = sink.asIndirectExpr()`
	`51`	`+ else child = sink.asExpr()`
	`52`	`+ )`
`43`	`53`	`}`
`44`	`54`	`}`
`45`	`55`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ private predicate sqlConnectInfo(FunctionCall source, Expr use) {`
`72`	`72`	`class SqlConnectInfo extends SystemData {`
`73`	`73`	`SqlConnectInfo() { sqlConnectInfo(this, _) }`
`74`	`74`
`75`		`- override DataFlow::Node getAnExpr() { sqlConnectInfo(this, result.asExpr()) }`
	`75`	`+ override DataFlow::Node getAnExpr() { sqlConnectInfo(this, result.asIndirectExpr(1)) }`
`76`	`76`
`77`	`77`	`override predicate isSensitive() { any() }`
`78`	`78`	`}`