Merge branch 'main' into port-url-redirect-query

Author: RasmusWL

cpp/ql/src/semmle/code/cpp/commons/Scanf.qll

@@ -92,7 +92,9 @@ class Snscanf extends ScanfFunction {
     this instanceof TopLevelFunction and
     (
       hasName("_snscanf") or // _snscanf(src, max_amount, format, args...)
-      hasName("_snwscanf") // _snwscanf(src, max_amount, format, args...)
+      hasName("_snwscanf") or // _snwscanf(src, max_amount, format, args...)
+      hasName("_snscanf_l") or // _snscanf_l(src, max_amount, format, locale, args...)
+      hasName("_snwscanf_l") // _snwscanf_l(src, max_amount, format, locale, args...)
       // note that the max_amount is not a limit on the output length, it's an input length
       // limit used with non null-terminated strings.
     )
@@ -101,6 +103,12 @@ class Snscanf extends ScanfFunction {
   override int getInputParameterIndex() { result = 0 }
 
   override int getFormatParameterIndex() { result = 2 }
+
+  /**
+   * Gets the position at which the maximum number of characters in the
+   * input string is specified.
+   */
+  int getInputLengthParameterIndex() { result = 1 }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/models/Models.qll

@@ -25,3 +25,4 @@ private import implementations.StdString
 private import implementations.Swap
 private import implementations.GetDelim
 private import implementations.SmartPointer
+private import implementations.Sscanf

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

@@ -24,7 +24,8 @@ private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffect
     or
     // bcopy(src, dest, num)
     // mempcpy(dest, src, num)
-    this.hasGlobalName(["bcopy", mempcpy(), "__builtin___memcpy_chk"])
+    // memccpy(dest, src, c, n)
+    this.hasGlobalName(["bcopy", mempcpy(), "memccpy", "__builtin___memcpy_chk"])
   }
 
   /**
@@ -41,7 +42,7 @@ private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffect
   /**
    * Gets the index of the parameter that is the size of the copy (in bytes).
    */
-  int getParamSize() { result = 2 }
+  int getParamSize() { if this.hasGlobalName("memccpy") then result = 3 else result = 2 }
 
   override predicate hasArrayInput(int bufParam) { bufParam = getParamSrc() }
 
@@ -71,7 +72,10 @@ private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffect
   override predicate hasOnlySpecificWriteSideEffects() { any() }
 
   override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
-    i = getParamDest() and buffer = true and mustWrite = true
+    i = getParamDest() and
+    buffer = true and
+    // memccpy only writes until a given character `c` is found
+    (if this.hasGlobalName("memccpy") then mustWrite = false else mustWrite = true)
   }
 
   override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
@@ -97,7 +101,7 @@ private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffect
   }
 
   override predicate parameterIsAlwaysReturned(int index) {
-    not this.hasGlobalName(["bcopy", mempcpy()]) and
+    not this.hasGlobalName(["bcopy", mempcpy(), "memccpy"]) and
     index = getParamDest()
   }
 }

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

@@ -15,8 +15,9 @@ import semmle.code.cpp.models.interfaces.SideEffect
 private class MemsetFunction extends ArrayFunction, DataFlowFunction, AliasFunction,
   SideEffectFunction {
   MemsetFunction() {
-    hasGlobalName(["memset", "wmemset", "bzero", "__builtin_memset", "__builtin_memset_chk"]) or
-    hasQualifiedName("std", ["memset", "wmemset"])
+    this.hasGlobalOrStdName(["memset", "wmemset"])
+    or
+    this.hasGlobalName([bzero(), "__builtin_memset", "__builtin_memset_chk"])
   }
 
   override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
@@ -28,17 +29,17 @@ private class MemsetFunction extends ArrayFunction, DataFlowFunction, AliasFunct
 
   override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
     bufParam = 0 and
-    (if hasGlobalName("bzero") then countParam = 1 else countParam = 2)
+    (if hasGlobalName(bzero()) then countParam = 1 else countParam = 2)
   }
 
