From 298ead162df7079479ee96ddb400870ee9b24cee Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 19 Oct 2018 09:40:13 +0100 Subject: [PATCH 1/2] CPP: Add more test cases for HResultBooleanConversion.ql. --- .../CWE/CWE-253/HResultBooleanConversion.c | 22 +++++++++++++++++++ .../CWE/CWE-253/HResultBooleanConversion.cpp | 22 +++++++++++++++++++ .../CWE-253/HResultBooleanConversion.expected | 4 ++++ 3 files changed, 48 insertions(+) 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 index 9edcd34a8df0..750c9ec0f131 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c @@ -97,4 +97,26 @@ void IncorrectTypeConversionTest() { { // ... } + + if (HresultFunction() == S_FALSE) // Correct Usage + { + // ... + } + + while (!HresultFunction()) {}; // BUG + while (FAILED(HresultFunction())) {}; // Correct Usage + + switch(hr) // Correct Usage [FALSE POSITIVE] + { + case S_OK: + case S_FALSE: + { + // ... + } break; + + default: + { + // ... + } break; + } } \ 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 index 04588c24264e..2c1c6e46ccd3 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.cpp @@ -94,4 +94,26 @@ void IncorrectTypeConversionTest() { { // ... } + + if (HresultFunction() == S_FALSE) // Correct Usage + { + // ... + } + + while (!HresultFunction()) {}; // BUG + while (FAILED(HresultFunction())) {}; // Correct Usage + + switch(hr) // Correct Usage [FALSE POSITIVE] + { + case S_OK: + case S_FALSE: + { + // ... + } break; + + default: + { + // ... + } break; + } } \ 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 index 15996702920f..306d39abf872 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.expected @@ -8,6 +8,8 @@ | 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.c:106:13:106:27 | call to HresultFunction | Usage of a type HRESULT as an argument of a unary logical operation | +| HResultBooleanConversion.c:109:12:109:13 | 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 | @@ -18,3 +20,5 @@ | 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 | +| HResultBooleanConversion.cpp:103:13:103:27 | call to HresultFunction | Implicit conversion from HRESULT to bool | +| HResultBooleanConversion.cpp:106:12:106:13 | hr | Direct usage of a type HRESULT as a conditional expression | From e9499b59e4d8e8c2c332b35799c743b0d9c26495 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 19 Oct 2018 10:16:28 +0100 Subject: [PATCH 2/2] CPP: Exclude switch statements. --- cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql | 1 + .../query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c | 2 +- .../Security/CWE/CWE-253/HResultBooleanConversion.cpp | 2 +- .../Security/CWE/CWE-253/HResultBooleanConversion.expected | 2 -- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql index 165fe0eed969..810879e262f0 100644 --- a/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql +++ b/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql @@ -48,6 +48,7 @@ where exists ctls.getControllingExpr() = e1 and e1.getType().(TypedefType).hasName("HRESULT") and not isHresultBooleanConverted(e1) + and not ctls instanceof SwitchStmt // not controlled by a boolean condition and msg = "Direct usage of a type " + e1.getType().toString() + " as a conditional expression" ) or 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 index 750c9ec0f131..732fd5f0f443 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.c @@ -106,7 +106,7 @@ void IncorrectTypeConversionTest() { while (!HresultFunction()) {}; // BUG while (FAILED(HresultFunction())) {}; // Correct Usage - switch(hr) // Correct Usage [FALSE POSITIVE] + switch(hr) // Correct Usage { case S_OK: case S_FALSE: 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 index 2c1c6e46ccd3..d2857226bfaa 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.cpp @@ -103,7 +103,7 @@ void IncorrectTypeConversionTest() { while (!HresultFunction()) {}; // BUG while (FAILED(HresultFunction())) {}; // Correct Usage - switch(hr) // Correct Usage [FALSE POSITIVE] + switch(hr) // Correct Usage { case S_OK: case S_FALSE: 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 index 306d39abf872..6968dbb1c895 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-253/HResultBooleanConversion.expected @@ -9,7 +9,6 @@ | 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.c:106:13:106:27 | call to HresultFunction | Usage of a type HRESULT as an argument of a unary logical operation | -| HResultBooleanConversion.c:109:12:109:13 | 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 | @@ -21,4 +20,3 @@ | 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 | | HResultBooleanConversion.cpp:103:13:103:27 | call to HresultFunction | Implicit conversion from HRESULT to bool | -| HResultBooleanConversion.cpp:106:12:106:13 | hr | Direct usage of a type HRESULT as a conditional expression |