diff --git a/.gitignore b/.gitignore index 4b055e55a09..fdcd174a97c 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-704/IncorrectTypeConversion.cpp b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.cpp similarity index 81% rename from cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.cpp rename to cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.cpp index bf711c9b288..44896e89e15 100644 --- a/cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.cpp +++ b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.cpp @@ -3,5 +3,5 @@ HRESULT hr = CoGetMalloc(1, &pMalloc); if (!hr) { - // code ... + // code ... } diff --git a/cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.qhelp b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.qhelp similarity index 100% rename from cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.qhelp rename to cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.qhelp 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 00000000000..7678cff23f0 --- /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/src/Security/CWE/CWE-704/IncorrectTypeConversion.ql b/cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.ql deleted file mode 100644 index 0ff7502d9a9..00000000000 --- a/cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.ql +++ /dev/null @@ -1,30 +0,0 @@ -/** - * @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-to-boolean-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() \ 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 00000000000..9edcd34a8df --- /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 00000000000..04588c24264 --- /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 00000000000..15996702920 --- /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 00000000000..a345e5c6dfb --- /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 diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.cpp deleted file mode 100644 index 8ecab54b4df..00000000000 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.cpp +++ /dev/null @@ -1,76 +0,0 @@ -// 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) // BUG - { - // ... - } - if (SUCCEEDED(hr)) // Correct Usage - { - // ... - } -} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.expected deleted file mode 100644 index 35c40158a77..00000000000 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.expected +++ /dev/null @@ -1,6 +0,0 @@ -| 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 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.qlref deleted file mode 100644 index 56b47b192d7..00000000000 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-704/IncorrectTypeConversion.ql \ No newline at end of file