CPP: Add query for CWE-125 Out-of-bounds Read with different interpretation of the string when use mbtowc

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.cpp

@@ -0,0 +1,6 @@
+
+...
+mbtowc(&wc, ptr, 4)); // BAD:we can get unpredictable results
+...
+mbtowc(&wc, ptr, MB_LEN_MAX); // GOOD
+...

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.qhelp

@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p> Using a function to convert multibyte or wide characters with an invalid length argument may result in an out-of-range access error or unexpected results.</p>
+
+</overview>
+
+<example>
+<p>The following example shows the erroneous and corrected method of using function mbtowc.</p>
+<sample src="DangerousWorksWithMultibyteOrWideCharacters.cpp" />
+
+</example>
+<references>
+
+<li>
+  CERT Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts - SEI CERT C Coding Standard - Confluence</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.ql

@@ -0,0 +1,236 @@
+/**
+ * @name Dangerous use convert function.
+ * @description Using convert function with an invalid length argument can result in an out-of-bounds access error or unexpected result.
+ * @kind problem
+ * @id cpp/dangerous-use-convert-function
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-125
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/** Holds if there are indications that the variable is treated as a string. */
+predicate exprMayBeString(Expr exp) {
+  (
+    exists(StringLiteral sl | globalValueNumber(exp) = globalValueNumber(sl))
+    or
+    exists(FunctionCall fctmp |
+      (
+        fctmp.getAnArgument().(VariableAccess).getTarget() = exp.(VariableAccess).getTarget() or
+        globalValueNumber(fctmp.getAnArgument()) = globalValueNumber(exp)
+      ) and
+      fctmp.getTarget().hasName(["strlen", "strcat", "strncat", "strcpy", "sptintf", "printf"])
+    )
+    or
+    exists(AssignExpr astmp |
+      astmp.getRValue().getValue() = "0" and
+      astmp.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() =
+        exp.(VariableAccess).getTarget()
+    )
+    or
+    exists(ComparisonOperation cotmp, Expr exptmp1, Expr exptmp2 |
+      exptmp1.getValue() = "0" and
+      (
+        exptmp2.(PointerDereferenceExpr).getOperand().(VariableAccess).getTarget() =
+          exp.(VariableAccess).getTarget() or
+        exptmp2.(ArrayExpr).getArrayBase().(VariableAccess).getTarget() =
+          exp.getAChild().(VariableAccess).getTarget()
+      ) and
+      cotmp.hasOperands(exptmp1, exptmp2)
+    )
+  )
+}
+
+/** Holds if expression is constant or operator call `sizeof`. */
+predicate argConstOrSizeof(Expr exp) {
+  exp.getValue().toInt() > 1 or
+  exp.(SizeofTypeOperator).getTypeOperand().getSize() > 1
+}
+
+/** Holds if expression is macro. */
+predicate argMacro(Expr exp) {
+  exists(MacroInvocation matmp |
+    exp = matmp.getExpr() and
+    (
+      matmp.getMacroName() = "MB_LEN_MAX" or
+      matmp.getMacroName() = "MB_CUR_MAX"
+    )
+  )
+}
+
+/** Holds if erroneous situations of using functions `mbtowc` and `mbrtowc` are detected. */
+predicate findUseCharacterConversion(Expr exp, string msg) {
+  exists(FunctionCall fc |
+    fc = exp and
+    (
+      exists(Loop lptmp | lptmp = fc.getEnclosingStmt().getParentStmt*()) and
+      fc.getTarget().hasName(["mbtowc", "mbrtowc", "_mbtowc_l"]) and
+      not fc.getArgument(0).isConstant() and
+      not fc.getArgument(1).isConstant() and
+      (
+        exprMayBeString(fc.getArgument(1)) and
+        argConstOrSizeof(fc.getArgument(2)) and
+        fc.getArgument(2).getValue().toInt() < 5 and
+        not argMacro(fc.getArgument(2)) and
+        msg = "Size can be less than maximum character length, use macro MB_CUR_MAX."
+        or
+        not exprMayBeString(fc.getArgument(1)) and
+        (
+          argConstOrSizeof(fc.getArgument(2))
+          or
+          argMacro(fc.getArgument(2))
+          or
+          exists(DecrementOperation dotmp |
+            globalValueNumber(dotmp.getAnOperand()) = globalValueNumber(fc.getArgument(2)) and
+            not exists(AssignSubExpr aetmp |
+              (
+                aetmp.getLValue().(VariableAccess).getTarget() =
+                  fc.getArgument(2).(VariableAccess).getTarget() or
+                globalValueNumber(aetmp.getLValue()) = globalValueNumber(fc.getArgument(2))
+              ) and
+              globalValueNumber(aetmp.getRValue()) = globalValueNumber(fc)
+            )
+          )
+        ) and
+        msg =
+          "Access beyond the allocated memory is possible, the length can change without changing the pointer."
+        or
+        exists(AssignPointerAddExpr aetmp |
+          (
+            aetmp.getLValue().(VariableAccess).getTarget() =
+              fc.getArgument(0).(VariableAccess).getTarget() or
+            globalValueNumber(aetmp.getLValue()) = globalValueNumber(fc.getArgument(0))
+          ) and
+          globalValueNumber(aetmp.getRValue()) = globalValueNumber(fc)
+        ) and
+        msg = "Maybe you're using the function's return value incorrectly."
+      )
+    )
+  )
+}
+
+/** Holds if detecting erroneous situations of working with multibyte characters. */
+predicate findUseMultibyteCharacter(Expr exp, string msg) {
+  exists(ArrayType arrayType, ArrayExpr arrayExpr |
+    arrayExpr = exp and
+    arrayExpr.getArrayBase().getType() = arrayType and
+    (
+      exists(AssignExpr assZero, SizeofExprOperator sizeofArray, Expr oneValue |
+        oneValue.getValue() = "1" and
+        sizeofArray.getExprOperand().getType() = arrayType and
+        assZero.getLValue() = arrayExpr and
+        arrayExpr.getArrayOffset().(SubExpr).hasOperands(sizeofArray, oneValue) and
+        assZero.getRValue().getValue() = "0"
+      ) and
+      arrayType.getArraySize() != arrayType.getByteSize() and
+      msg =
+        "The size of the array element is greater than one byte, so the offset will point outside the array."
+      or
+      exists(FunctionCall mbFunction |
+        (
+          mbFunction.getTarget().getName().matches("_mbs%") or
+          mbFunction.getTarget().getName().matches("mbs%") or
+          mbFunction.getTarget().getName().matches("_mbc%") or
+          mbFunction.getTarget().getName().matches("mbc%")
+        ) and
+        mbFunction.getAnArgument().(VariableAccess).getTarget().getADeclarationEntry().getType() =
+          arrayType
+      ) and
+      exists(Loop loop, SizeofExprOperator sizeofArray, AssignExpr assignExpr |
+        arrayExpr.getEnclosingStmt().getParentStmt*() = loop and
+        sizeofArray.getExprOperand().getType() = arrayType and
+        assignExpr.getLValue() = arrayExpr and
+        loop.getCondition().(LTExpr).getLeftOperand().(VariableAccess).getTarget() =
+          arrayExpr.getArrayOffset().getAChild*().(VariableAccess).getTarget() and
+        loop.getCondition().(LTExpr).getRightOperand() = sizeofArray
+      ) and
+      msg =
+        "This buffer may contain multibyte characters, so attempting to copy may result in part of the last character being lost."
+    )
+  )
+  or
+  exists(FunctionCall mbccpy, Loop loop, SizeofExprOperator sizeofOp |
+    mbccpy.getTarget().hasName("_mbccpy") and
+    mbccpy.getArgument(0) = exp and
+    exp.getEnclosingStmt().getParentStmt*() = loop and
+    sizeofOp.getExprOperand().getType() =
+      exp.getAChild*().(VariableAccess).getTarget().getADeclarationEntry().getType() and
+    loop.getCondition().(LTExpr).getLeftOperand().(VariableAccess).getTarget() =
+      exp.getAChild*().(VariableAccess).getTarget() and
+    loop.getCondition().(LTExpr).getRightOperand() = sizeofOp and
+    msg =
+      "This buffer may contain multibyte characters, so an attempt to copy may result in an overflow."
+  )
+}
+
+/** Holds if erroneous situations of using functions `MultiByteToWideChar` and `WideCharToMultiByte` or `mbstowcs` and `_mbstowcs_l` and `mbsrtowcs` are detected. */
+predicate findUseStringConversion(
+  Expr exp, string msg, int posBufSrc, int posBufDst, int posSizeDst, string nameCalls
+) {
+  exists(FunctionCall fc |
+    fc = exp and
+    posBufSrc in [0 .. fc.getNumberOfArguments() - 1] and
+    posSizeDst in [0 .. fc.getNumberOfArguments() - 1] and
+    (
+      fc.getTarget().hasName(nameCalls) and
+      (
+        globalValueNumber(fc.getArgument(posBufDst)) = globalValueNumber(fc.getArgument(posBufSrc)) and
+        msg =
+          "According to the definition of the functions, if the source buffer and the destination buffer are the same, undefined behavior is possible."
+        or
+        exists(ArrayType arrayDst |
+          fc.getArgument(posBufDst).(VariableAccess).getTarget().getADeclarationEntry().getType() =
+            arrayDst and
+          fc.getArgument(posSizeDst).getValue().toInt() >= arrayDst.getArraySize() and
+          not exists(AssignExpr assZero |
+            assZero.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() =
+              fc.getArgument(posBufDst).(VariableAccess).getTarget() and
+            assZero.getRValue().getValue() = "0"
+          ) and
+          not exists(Expr someExp, FunctionCall checkSize |
+            checkSize.getASuccessor*() = fc and
+            checkSize.getTarget().hasName(nameCalls) and
+            checkSize.getArgument(posSizeDst).getValue() = "0" and
+            globalValueNumber(checkSize) = globalValueNumber(someExp) and
+            someExp.getEnclosingStmt().getParentStmt*() instanceof IfStmt
+          ) and
+          exprMayBeString(fc.getArgument(posBufDst)) and
+          msg =
+            "According to the definition of the functions, it is not guaranteed to write a null character at the end of the string, so access beyond the bounds of the destination buffer is possible."
+        )
+        or
+        exists(FunctionCall allocMem |
+          allocMem.getTarget().hasName(["calloc", "malloc"]) and
+          globalValueNumber(fc.getArgument(posBufDst)) = globalValueNumber(allocMem) and
+          (
+            allocMem.getArgument(allocMem.getNumberOfArguments() - 1).getValue() = "1" or
+            not exists(SizeofOperator sizeofOperator |
+              globalValueNumber(allocMem
+                    .getArgument(allocMem.getNumberOfArguments() - 1)
+                    .getAChild*()) = globalValueNumber(sizeofOperator)
+            )
+          ) and
+          msg =
+            "The buffer destination has a type other than char, you need to take this into account when allocating memory."
+        )
+        or
+        fc.getArgument(posBufDst).getValue() = "0" and
+        fc.getArgument(posSizeDst).getValue() != "0" and
+        msg =
+          "If the destination buffer is NULL and its size is not 0, then undefined behavior is possible."
+      )
+    )
+  )
+}
+
+from Expr exp, string msg
+where
+  findUseCharacterConversion(exp, msg) or
+  findUseMultibyteCharacter(exp, msg) or
+  findUseStringConversion(exp, msg, 1, 0, 2, ["mbstowcs", "_mbstowcs_l", "mbsrtowcs"]) or
+  findUseStringConversion(exp, msg, 2, 4, 5, ["MultiByteToWideChar", "WideCharToMultiByte"])
+select exp, msg

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-125/semmle/tests/DangerousWorksWithMultibyteOrWideCharacters.expected

