Cast between semantically different integer types: HRESULT to/from bool

.gitignore

@@ -12,3 +12,6 @@
 /.vs/ql/v15/Browse.VC.opendb
 /.vs/ql/v15/Browse.VC.db
 /.vs/ProjectSettings.json
+/.vs/slnx.sqlite-journal
+/.vs/ql2/v15/Browse.VC.opendb
+/.vs/ql2/v15/Browse.VC.db

cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.cpp

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

cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.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="HResultBooleanConversion.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-253/HResultBooleanConversion.ql

@@ -0,0 +1,71 @@
+/**
+ * @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/hresult-boolean-conversion
+ * @problem.severity error
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-253
+ *       external/microsoft/C6214
+ *       external/microsoft/C6215
+ *       external/microsoft/C6216
+ *       external/microsoft/C6217
+ *       external/microsoft/C6230
+ */
+import cpp
+
+predicate isHresultBooleanConverted( Expr e1, Cast e2 )
+{
+  exists ( Type t1, Type t2 |
+    t1 = e1.getType() and
+    t2 = e2.getType() and 
+    ((t1.hasName("bool") or t1.hasName("BOOL") or t1.hasName("_Bool")) and t2.hasName("HRESULT") or 
+     (t2.hasName("bool") or t2.hasName("BOOL") or t2.hasName("_Bool")) and t1.hasName("HRESULT") 
+  ))
+}
+
+predicate isHresultBooleanConverted( Expr e1 )
+{
+  exists( Cast e2 | 
+    e2 = e1.getConversion() and
+    isHresultBooleanConverted(e1, e2)
+  )
+}
+
+from Expr e1, string msg
+where exists 
+  ( 
+    Cast e2  | 
+    e2 = e1.getConversion() |
+    isHresultBooleanConverted( e1, e2 )
+    and if e2.isImplicit() then ( msg = "Implicit conversion from " + e1.getType().toString() + " to " +  e2.getType().toString()) 
+      else ( msg = "Explicit conversion from " + e1.getType().toString() + " to " +  e2.getType().toString())
+  ) 
+  or exists
+  ( 
+    ControlStructure ctls | 
+    ctls.getControllingExpr() = e1
+    and e1.getType().(TypedefType).hasName("HRESULT")
+    and not isHresultBooleanConverted(e1)
+    and msg = "Direct usage of a type " + e1.getType().toString() + " as a conditional expression"
+  ) 
+  or
+  (
+    exists( BinaryLogicalOperation blop | 
+      blop.getAnOperand() = e1 |
+      e1.getType().(TypedefType).hasName("HRESULT")
+      and msg = "Usage of a type " + e1.getType().toString() + " as an argument of a binary logical operation"
+    )
+    or exists
+    (
+      UnaryLogicalOperation ulop | 
+      ulop.getAnOperand() = e1 |
+      e1.getType().(TypedefType).hasName("HRESULT")
+      and msg = "Usage of a type " + e1.getType().toString() + " as an argument of a unary logical operation"
+    )
+    and not isHresultBooleanConverted(e1)
+  )
+select e1, msg

cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c

@@ -0,0 +1,100 @@
+// semmle-extractor-options: --microsoft
+// winnt.h
+typedef long HRESULT;
+#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)
+#define FAILED(hr) (((HRESULT)(hr)) < 0)
+
+typedef _Bool    bool;
+#define FALSE 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;
+}
+
+bool BoolFunction2()
+{
+    return FALSE;
+}
+
+HRESULT IncorrectHresultFunction()
+{
+    return BoolFunction(); // BUG
+}
+
+HRESULT IncorrectHresultFunction2()
+{
+    return BoolFunction2(); // BUG
+}
+
+void IncorrectTypeConversionTest() {
+
+    HRESULT hr = HresultFunction();
+    if ((BOOL)hr) // BUG
+    {
+        // ...
+    }
+    if ((bool)hr) // BUG
+    {
+        // ...
+    }
+    if (SUCCEEDED(hr)) // Correct Usage
+    {
+        // ...
+    }
+
+    if (SUCCEEDED(BoolFunction())) // BUG
+    {
+        // ...
+    }
+    if (SUCCEEDED(BoolFunction2())) // BUG
+    {
+        // ...
+    }
+    if (BoolFunction()) // Correct Usage
+    {
+        // ...
+    }
+    BOOL b = IncorrectHresultFunction(); // BUG
+    bool b2 = IncorrectHresultFunction(); // BUG
+
+    hr = E_UNEXPECTED;
+    if (!hr) // BUG
+    {
+        // ...
+    }
+    if (!FAILED(hr)) // Correct Usage
+    {
+        // ...
+    }
+
+    hr = S_FALSE;
+    if (hr) // BUG
+    {
+        // ...
+    }
+    if (SUCCEEDED(hr)) // Correct Usage
+    {
+        // ...
+    }
+}

cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.cpp

@@ -0,0 +1,97 @@
+// semmle-extractor-options: --microsoft
+// 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;
+}
+
+bool BoolFunction2()
+{
+    return FALSE;
+}
+
+HRESULT IncorrectHresultFunction()
+{
+    return BoolFunction(); // BUG
+}
+
+HRESULT IncorrectHresultFunction2()
+{
+    return BoolFunction2(); // BUG
+}
+
+void IncorrectTypeConversionTest() {
+
+    HRESULT hr = HresultFunction();
+    if ((BOOL)hr) // BUG
+    {
+        // ...
+    }
+    if ((bool)hr) // BUG
+    {
+        // ...
+    }
+    if (SUCCEEDED(hr)) // Correct Usage
+    {
+        // ...
+    }
+
+    if (SUCCEEDED(BoolFunction())) // BUG
+    {
+        // ...
+    }
+    if (SUCCEEDED(BoolFunction2())) // BUG
+    {
+        // ...
+    }
+    if (BoolFunction()) // Correct Usage
+    {
+        // ...
+    }
+    BOOL b = IncorrectHresultFunction(); // BUG
+    bool b2 = IncorrectHresultFunction(); // BUG
+
+    hr = E_UNEXPECTED;
+    if (!hr) // BUG
+    {
+        // ...
+    }
+    if (!FAILED(hr)) // Correct Usage
+    {
+        // ...
+    }
+
+    hr = S_FALSE;
+    if (hr) // BUG
+    {
+        // ...
+    }
+    if (SUCCEEDED(hr)) // Correct Usage
+    {
+        // ...
+    }
+}

cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.expected

@@ -0,0 +1,20 @@
+| HResultBooleanConversion.c:42:12:42:23 | call to BoolFunction | Implicit conversion from BOOL to HRESULT |
+| HResultBooleanConversion.c:47:12:47:24 | call to BoolFunction2 | Implicit conversion from bool to HRESULT |
+| HResultBooleanConversion.c:53:15:53:16 | hr | Explicit conversion from HRESULT to BOOL |
+| HResultBooleanConversion.c:57:15:57:16 | hr | Explicit conversion from HRESULT to bool |
+| HResultBooleanConversion.c:66:9:66:33 | (...) | Explicit conversion from BOOL to HRESULT |
+| HResultBooleanConversion.c:70:9:70:34 | (...) | Explicit conversion from bool to HRESULT |
+| HResultBooleanConversion.c:78:14:78:37 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to BOOL |
+| HResultBooleanConversion.c:79:15:79:38 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to bool |
+| HResultBooleanConversion.c:82:10:82:11 | hr | Usage of a type HRESULT as an argument of a unary logical operation |
+| HResultBooleanConversion.c:92:9:92:10 | hr | Direct usage of a type HRESULT as a conditional expression |
+| HResultBooleanConversion.cpp:39:12:39:23 | call to BoolFunction | Implicit conversion from BOOL to HRESULT |
+| HResultBooleanConversion.cpp:44:12:44:24 | call to BoolFunction2 | Implicit conversion from bool to HRESULT |
+| HResultBooleanConversion.cpp:50:15:50:16 | hr | Explicit conversion from HRESULT to BOOL |
+| HResultBooleanConversion.cpp:54:15:54:16 | hr | Explicit conversion from HRESULT to bool |
+| HResultBooleanConversion.cpp:63:9:63:33 | (...) | Explicit conversion from BOOL to HRESULT |
+| HResultBooleanConversion.cpp:67:9:67:34 | (...) | Explicit conversion from bool to HRESULT |
+| HResultBooleanConversion.cpp:75:14:75:37 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to BOOL |
+| HResultBooleanConversion.cpp:76:15:76:38 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to bool |
+| HResultBooleanConversion.cpp:79:10:79:11 | hr | Implicit conversion from HRESULT to bool |
+| HResultBooleanConversion.cpp:89:9:89:10 | hr | Implicit conversion from HRESULT to bool |

cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-253/HResultBooleanConversion.ql