Merge pull request github#5938 from MathiasVP/promote-access-of-memory-location-after-end-of-buffer-using-strncat

C++: Promote `cpp/access-memory-location-after-end-buffer-strncat` out of experimental

Author: geoffw0

cpp/change-notes/2021-05-21-unsafe-strncat.md

@@ -0,0 +1,2 @@
+lgtm
+* The "Potentially unsafe call to strncat" query (cpp/unsafe-strncat) query has been improved to detect more cases of unsafe calls to `strncat`.

cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.cpp

@@ -2,3 +2,7 @@ strncat(dest, src, strlen(dest)); //wrong: should use remaining size of dest
 
 strncat(dest, src, sizeof(dest)); //wrong: should use remaining size of dest. 
                                   //Also fails if dest is a pointer and not an array.
+ 
+strncat(dest, source, sizeof(dest) - strlen(dest)); // wrong: writes a zero byte past the `dest` buffer.
+
+strncat(dest, source, sizeof(dest) - strlen(dest) - 1); // correct: reserves space for the zero byte.

cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.qhelp

@@ -4,7 +4,17 @@
 <qhelp>
 <overview>
 <p>The standard library function <code>strncat</code> appends a source string to a target string. 
-The third argument defines the maximum number of characters to append and should be less than or equal to the remaining space in the destination buffer. Calls of the form <code>strncat(dest, src, strlen(dest))</code> or <code>strncat(dest, src, sizeof(dest))</code> set the third argument to the entire size of the destination buffer. Executing a call of this type may cause a buffer overflow unless the buffer is known to be empty. Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
+The third argument defines the maximum number of characters to append and should be less than or equal
+to the remaining space in the destination buffer.</p>
+
+<p>Calls of the form <code>strncat(dest, src, strlen(dest))</code> or <code>strncat(dest, src, sizeof(dest))</code> set
+the third argument to the entire size of the destination buffer.
+Executing a call of this type may cause a buffer overflow unless the buffer is known to be empty.</p>
+
+<p>Similarly, calls of the form <code>strncat(dest, src, sizeof (dest) - strlen (dest))</code> allow one
+byte to be written ouside the <code>dest</code> buffer.</p>
+
+<p>Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
 
 </overview>
 <recommendation>
@@ -25,6 +35,10 @@ The third argument defines the maximum number of characters to append and should
 <li>
   M. Donaldson, <em>Inside the Buffer Overflow Attack: Mechanism, Method &amp; Prevention</em>. SANS Institute InfoSec Reading Room, 2002.
 </li>
+<li>
+  CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator">STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator</a>.
+</li>
 
 
 </references>

cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.ql

@@ -1,26 +1,68 @@
 /**
  * @name Potentially unsafe call to strncat
- * @description Calling 'strncat' with the size of the destination buffer
- *              as the third argument may result in a buffer overflow.
+ * @description Calling 'strncat' with an incorrect size argument may result in a buffer overflow.
  * @kind problem
  * @problem.severity warning
  * @precision medium
  * @id cpp/unsafe-strncat
  * @tags reliability
  *       correctness
  *       security
+ *       external/cwe/cwe-788
  *       external/cwe/cwe-676
  *       external/cwe/cwe-119
  *       external/cwe/cwe-251
  */
 
 import cpp
 import Buffer
+import semmle.code.cpp.models.implementations.Strcat
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
-from FunctionCall fc, VariableAccess va1, VariableAccess va2
-where
-  fc.getTarget().(Function).hasName("strncat") and
-  va1 = fc.getArgument(0) and
-  va2 = fc.getArgument(2).(BufferSizeExpr).getArg() and
-  va1.getTarget() = va2.getTarget()
+/**
+ * Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
+ * destination arguments, respectively.
+ */
+predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
+  exists(StrcatFunction strcat |
+    strcat = call.getTarget() and
+    sizeArg = call.getArgument(strcat.getParamSize()) and
+    destArg = call.getArgument(strcat.getParamDest())
+  )
+}
+
+/**
+ * Holds if `fc` is a call to `strncat` with size argument `sizeArg` and destination
+ * argument `destArg`, and `destArg` is the size of the buffer pointed to by `destArg`.
+ */
+predicate case1(FunctionCall fc, Expr sizeArg, VariableAccess destArg) {
+  interestringCallWithArgs(fc, sizeArg, destArg) and
+  exists(VariableAccess va |
+    va = sizeArg.(BufferSizeExpr).getArg() and
+    destArg.getTarget() = va.getTarget()
+  )
+}
+
+/**
+ * Holds if `fc` is a call to `strncat` with size argument `sizeArg` and destination
+ * argument `destArg`, and `sizeArg` computes the value `sizeof (dest) - strlen (dest)`.
+ */
+predicate case2(FunctionCall fc, Expr sizeArg, VariableAccess destArg) {
+  interestringCallWithArgs(fc, sizeArg, destArg) and
+  exists(SubExpr sub, int n |
+    // The destination buffer is an array of size n
+    destArg.getUnspecifiedType().(ArrayType).getSize() = n and
+    // The size argument is equivalent to a subtraction
+    globalValueNumber(sizeArg).getAnExpr() = sub and
+    // ... where the left side of the subtraction is the constant n
+    globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
+    // ... and the right side of the subtraction is a call to `strlen` where the argument is the
+    // destination buffer.
+    globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
+      globalValueNumber(destArg).getAnExpr()
+  )
+}
+
+from FunctionCall fc, Expr sizeArg, Expr destArg
+where case1(fc, sizeArg, destArg) or case2(fc, sizeArg, destArg)
 select fc, "Potentially unsafe call to strncat."

cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.c

@@ -1,9 +1,9 @@
-| test.c:54:3:54:24 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:55:3:55:40 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:56:3:56:44 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:57:3:57:44 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:58:3:58:48 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:59:3:59:48 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:60:3:60:52 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:61:3:61:50 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:62:3:62:54 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:16:3:16:24 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:17:3:17:40 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:18:3:18:44 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:19:3:19:44 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:20:3:20:48 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:21:3:21:48 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:22:3:22:52 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:23:3:23:50 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:24:3:24:54 | ... = ... | potential unsafe or redundant assignment. |