Merge pull request github#4729 from MathiasVP/safe-external-api-function-use-model-interfaces-only

C++: Use model interfaces in DefaultSafeExternalAPIFunction

Author: MathiasVP

cpp/ql/src/Security/CWE/CWE-020/SafeExternalAPIFunction.qll

@@ -3,7 +3,7 @@
  */
 
 private import cpp
-private import semmle.code.cpp.models.implementations.Pure
+private import semmle.code.cpp.models.interfaces.SideEffect
 
 /**
  * A `Function` that is considered a "safe" external API from a security perspective.
@@ -13,9 +13,12 @@ abstract class SafeExternalAPIFunction extends Function { }
 /** The default set of "safe" external APIs. */
 private class DefaultSafeExternalAPIFunction extends SafeExternalAPIFunction {
   DefaultSafeExternalAPIFunction() {
-    // implementation note: this should be based on the properties of public interfaces, rather than accessing implementation classes directly.  When we've done that, the three classes referenced here should be made fully private.
-    this instanceof PureStrFunction or
-    this instanceof StrLenFunction or
-    this instanceof PureMemFunction
+    // If a function does not write to any of its arguments, we consider it safe to
+    // pass untrusted data to it. This means that string functions such as `strcmp`
+    // and `strlen`, as well as memory functions such as `memcmp`, are considered safe.
+    exists(SideEffectFunction model | model = this |
+      model.hasOnlySpecificWriteSideEffects() and
+      not model.hasSpecificWriteSideEffect(_, _, _)
+    )
   }
 }

cpp/ql/src/Security/CWE/CWE-020/ir/SafeExternalAPIFunction.qll

@@ -3,7 +3,7 @@
  */
 
 private import cpp
-private import semmle.code.cpp.models.implementations.Pure
+private import semmle.code.cpp.models.interfaces.SideEffect
 
 /**
  * A `Function` that is considered a "safe" external API from a security perspective.
@@ -13,9 +13,12 @@ abstract class SafeExternalAPIFunction extends Function { }
 /** The default set of "safe" external APIs. */
 private class DefaultSafeExternalAPIFunction extends SafeExternalAPIFunction {
   DefaultSafeExternalAPIFunction() {
-    // implementation note: this should be based on the properties of public interfaces, rather than accessing implementation classes directly.  When we've done that, the three classes referenced here should be made fully private.
-    this instanceof PureStrFunction or
-    this instanceof StrLenFunction or
-    this instanceof PureMemFunction
+    // If a function does not write to any of its arguments, we consider it safe to
+    // pass untrusted data to it. This means that string functions such as `strcmp`
+    // and `strlen`, as well as memory functions such as `memcmp`, are considered safe.
+    exists(SideEffectFunction model | model = this |
+      model.hasOnlySpecificWriteSideEffects() and
+      not model.hasSpecificWriteSideEffect(_, _, _)
+    )
   }
 }

cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll

@@ -3,12 +3,9 @@ import semmle.code.cpp.models.interfaces.Taint
 import semmle.code.cpp.models.interfaces.Alias
 import semmle.code.cpp.models.interfaces.SideEffect
 
-/**
- * Pure string functions.
- *
- * INTERNAL: do not use.
- */
-class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideEffectFunction {
+/** Pure string functions. */
+private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction,
+  SideEffectFunction {
   PureStrFunction() {
     hasGlobalOrStdName([
         "atof", "atoi", "atol", "atoll", "strcasestr", "strchnul", "strchr", "strchrnul", "strstr",
@@ -63,12 +60,8 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
   }
 }
 
-/**
- * String standard `strlen` function, and related functions for computing string lengths.
- *
- * INTERNAL: do not use.
- */
-class StrLenFunction extends AliasFunction, ArrayFunction, SideEffectFunction {
+/** String standard `strlen` function, and related functions for computing string lengths. */
+private class StrLenFunction extends AliasFunction, ArrayFunction, SideEffectFunction {
   StrLenFunction() {
     hasGlobalOrStdName(["strlen", "strnlen", "wcslen"])
     or
@@ -118,12 +111,9 @@ private class PureFunction extends TaintFunction, SideEffectFunction {
   override predicate hasOnlySpecificWriteSideEffects() { any() }
 }
 
-/**
- * Pure raw-memory functions.
- *
- * INTERNAL: do not use.
- */
-class PureMemFunction extends AliasFunction, ArrayFunction, TaintFunction, SideEffectFunction {
+/** Pure raw-memory functions. */
+private class PureMemFunction extends AliasFunction, ArrayFunction, TaintFunction,
+  SideEffectFunction {
   PureMemFunction() { hasGlobalOrStdName(["memchr", "memrchr", "rawmemchr", "memcmp", "memmem"]) }
 
   override predicate hasArrayInput(int bufParam) {