C++: Merge the experimental query 'cpp/access-memory-location-after-end-buffer-strncat' into 'cpp/unsafe-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,11 @@
 <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.
+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.
+Similarly, calls of the form <code>strncat(dest, src, sizeof (dest) - strlen (dest))</code> allows one byte to be written ouside the `dest` buffer.
+Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
 
 </overview>
 <recommendation>
@@ -25,6 +29,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,7 +1,7 @@
 /**
  * @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 the size of the destination buffer as the third argument may result in a buffer overflow.
+ *              Similarly, calling `strncat` with `sizeof (dest) - strlen (dest)` as the third argument may result in a buffer overflow.
  * @kind problem
  * @problem.severity warning
  * @precision medium
@@ -9,6 +9,7 @@
  * @tags reliability
  *       correctness
  *       security
+ *       external/cwe/cwe-788
  *       external/cwe/cwe-676
  *       external/cwe/cwe-119
  *       external/cwe/cwe-251
@@ -16,11 +17,55 @@
 
 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(StrcatFunction strncat, VariableAccess va |
+    fc.getTarget() = strncat and
+    destArg = fc.getArgument(strncat.getParamDest()) and
+    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."