-  override predicate parameterNeverEscapes(int index) { hasGlobalName("bzero") and index = 0 }
+  override predicate parameterNeverEscapes(int index) { hasGlobalName(bzero()) and index = 0 }
 
   override predicate parameterEscapesOnlyViaReturn(int index) {
-    not hasGlobalName("bzero") and index = 0
+    not hasGlobalName(bzero()) and index = 0
   }
 
   override predicate parameterIsAlwaysReturned(int index) {
-    not hasGlobalName("bzero") and index = 0
+    not hasGlobalName(bzero()) and index = 0
   }
 
   override predicate hasOnlySpecificReadSideEffects() { any() }
@@ -51,6 +52,8 @@ private class MemsetFunction extends ArrayFunction, DataFlowFunction, AliasFunct
 
   override ParameterIndex getParameterSizeIndex(ParameterIndex i) {
     i = 0 and
-    if hasGlobalName("bzero") then result = 1 else result = 2
+    if hasGlobalName(bzero()) then result = 1 else result = 2
   }
 }
+
+private string bzero() { result = ["bzero", "explicit_bzero"] }

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

@@ -8,9 +8,8 @@ private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunctio
   SideEffectFunction {
   PureStrFunction() {
     hasGlobalOrStdName([
-        "atof", "atoi", "atol", "atoll", "strcasestr", "strchnul", "strchr", "strchrnul", "strstr",
-        "strpbrk", "strcmp", "strcspn", "strncmp", "strrchr", "strspn", "strtod", "strtof",
-        "strtol", "strtoll", "strtoq", "strtoul"
+        atoi(), "strcasestr", "strchnul", "strchr", "strchrnul", "strstr", "strpbrk", "strrchr",
+        "strspn", strtol(), strrev(), strcmp(), strlwr(), strupr()
       ])
   }
 
@@ -24,11 +23,16 @@ private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunctio
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     exists(ParameterIndex i |
-      input.isParameter(i) and
-      exists(getParameter(i))
-      or
-      input.isParameterDeref(i) and
-      getParameter(i).getUnspecifiedType() instanceof PointerType
+      (
+        input.isParameter(i) and
+        exists(getParameter(i))
+        or
+        input.isParameterDeref(i) and
+        getParameter(i).getUnspecifiedType() instanceof PointerType
+      ) and
+      // Functions that end with _l also take a locale argument (always as the last argument),
+      // and we don't want taint from those arguments.
+      (not this.getName().matches("%\\_l") or exists(getParameter(i + 1)))
     ) and
     (
       output.isReturnValueDeref() and
@@ -60,6 +64,31 @@ private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunctio
   }
 }
 
