Merge pull request #5009 from ihsinme/ihsinme-patch-219

CPP: add query for CWE-788 Access of memory location after the end of a buffer using strncat.

Author: MathiasVP

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

@@ -0,0 +1,4 @@
+ 
+strncat(dest, source, sizeof(dest) - strlen(dest)); // BAD: writes a zero byte past the `dest` buffer.
+
+strncat(dest, source, sizeof(dest) - strlen(dest) -1); // GOOD: Reserves space for the zero byte.

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

@@ -0,0 +1,32 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The standard library function <code>strncat(dest, source, count)</code> appends the <code>source</code> string to the <code>dest</code> string. <code>count</code> specifies the maximum number of characters to append and must be less than the remaining space in the target buffer. Calls of the form <code> strncat (dest, source, sizeof (dest) - strlen (dest)) </code> set the third argument to one more than possible. So when the <code>dest</code> is full, the expression <code> sizeof (dest) - strlen (dest) </code> will be equal to one, and not zero as the programmer might think. Making a call of this type may result in a zero byte being written just outside the <code>dest</code> buffer.</p>
+
+
+</overview>
+<recommendation>
+
+<p>We recommend subtracting one from the third argument. For example, replace <code>strncat(dest, source, sizeof(dest)-strlen(dest))</code> with <code>strncat(dest, source, sizeof(dest)-strlen(dest)-1)</code>.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates an erroneous and corrected use of the <code>strncat</code> function.</p>
+<sample src="AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.c" />
+
+</example>
+<references>
+
+<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>
+<li>
+  CERT C 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</a>.
+</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,64 @@
+/**
+ * @name Access Of Memory Location After The End Of A Buffer Using Strncat
+ * @description Calls of the form `strncat(dest, source, sizeof (dest) - strlen (dest))` set the third argument to one more than possible. So when `dest` is full, the expression `sizeof(dest) - strlen (dest)` will be equal to one, and not zero as the programmer might think. Making a call of this type may result in a zero byte being written just outside the `dest` buffer.
+ * @kind problem
+ * @id cpp/access-memory-location-after-end-buffer
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-788
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/**
+ * A call to `strncat` of the form `strncat(buff, str, someExpr - strlen(buf))`, for some expression `someExpr` equal to `sizeof(buff)`.
+ */
+class WrongCallStrncat extends FunctionCall {
+  Expr leftsomeExpr;
+
+  WrongCallStrncat() {
+    this.getTarget().hasGlobalOrStdName("strncat") and
+    // the expression of the first argument in `strncat` and `strnlen` is identical
+    globalValueNumber(this.getArgument(0)) =
+      globalValueNumber(this.getArgument(2).(SubExpr).getRightOperand().(StrlenCall).getStringExpr()) and
+    // using a string constant often speaks of manually calculating the length of the required buffer.
+    (
+      not this.getArgument(1) instanceof StringLiteral and
+      not this.getArgument(1) instanceof CharLiteral
+    ) and
+    // for use in predicates
+    leftsomeExpr = this.getArgument(2).(SubExpr).getLeftOperand()
+  }
+
+  /**
+   * Holds if the left side of the expression `someExpr` equal to `sizeof(buf)`.
+   */
+  predicate isExpressionEqualSizeof() {
+    // the left side of the expression `someExpr` is `sizeof(buf)`.
+    globalValueNumber(this.getArgument(0)) =
+      globalValueNumber(leftsomeExpr.(SizeofExprOperator).getExprOperand())
+    or
+    // value of the left side of the expression `someExpr` equal  `sizeof(buf)` value, and `buf` is array.
+    leftsomeExpr.getValue().toInt() = this.getArgument(0).getType().getSize()
+  }
+
+  /**
+   * Holds if the left side of the expression `someExpr` equal to variable containing the length of the memory allocated for the buffer.
+   */
+  predicate isVariableEqualValueSizegBuffer() {
+    // the left side of expression `someExpr` is the variable that was used in the function of allocating memory for the buffer`.
+    exists(AllocationExpr alc |
+      leftsomeExpr.(VariableAccess).getTarget() =
+        alc.(FunctionCall).getArgument(0).(VariableAccess).getTarget()
+    )
+  }
+}
+
+from WrongCallStrncat sc
+where
+  sc.isExpressionEqualSizeof() or
+  sc.isVariableEqualValueSizegBuffer()
+select sc, "if the used buffer is full, writing out of the buffer is possible"

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.expected

@@ -0,0 +1,3 @@
+| test.c:4:3:4:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
+| test.c:11:3:11:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
+| test.c:19:3:19:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.c

@@ -0,0 +1,28 @@
+void workFunction_0(char *s) {
+  char buf[80];
+  strncat(buf, s, sizeof(buf)-strlen(buf)-1); // GOOD
+  strncat(buf, s, sizeof(buf)-strlen(buf));  // BAD
+  strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD [NOT DETECTED] 
+}
+void workFunction_1(char *s) {
+#define MAX_SIZE 80
+  char buf[MAX_SIZE];
+  strncat(buf, s, MAX_SIZE-strlen(buf)-1); // GOOD
+  strncat(buf, s, MAX_SIZE-strlen(buf));  // BAD
+  strncat(buf, "fix", MAX_SIZE-strlen(buf)); // BAD [NOT DETECTED]
+}
+void workFunction_2_0(char *s) {
+  char * buf;
+  int len=80;
+  buf = (char *) malloc(len);
+  strncat(buf, s, len-strlen(buf)-1); // GOOD
+  strncat(buf, s, len-strlen(buf));  // BAD
+  strncat(buf, "fix", len-strlen(buf)); // BAD [NOT DETECTED]
+}
+void workFunction_2_1(char *s) {
+  char * buf;
+  int len=80;
+  buf = (char *) malloc(len+1);
+  strncat(buf, s, len-strlen(buf)-1); // GOOD
+  strncat(buf, s, len-strlen(buf));  // GOOD
+}