@@ -0,0 +1,26 @@
+| test1.cpp:28:5:28:23 | call to WideCharToMultiByte | According to the definition of the functions, if the source buffer and the destination buffer are the same, undefined behavior is possible. |
+| test1.cpp:29:5:29:23 | call to MultiByteToWideChar | According to the definition of the functions, if the source buffer and the destination buffer are the same, undefined behavior is possible. |
+| test1.cpp:45:3:45:21 | call to WideCharToMultiByte | According to the definition of the functions, it is not guaranteed to write a null character at the end of the string, so access beyond the bounds of the destination buffer is possible. |
+| test1.cpp:58:3:58:21 | call to MultiByteToWideChar | The buffer destination has a type other than char, you need to take this into account when allocating memory. |
+| test1.cpp:70:3:70:21 | call to MultiByteToWideChar | The buffer destination has a type other than char, you need to take this into account when allocating memory. |
+| test1.cpp:76:10:76:28 | call to WideCharToMultiByte | If the destination buffer is NULL and its size is not 0, then undefined behavior is possible. |
+| test1.cpp:93:5:93:23 | call to WideCharToMultiByte | According to the definition of the functions, it is not guaranteed to write a null character at the end of the string, so access beyond the bounds of the destination buffer is possible. |
+| test2.cpp:15:5:15:12 | call to mbstowcs | According to the definition of the functions, if the source buffer and the destination buffer are the same, undefined behavior is possible. |
+| test2.cpp:17:5:17:15 | call to _mbstowcs_l | According to the definition of the functions, if the source buffer and the destination buffer are the same, undefined behavior is possible. |
+| test2.cpp:19:5:19:13 | call to mbsrtowcs | According to the definition of the functions, if the source buffer and the destination buffer are the same, undefined behavior is possible. |
+| test2.cpp:35:3:35:10 | call to mbstowcs | According to the definition of the functions, it is not guaranteed to write a null character at the end of the string, so access beyond the bounds of the destination buffer is possible. |
+| test2.cpp:48:3:48:10 | call to mbstowcs | The buffer destination has a type other than char, you need to take this into account when allocating memory. |
+| test2.cpp:60:3:60:10 | call to mbstowcs | The buffer destination has a type other than char, you need to take this into account when allocating memory. |
+| test2.cpp:66:10:66:17 | call to mbstowcs | If the destination buffer is NULL and its size is not 0, then undefined behavior is possible. |
+| test2.cpp:80:3:80:10 | call to mbstowcs | According to the definition of the functions, it is not guaranteed to write a null character at the end of the string, so access beyond the bounds of the destination buffer is possible. |
+| test3.cpp:16:5:16:13 | access to array | This buffer may contain multibyte characters, so attempting to copy may result in part of the last character being lost. |
+| test3.cpp:36:13:36:18 | ... + ... | This buffer may contain multibyte characters, so an attempt to copy may result in an overflow. |
+| test3.cpp:47:3:47:24 | access to array | The size of the array element is greater than one byte, so the offset will point outside the array. |
+| test.cpp:66:27:66:32 | call to mbtowc | Size can be less than maximum character length, use macro MB_CUR_MAX. |
+| test.cpp:76:27:76:32 | call to mbtowc | Size can be less than maximum character length, use macro MB_CUR_MAX. |
+| test.cpp:106:11:106:16 | call to mbtowc | Access beyond the allocated memory is possible, the length can change without changing the pointer. |
+| test.cpp:123:11:123:16 | call to mbtowc | Access beyond the allocated memory is possible, the length can change without changing the pointer. |
+| test.cpp:140:11:140:16 | call to mbtowc | Access beyond the allocated memory is possible, the length can change without changing the pointer. |
+| test.cpp:158:11:158:16 | call to mbtowc | Access beyond the allocated memory is possible, the length can change without changing the pointer. |
+| test.cpp:181:11:181:16 | call to mbtowc | Access beyond the allocated memory is possible, the length can change without changing the pointer. |
+| test.cpp:197:11:197:16 | call to mbtowc | Maybe you're using the function's return value incorrectly. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-125/semmle/tests/DangerousWorksWithMultibyteOrWideCharacters.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.ql