diff --git a/.gitignore b/.gitignore index 4b055e55a091..fdcd174a97c6 100644 --- a/.gitignore +++ b/.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 diff --git a/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.cpp b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.cpp new file mode 100644 index 000000000000..44896e89e153 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.cpp @@ -0,0 +1,7 @@ +LPMALLOC pMalloc; +HRESULT hr = CoGetMalloc(1, &pMalloc); + +if (!hr) +{ + // code ... +} diff --git a/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.qhelp b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.qhelp new file mode 100644 index 000000000000..d5b949691f01 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.qhelp @@ -0,0 +1,26 @@ + + + + +

This query indicates that an HRESULT is being cast to a boolean type or vice versa.

+

The typical success value (S_OK) of an HRESULT equals 0. However, 0 indicates failure for a boolean type.

+

Casting an HRESULT to a boolean type and then using it in a test expression will yield an incorrect result.

+ + + +

To check if a call that returns an HRESULT succeeded use the FAILED macro.

+ + + +

In the following example, HRESULT is used in a test expression incorrectly as it may yield an incorrect result.

+ + +

To fix this issue, use the FAILED macro in the test expression.

+ + + + + + diff --git a/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql new file mode 100644 index 000000000000..7678cff23f0f --- /dev/null +++ b/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 \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c new file mode 100644 index 000000000000..9edcd34a8df0 --- /dev/null +++ b/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 + { + // ... + } +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.cpp new file mode 100644 index 000000000000..04588c24264e --- /dev/null +++ b/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 + { + // ... + } +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.expected new file mode 100644 index 000000000000..15996702920f --- /dev/null +++ b/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 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.qlref new file mode 100644 index 000000000000..a345e5c6dfb3 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-253/HResultBooleanConversion.ql \ No newline at end of file