+private string atoi() { result = ["atof", "atoi", "atol", "atoll"] }
+
+private string strtol() { result = ["strtod", "strtof", "strtol", "strtoll", "strtoq", "strtoul"] }
+
+private string strlwr() {
+  result = ["_strlwr", "_wcslwr", "_mbslwr", "_strlwr_l", "_wcslwr_l", "_mbslwr_l"]
+}
+
+private string strupr() {
+  result = ["_strupr", "_wcsupr", "_mbsupr", "_strupr_l", "_wcsupr_l", "_mbsupr_l"]
+}
+
+private string strrev() { result = ["_strrev", "_wcsrev", "_mbsrev", "_mbsrev_l"] }
+
+private string strcmp() {
+  // NOTE: `strcoll` doesn't satisfy _all_ the definitions of purity: its behavior depends on
+  // `LC_COLLATE` (which is set by `setlocale`). Not sure this behavior worth including in the model, so
+  // for now we interpret the function as being pure.
+  result =
+    [
+      "strcmp", "strcspn", "strncmp", "strcoll", "strverscmp", "_mbsnbcmp", "_mbsnbcmp_l",
+      "_stricmp"
+    ]
+}
+
 /** String standard `strlen` function, and related functions for computing string lengths. */
 private class StrLenFunction extends AliasFunction, ArrayFunction, SideEffectFunction {
   StrLenFunction() {
@@ -114,19 +143,28 @@ private class PureFunction extends TaintFunction, SideEffectFunction {
 /** Pure raw-memory functions. */
 private class PureMemFunction extends AliasFunction, ArrayFunction, TaintFunction,
   SideEffectFunction {
-  PureMemFunction() { hasGlobalOrStdName(["memchr", "memrchr", "rawmemchr", "memcmp", "memmem"]) }
+  PureMemFunction() {
+    hasGlobalOrStdName([
+        "memchr", "__builtin_memchr", "memrchr", "rawmemchr", "memcmp", "__builtin_memcmp", "memmem"
+      ]) or
+    this.hasGlobalName("memfrob")
+  }
 
   override predicate hasArrayInput(int bufParam) {
     getParameter(bufParam).getUnspecifiedType() instanceof PointerType
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     exists(ParameterIndex i |
-      input.isParameter(i) and
-      exists(getParameter(i))
-      or
-      input.isParameterDeref(i) and
-      getParameter(i).getUnspecifiedType() instanceof PointerType
+      (
+        input.isParameter(i) and
+        exists(getParameter(i))
+        or
+        input.isParameterDeref(i) and
+        getParameter(i).getUnspecifiedType() instanceof PointerType
+      ) and
+      // `memfrob` should not have taint from the size argument.
+      (not this.hasGlobalName("memfrob") or i = 0)
     ) and
     (
       output.isReturnValueDeref() and

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

@@ -0,0 +1,72 @@
+/**
+ * Provides implementation classes modeling `sscanf`, `fscanf` and various similar
+ * functions. See `semmle.code.cpp.models.Models` for usage information.
+ */
+
+import semmle.code.cpp.Function
+import semmle.code.cpp.commons.Scanf
+import semmle.code.cpp.models.interfaces.ArrayFunction
+import semmle.code.cpp.models.interfaces.Taint
+import semmle.code.cpp.models.interfaces.Alias
+import semmle.code.cpp.models.interfaces.SideEffect
+
+/**
+ * The standard function `sscanf`, `fscanf` and its assorted variants
+ */
+private class SscanfModel extends ArrayFunction, TaintFunction, AliasFunction, SideEffectFunction {
+  SscanfModel() { this instanceof Sscanf or this instanceof Fscanf or this instanceof Snscanf }
+
+  override predicate hasArrayWithNullTerminator(int bufParam) {
+    bufParam = this.(ScanfFunction).getFormatParameterIndex()
+    or
+    not this instanceof Fscanf and
+    bufParam = this.(ScanfFunction).getInputParameterIndex()
+  }
+
+  override predicate hasArrayInput(int bufParam) { hasArrayWithNullTerminator(bufParam) }
+
+  private int getLengthParameterIndex() { result = this.(Snscanf).getInputLengthParameterIndex() }
+
+  private int getLocaleParameterIndex() {
+    this.getName().matches("%\\_l") and
+    (
+      if exists(getLengthParameterIndex())
+      then result = getLengthParameterIndex() + 2
+      else result = 2
+    )
+  }
+
+  private int getArgsStartPosition() { result = this.getNumberOfParameters() }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    input.isParameterDeref(this.(ScanfFunction).getInputParameterIndex()) and
+    output.isParameterDeref(any(int i | i >= getArgsStartPosition()))
+  }
+
+  override predicate parameterNeverEscapes(int index) {
+    index = [0 .. max(getACallToThisFunction().getNumberOfArguments())]
+  }
+
+  override predicate parameterEscapesOnlyViaReturn(int index) { none() }
+
+  override predicate parameterIsAlwaysReturned(int index) { none() }
+
+  override predicate hasOnlySpecificReadSideEffects() { any() }
+
+  override predicate hasOnlySpecificWriteSideEffects() { any() }
+
+  override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
+    i >= getArgsStartPosition() and
+    buffer = true and
+    mustWrite = true
+  }
+
+  override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
+    buffer = true and
+    i =
+      [
+        this.(ScanfFunction).getInputParameterIndex(),
+        this.(ScanfFunction).getFormatParameterIndex(), getLocaleParameterIndex()
+      ]
+  }
+}

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

@@ -13,16 +13,20 @@ import semmle.code.cpp.models.interfaces.SideEffect
  */
 class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, SideEffectFunction {
   StrcatFunction() {
-    getName() =
-      [
+    this.hasGlobalOrStdName([
         "strcat", // strcat(dst, src)
         "strncat", // strncat(dst, src, max_amount)
         "wcscat", // wcscat(dst, src)
+        "wcsncat" // wcsncat(dst, src, max_amount)
+      ])
+    or
+    this.hasGlobalName([
         "_mbscat", // _mbscat(dst, src)
-        "wcsncat", // wcsncat(dst, src, max_amount)
         "_mbsncat", // _mbsncat(dst, src, max_amount)
-        "_mbsncat_l" // _mbsncat_l(dst, src, max_amount, locale)
-      ]
+        "_mbsncat_l", // _mbsncat_l(dst, src, max_amount, locale)
+        "_mbsnbcat", // _mbsnbcat(dest, src, count)
+        "_mbsnbcat_l" // _mbsnbcat_l(dest, src, count, locale)
+      ])
   }
 
   /**
@@ -50,7 +54,7 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
     input.isParameter(2) and
     output.isParameterDeref(0)
     or
-    getName() = "_mbsncat_l" and
+    getName() = ["_mbsncat_l", "_mbsnbcat_l"] and
     input.isParameter(3) and
     output.isParameterDeref(0)
     or

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

@@ -13,25 +13,36 @@ import semmle.code.cpp.models.interfaces.SideEffect
  */
 class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, SideEffectFunction {
   StrcpyFunction() {
-    getName() =
-      [
+    this.hasGlobalOrStdName([
         "strcpy", // strcpy(dst, src)
         "wcscpy", // wcscpy(dst, src)
-        "_mbscpy", // _mbscpy(dst, src)
         "strncpy", // strncpy(dst, src, max_amount)
-        "_strncpy_l", // _strncpy_l(dst, src, max_amount, locale)
         "wcsncpy", // wcsncpy(dst, src, max_amount)
+        "strxfrm", // strxfrm(dest, src, max_amount)
+        "wcsxfrm" // wcsxfrm(dest, src, max_amount)
+      ])
+    or
+    this.hasGlobalName([
+        "_mbscpy", // _mbscpy(dst, src)
+        "_strncpy_l", // _strncpy_l(dst, src, max_amount, locale)
         "_wcsncpy_l", // _wcsncpy_l(dst, src, max_amount, locale)
         "_mbsncpy", // _mbsncpy(dst, src, max_amount)
-        "_mbsncpy_l"
-      ] // _mbsncpy_l(dst, src, max_amount, locale)
+        "_mbsncpy_l", // _mbsncpy_l(dst, src, max_amount, locale)
+        "_strxfrm_l", // _strxfrm_l(dest, src, max_amount, locale)
+        "wcsxfrm_l", // _strxfrm_l(dest, src, max_amount, locale)
+        "_mbsnbcpy", // _mbsnbcpy(dest, src, max_amount)
+        "stpcpy", // stpcpy(dest, src)
+        "stpncpy" // stpcpy(dest, src, max_amount)
+      ])
     or
-    getName() =
-      [
-        "strcpy_s", // strcpy_s(dst, max_amount, src)
-        "wcscpy_s", // wcscpy_s(dst, max_amount, src)
-        "_mbscpy_s"
-      ] and // _mbscpy_s(dst, max_amount, src)
+    (
+      this.hasGlobalOrStdName([
+          "strcpy_s", // strcpy_s(dst, max_amount, src)
+          "wcscpy_s" // wcscpy_s(dst, max_amount, src)
+        ])
+      or
+      this.hasGlobalName("_mbscpy_s") // _mbscpy_s(dst, max_amount, src)
+    ) and
     // exclude the 2-parameter template versions
     // that find the size of a fixed size destination buffer.
     getNumberOfParameters() = 3
@@ -48,10 +59,10 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
   int getParamSize() {
     if isSVariant()
     then result = 1
-    else
-      if exists(getName().indexOf("ncpy"))
-      then result = 2
-      else none()
+    else (
+      getName().matches(["%ncpy%", "%nbcpy%", "%xfrm%"]) and
+      result = 2
+    )
   }
 
   /**