Skip to content

Commit 8f19efe

Browse files
jbjjbj
authored
Merge pull request #211 from raulgarciamsft/users/raulga/HESULT
Cast between semantically different integer types: HRESULT to/from bool
2 parents 4732526 + c3b523c commit 8f19efe

File tree

8 files changed

+325
-0
lines changed

8 files changed

+325
-0
lines changed

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,6 @@
1212
/.vs/ql/v15/Browse.VC.opendb
1313
/.vs/ql/v15/Browse.VC.db
1414
/.vs/ProjectSettings.json
15+
/.vs/slnx.sqlite-journal
16+
/.vs/ql2/v15/Browse.VC.opendb
17+
/.vs/ql2/v15/Browse.VC.db

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
LPMALLOC pMalloc;
2+
HRESULT hr = CoGetMalloc(1, &pMalloc);
3+
4+
if (!hr)
5+
{
6+
// code ...
7+
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>This query indicates that an <code>HRESULT</code> is being cast to a boolean type or vice versa.</p>
8+
<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>
9+
<p>Casting an <code>HRESULT</code> to a boolean type and then using it in a test expression will yield an incorrect result.</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>To check if a call that returns an HRESULT succeeded use the <code>FAILED</code> macro.</p>
14+
</recommendation>
15+
16+
<example>
17+
<p>In the following example, <code>HRESULT</code> is used in a test expression incorrectly as it may yield an incorrect result.</p>
18+
<sample src="HResultBooleanConversion.cpp" />
19+
20+
<p>To fix this issue, use the <code>FAILED</code> macro in the test expression.</p>
21+
</example>
22+
23+
<references>
24+
</references>
25+
26+
</qhelp>

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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/**
2+
* @name Cast between semantically different integer types: HRESULT to/from a Boolean type
3+
* @description Cast between semantically different integer types: HRESULT to/from a Boolean type.
4+
* Boolean types indicate success by a non-zero value, whereas success (S_OK) in HRESULT is indicated by a value of 0.
5+
* Casting an HRESULT to/from a Boolean type and then using it in a test expression will yield an incorrect result.
6+
* @kind problem
7+
* @id cpp/hresult-boolean-conversion
8+
* @problem.severity error
9+
* @precision high
10+
* @tags security
11+
* external/cwe/cwe-253
12+
* external/microsoft/C6214
13+
* external/microsoft/C6215
14+
* external/microsoft/C6216
15+
* external/microsoft/C6217
16+
* external/microsoft/C6230
17+
*/
18+
import cpp
19+
20+
predicate isHresultBooleanConverted( Expr e1, Cast e2 )
21+
{
22+
exists ( Type t1, Type t2 |
23+
t1 = e1.getType() and
24+
t2 = e2.getType() and
25+
((t1.hasName("bool") or t1.hasName("BOOL") or t1.hasName("_Bool")) and t2.hasName("HRESULT") or
26+
(t2.hasName("bool") or t2.hasName("BOOL") or t2.hasName("_Bool")) and t1.hasName("HRESULT")
27+
))
28+
}
29+
30+
predicate isHresultBooleanConverted( Expr e1 )
31+
{
32+
exists( Cast e2 |
33+
e2 = e1.getConversion() and
34+
isHresultBooleanConverted(e1, e2)
35+
)
36+
}
37+
38+
from Expr e1, string msg
39+
where exists
40+
(
41+
Cast e2 |
42+
e2 = e1.getConversion() |
43+
isHresultBooleanConverted( e1, e2 )
44+
and if e2.isImplicit() then ( msg = "Implicit conversion from " + e1.getType().toString() + " to " + e2.getType().toString())
45+
else ( msg = "Explicit conversion from " + e1.getType().toString() + " to " + e2.getType().toString())
46+
)
47+
or exists
48+
(
49+
ControlStructure ctls |
50+
ctls.getControllingExpr() = e1
51+
and e1.getType().(TypedefType).hasName("HRESULT")
52+
and not isHresultBooleanConverted(e1)
53+
and msg = "Direct usage of a type " + e1.getType().toString() + " as a conditional expression"
54+
)
55+
or
56+
(
57+
exists( BinaryLogicalOperation blop |
58+
blop.getAnOperand() = e1 |
59+
e1.getType().(TypedefType).hasName("HRESULT")
60+
and msg = "Usage of a type " + e1.getType().toString() + " as an argument of a binary logical operation"
61+
)
62+
or exists
63+
(
64+
UnaryLogicalOperation ulop |
65+
ulop.getAnOperand() = e1 |
66+
e1.getType().(TypedefType).hasName("HRESULT")
67+
and msg = "Usage of a type " + e1.getType().toString() + " as an argument of a unary logical operation"
68+
)
69+
and not isHresultBooleanConverted(e1)
70+
)
71+
select e1, msg

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

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// semmle-extractor-options: --microsoft
2+
// winnt.h
3+
typedef long HRESULT;
4+
#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)
5+
#define FAILED(hr) (((HRESULT)(hr)) < 0)
6+
7+
typedef _Bool bool;
8+
#define FALSE 0
9+
10+
// minwindef.h
11+
typedef int BOOL;
12+
#ifndef FALSE
13+
#define FALSE 0
14+
#endif
15+
#ifndef TRUE
16+
#define TRUE 1
17+
#endif
18+
19+
// winerror.h
20+
#define S_OK ((HRESULT)0L)
21+
#define S_FALSE ((HRESULT)1L)
22+
#define _HRESULT_TYPEDEF_(_sc) ((HRESULT)_sc)
23+
#define E_UNEXPECTED _HRESULT_TYPEDEF_(0x8000FFFFL)
24+
25+
HRESULT HresultFunction()
26+
{
27+
return S_OK;
28+
}
29+
30+
BOOL BoolFunction()
31+
{
32+
return FALSE;
33+
}
34+
35+
bool BoolFunction2()
36+
{
37+
return FALSE;
38+
}
39+
40+
HRESULT IncorrectHresultFunction()
41+
{
42+
return BoolFunction(); // BUG
43+
}
44+
45+
HRESULT IncorrectHresultFunction2()
46+
{
47+
return BoolFunction2(); // BUG
48+
}
49+
50+
void IncorrectTypeConversionTest() {
51+
52+
HRESULT hr = HresultFunction();
53+
if ((BOOL)hr) // BUG
54+
{
55+
// ...
56+
}
57+
if ((bool)hr) // BUG
58+
{
59+
// ...
60+
}
61+
if (SUCCEEDED(hr)) // Correct Usage
62+
{
63+
// ...
64+
}
65+
66+
if (SUCCEEDED(BoolFunction())) // BUG
67+
{
68+
// ...
69+
}
70+
if (SUCCEEDED(BoolFunction2())) // BUG
71+
{
72+
// ...
73+
}
74+
if (BoolFunction()) // Correct Usage
75+
{
76+
// ...
77+
}
78+
BOOL b = IncorrectHresultFunction(); // BUG
79+
bool b2 = IncorrectHresultFunction(); // BUG
80+
81+
hr = E_UNEXPECTED;
82+
if (!hr) // BUG
83+
{
84+
// ...
85+
}
86+
if (!FAILED(hr)) // Correct Usage
87+
{
88+
// ...
89+
}
90+
91+
hr = S_FALSE;
92+
if (hr) // BUG
93+
{
94+
// ...
95+
}
96+
if (SUCCEEDED(hr)) // Correct Usage
97+
{
98+
// ...
99+
}
100+
}

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

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// semmle-extractor-options: --microsoft
2+
// winnt.h
3+
typedef long HRESULT;
4+
#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)
5+
#define FAILED(hr) (((HRESULT)(hr)) < 0)
6+
7+
// minwindef.h
8+
typedef int BOOL;
9+
#ifndef FALSE
10+
#define FALSE 0
11+
#endif
12+
#ifndef TRUE
13+
#define TRUE 1
14+
#endif
15+
16+
// winerror.h
17+
#define S_OK ((HRESULT)0L)
18+
#define S_FALSE ((HRESULT)1L)
19+
#define _HRESULT_TYPEDEF_(_sc) ((HRESULT)_sc)
20+
#define E_UNEXPECTED _HRESULT_TYPEDEF_(0x8000FFFFL)
21+
22+
HRESULT HresultFunction()
23+
{
24+
return S_OK;
25+
}
26+
27+
BOOL BoolFunction()
28+
{
29+
return FALSE;
30+
}
31+
32+
bool BoolFunction2()
33+
{
34+
return FALSE;
35+
}
36+
37+
HRESULT IncorrectHresultFunction()
38+
{
39+
return BoolFunction(); // BUG
40+
}
41+
42+
HRESULT IncorrectHresultFunction2()
43+
{
44+
return BoolFunction2(); // BUG
45+
}
46+
47+
void IncorrectTypeConversionTest() {
48+
49+
HRESULT hr = HresultFunction();
50+
if ((BOOL)hr) // BUG
51+
{
52+
// ...
53+
}
54+
if ((bool)hr) // BUG
55+
{
56+
// ...
57+
}
58+
if (SUCCEEDED(hr)) // Correct Usage
59+
{
60+
// ...
61+
}
62+
63+
if (SUCCEEDED(BoolFunction())) // BUG
64+
{
65+
// ...
66+
}
67+
if (SUCCEEDED(BoolFunction2())) // BUG
68+
{
69+
// ...
70+
}
71+
if (BoolFunction()) // Correct Usage
72+
{
73+
// ...
74+
}
75+
BOOL b = IncorrectHresultFunction(); // BUG
76+
bool b2 = IncorrectHresultFunction(); // BUG
77+
78+
hr = E_UNEXPECTED;
79+
if (!hr) // BUG
80+
{
81+
// ...
82+
}
83+
if (!FAILED(hr)) // Correct Usage
84+
{
85+
// ...
86+
}
87+
88+
hr = S_FALSE;
89+
if (hr) // BUG
90+
{
91+
// ...
92+
}
93+
if (SUCCEEDED(hr)) // Correct Usage
94+
{
95+
// ...
96+
}
97+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
| HResultBooleanConversion.c:42:12:42:23 | call to BoolFunction | Implicit conversion from BOOL to HRESULT |
2+
| HResultBooleanConversion.c:47:12:47:24 | call to BoolFunction2 | Implicit conversion from bool to HRESULT |
3+
| HResultBooleanConversion.c:53:15:53:16 | hr | Explicit conversion from HRESULT to BOOL |
4+
| HResultBooleanConversion.c:57:15:57:16 | hr | Explicit conversion from HRESULT to bool |
5+
| HResultBooleanConversion.c:66:9:66:33 | (...) | Explicit conversion from BOOL to HRESULT |
6+
| HResultBooleanConversion.c:70:9:70:34 | (...) | Explicit conversion from bool to HRESULT |
7+
| HResultBooleanConversion.c:78:14:78:37 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to BOOL |
8+
| HResultBooleanConversion.c:79:15:79:38 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to bool |
9+
| HResultBooleanConversion.c:82:10:82:11 | hr | Usage of a type HRESULT as an argument of a unary logical operation |
10+
| HResultBooleanConversion.c:92:9:92:10 | hr | Direct usage of a type HRESULT as a conditional expression |
11+
| HResultBooleanConversion.cpp:39:12:39:23 | call to BoolFunction | Implicit conversion from BOOL to HRESULT |
12+
| HResultBooleanConversion.cpp:44:12:44:24 | call to BoolFunction2 | Implicit conversion from bool to HRESULT |
13+
| HResultBooleanConversion.cpp:50:15:50:16 | hr | Explicit conversion from HRESULT to BOOL |
14+
| HResultBooleanConversion.cpp:54:15:54:16 | hr | Explicit conversion from HRESULT to bool |
15+
| HResultBooleanConversion.cpp:63:9:63:33 | (...) | Explicit conversion from BOOL to HRESULT |
16+
| HResultBooleanConversion.cpp:67:9:67:34 | (...) | Explicit conversion from bool to HRESULT |
17+
| HResultBooleanConversion.cpp:75:14:75:37 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to BOOL |
18+
| HResultBooleanConversion.cpp:76:15:76:38 | call to IncorrectHresultFunction | Implicit conversion from HRESULT to bool |
19+
| HResultBooleanConversion.cpp:79:10:79:11 | hr | Implicit conversion from HRESULT to bool |
20+
| HResultBooleanConversion.cpp:89:9:89:10 | hr | Implicit conversion from HRESULT to bool |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE/CWE-253/HResultBooleanConversion.ql

0 commit comments

Comments
 (0)