Merge pull request #695 from raulgarciamsft/users/raulga/c6324

cpp - Using the return value of a strcpy or related string copy function in an if statement

cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp

@@ -0,0 +1,4 @@
+if(strcpy(szbuf1, "Manager") == 0) // most likely strcmp was intended instead of strcpy
+{
+	// ...
+}

cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.qhelp

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>This rule finds uses of the string copy function calls that return the <code>destination</code> parameter, 
+and that do not have a return value reserved to indicate an error.</p>
+
+<p>The rule flags occurrences using such string copy functions as the conditional of an <code>if</code> statement, either directly, as part of an equality operator or a logical operator.</p>
+  
+<p>The string copy functions that the rule takes into consideration are: </p>
+<ul>
+  <li>strcpy</li>
+  <li>wcscpy</li>
+  <li>_mbscpy</li>
+  <li>strncpy</li>
+  <li>_strncpy_l</li>
+  <li>wcsncpy</li>
+  <li>_wcsncpy_l</li>
+  <li>_mbsncpy</li>
+  <li>_mbsncpy_l</li>
+</ul>
+  
+<p>NOTE: It is highly recommended to consider using a more secure version of string manipulation functions suchas as <code>strcpy_s</code>.</p>
+
+</overview>
+<recommendation>
+  <p>Check to ensure that the flagged expressions are not typos.</p>
+  <p>If a string comparison is intended, change the function to the appropriate string comparison function.</p>
+  <p>If a string copy is really intended, very likely a secure version of the string copy function such as <code>strcpy_s</code> was intended instead of the insecure version of the string copy function.</p>
+
+</recommendation>
+<example><sample src="UsingStrcpyAsBoolean.cpp" />
+</example>
+
+<references>
+<li>Microsoft Books on Line: <a href="https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2012/ccf4h9w8(v=vs.110)">C6324</a></li>
+<li>Microsoft Books on Line: <a href="https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strcpy-wcscpy-mbscpy?view=vs-2017">strcpy, wcscpy, _mbscpy</a></li>
+<li>US-CERT: <a href="https://www.us-cert.gov/bsi/articles/knowledge/coding-practices/strcpy_s-and-strcat_s">strncpy_s() and strncat_s()</a></li>
+
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql

@@ -0,0 +1,77 @@
+/**
+ * @name Using the return value of a strcpy or related string copy function as a boolean operator
+ * @description The return value for strcpy, strncpy, or related string copy functions have no reserved return value to indicate an error.
+ *              Using the return values of these functions as boolean function .
+ *              Either the intent was to use a more secure version of a string copy function (such as strcpy_s), or a string compare function (such as strcmp).
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/string-copy-return-value-as-boolean
+ * @tags external/microsoft/C6324
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.DataFlow
+
+predicate isStringComparisonFunction(string functionName) {
+  functionName = "strcpy"
+  or functionName = "wcscpy"
+  or functionName = "_mbscpy"
+  or functionName = "strncpy"
+  or functionName = "_strncpy_l"
+  or functionName = "wcsncpy"
+  or functionName = "_wcsncpy_l"
+  or functionName = "_mbsncpy"
+  or functionName = "_mbsncpy_l"
+}
+
+predicate isBoolean( Expr e1 )
+{
+  exists ( Type t1 |
+    t1 = e1.getType() and
+    (t1.hasName("bool") or t1.hasName("BOOL") or t1.hasName("_Bool"))
+  )
+}
+
+predicate isStringCopyCastedAsBoolean( FunctionCall func, Expr expr1, string msg ) {
+  DataFlow::localFlow(DataFlow::exprNode(func), DataFlow::exprNode(expr1))
+  and isBoolean( expr1.getConversion*())
+  and isStringComparisonFunction( func.getTarget().getQualifiedName())
+  and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used as boolean."
+}
+
+predicate isStringCopyUsedInLogicalOperationOrCondition( FunctionCall func, Expr expr1, string msg ) {
+    isStringComparisonFunction( func.getTarget().getQualifiedName() )
+    and ((( 
+      // it is being used in an equality or logical operation 
+      exists( EqualityOperation eop |
+        eop = expr1
+        and func = eop.getAChild()
+      )
+      or exists( UnaryLogicalOperation ule |
+        expr1 = ule
+        and func = ule.getAChild()
+      )
+      or exists( BinaryLogicalOperation ble |
+        expr1 = ble
+        and func = ble.getAChild()
+      )
+    )
+    and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used in a logical operation."
+    )
+    or
+      exists( ConditionalStmt condstmt |
+      condstmt.getAChild() = expr1 |
+      // or the string copy function is used directly as the conditional expression
+      func = condstmt.getChild(0)
+      and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used directly in a conditional expression."
+    ))
+}
+
+from FunctionCall func, Expr expr1, string msg
+where 
+  ( isStringCopyCastedAsBoolean(func, expr1, msg) and 
+    not isStringCopyUsedInLogicalOperationOrCondition(func, expr1, _)
+  )
+  or isStringCopyUsedInLogicalOperationOrCondition(func, expr1, msg)
+select expr1, msg