Cast between semantically different integer types: HRESULT to/from a Boolean type.

Closing the gap between Semmle and PreFast.
Covers C6214, C6215, C6216, C6217, C6230

Author: raulgarciamsft

.gitignore

@@ -8,3 +8,7 @@
 # qltest projects and artifacts
 */ql/test/**/*.testproj
 */ql/test/**/*.actual
+/.vs/slnx.sqlite
+/.vs/ql2/v15/Browse.VC.opendb
+/.vs/ql2/v15/Browse.VC.db
+/.vs/ProjectSettings.json

cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.cpp

@@ -0,0 +1,7 @@
+LPMALLOC pMalloc;
+HRESULT hr = CoGetMalloc(1, &pMalloc);
+
+if (!hr)
+{
+	// code ...
+}

cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.qhelp

@@ -0,0 +1,26 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+  <p>This query indicates that an <code>HRESULT</code> is being cast to a boolean type or vice versa.</p>
+  <p>The typical success value (<code>S_OK</code>) of an <code>HRESULT</code> equals 0. However, 0 indicates failure for a boolean type.</p>
+  <p>Casting an <code>HRESULT</code> to a boolean type and then using it in a test expression will yield an incorrect result.</p>
+</overview>
+
+<recommendation>
+<p>To check if a call that returns an HRESULT succeeded use the <code>FAILED</code> macro.</p>
+</recommendation>
+
+<example>
+<p>In the following example, <code>HRESULT</code> is used in a test expression incorrectly as it may yield an incorrect result.</p>
+<sample src="IncorrectTypeConversion.cpp" />
+
+<p>To fix this issue, use the <code>FAILED</code> macro in the test expression.</p>
+</example>
+
+<references>
+</references>
+
+</qhelp>

cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.ql

@@ -0,0 +1,30 @@
+/**
+ * @name Cast between semantically different integer types: HRESULT to/from a Boolean type
+ * @description Cast between semantically different integer types: HRESULT to/from a Boolean type.
+ *              Boolean types indicate success by a non-zero value, whereas success (S_OK) in HRESULT is indicated by a value of 0. 
+ *              Casting an HRESULT to/from a Boolean type and then using it in a test expression will yield an incorrect result.
+ * @kind problem
+ * @id cpp/incorrect-type-conversion
+ * @problem.severity error
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-704
+ *       external/microsoft/C6214
+ *       external/microsoft/C6215
+ *       external/microsoft/C6216
+ *       external/microsoft/C6217
+ *       external/microsoft/C6230
+ */
+import cpp
+
+from Expr e1, Cast e2, string msg
+where e2 = e1.getConversion() and
+  exists ( Type t1, Type t2 |
+    t1 = e1.getType() and
+    t2 = e2.getType() and 
+    ((t1.hasName("bool") or t1.hasName("BOOL")) and t2.hasName("HRESULT") or 
+     (t2.hasName("bool") or t2.hasName("BOOL")) and t1.hasName("HRESULT") 
+  ))
+  and if e2.isImplicit() then ( msg = "Implicit" ) 
+      else ( msg = "Explicit" )
+select e1, msg + " conversion from " + e1.getType().toString() + " to " +  e2.getType().toString() 

cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.cpp

@@ -0,0 +1,76 @@
+// winnt.h
+typedef long HRESULT; 
+#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)
+#define FAILED(hr) (((HRESULT)(hr)) < 0)
+
+// minwindef.h
+typedef int                 BOOL; 
+#ifndef FALSE
+#define FALSE               0
+#endif
+#ifndef TRUE
+#define TRUE                1
+#endif
+
+// winerror.h
+#define S_OK                                   ((HRESULT)0L)
+#define S_FALSE                                ((HRESULT)1L)
+#define _HRESULT_TYPEDEF_(_sc) ((HRESULT)_sc)
+#define E_UNEXPECTED                     _HRESULT_TYPEDEF_(0x8000FFFFL)
+
+HRESULT HresultFunction()
+{
+	return S_OK;
+}
+
+BOOL BoolFunction()
+{
+	return FALSE;
+}
+
+HRESULT IncorrectHresultFunction()
+{
+	return BoolFunction(); // BUG
+}
+
+void IncorrectTypeConversionTest() {
+	HRESULT hr = HresultFunction();
+	if ((BOOL)hr) // BUG
+	{
+		// ...
+	}
+	if (SUCCEEDED(hr)) // Correct Usage
+	{
+		// ...
+	}
+
+	if (SUCCEEDED(BoolFunction())) // BUG
+	{
+		// ...
+	}
+	if (BoolFunction()) // Correct Usage
+	{
+		// ...
+	}
+	BOOL b = IncorrectHresultFunction(); // BUG
+
+	hr = E_UNEXPECTED;
+	if (!hr) // BUG
+	{
+		// ...
+	}
+	if (!FAILED(hr)) // Correct Usage
+	{
+		// ...
+	}
+
+	hr = S_FALSE;
+	if (hr) // Should fail
+	{
+		// ...
+	}
+	if (SUCCEEDED(hr)) // Correct Usage
+	{
+		// ...
+	}
+}

cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.expected

@@ -0,0 +1,6 @@
+| IncorrectTypeConversion.cpp:33:9:33:20 | call to BoolFunction | Implicit conversion from BOOL to HRESULT |
+| IncorrectTypeConversion.cpp:38:12:38:13 | hr | Explicit conversion from HRESULT to BOOL |
+| IncorrectTypeConversion.cpp:47:6:47:30 | (...) | Explicit conversion from BOOL to HRESULT |
+| IncorrectTypeConversion.cpp:55:11:55:34 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to BOOL |
+| IncorrectTypeConversion.cpp:58:7:58:8 | hr | Implicit conversion from HRESULT to bool |
+| IncorrectTypeConversion.cpp:68:6:68:7 | hr | Implicit conversion from HRESULT to bool |

cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-704/incorrectTypeConversion.ql