Skip to content

Commit 939b218

Browse files
bdrodesbdrodes
authored
Wchar fp fixes (#107)
* Adding tests and updated expected file with false positives to correct. (cherry picked from commit 26e5853) * Modifications to the query to address false positives. (cherry picked from commit cc24f1e) * Updating expected file, false positives now resolved. (cherry picked from commit 92c8d39) * Correct comment. (cherry picked from commit 338ab96) * Changing from hasIntermediateType to getABaseType. (cherry picked from commit c4737c7) * Switching to looking for explicit declaration of unsigned char, to avoid cases where unsigned char is the default char width for `char`. (cherry picked from commit 51e787b) * Altering ordering for exists statement to be clearer. (cherry picked from commit 31324fc) * Altering exists predicate ordering to be clearer. (cherry picked from commit c91f7f4) * Changing name of predicate to be clearer, and removing an unused parameter. (cherry picked from commit 318e75c) * Removing unnecessary bracket/singleton set literal. (cherry picked from commit 1625191) * Formatting. (cherry picked from commit c496503)
1 parent 60cda95 commit 939b218

3 files changed

Lines changed: 135 additions & 6 deletions

File tree

‎cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql‎

Lines changed: 71 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,85 @@
1313
*/
1414

1515
import cpp
16+
import semmle.code.cpp.controlflow.Guards
1617

1718
class WideCharPointerType extends PointerType {
1819
WideCharPointerType() { this.getBaseType() instanceof WideCharType }
1920
}
2021

22+
/**
23+
* Given type `t`, recurses through and returns all
24+
* intermediate base types, including `t`.
25+
*/
26+
Type getABaseType(Type t) {
27+
result = t
28+
or
29+
result = getABaseType(t.(DerivedType).getBaseType())
30+
or
31+
result = getABaseType(t.(TypedefType).getBaseType())
32+
}
33+
2134
/**
2235
* A type that may also be `CharPointerType`, but that are likely used as arbitrary buffers.
2336
*/
2437
class UnlikelyToBeAStringType extends Type {
2538
UnlikelyToBeAStringType() {
26-
this.(PointerType).getBaseType().(CharType).isUnsigned() or
27-
this.(PointerType).getBaseType().getName().toLowerCase().matches("%byte") or
28-
this.getName().toLowerCase().matches("%byte") or
29-
this.(PointerType).getBaseType().hasName("uint8_t")
39+
exists(Type targ | getABaseType(this) = targ |
40+
// NOTE: not using CharType isUnsigned, but rather look for any explicitly declared unsigned
41+
// char types. Assuming these are used for buffers, not strings.
42+
targ.(CharType).getName().toLowerCase().matches("unsigned%") or
43+
targ.getName().toLowerCase().matches(["uint8_t", "%byte%"])
44+
)
3045
}
3146
}
3247

48+
// Types that can be wide depending on the UNICODE macro
49+
// see https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types
50+
class UnicodeMacroDependentWidthType extends Type {
51+
UnicodeMacroDependentWidthType() {
52+
exists(Type targ | getABaseType(this) = targ |
53+
targ.getName() in [
54+
"LPCTSTR",
55+
"LPTSTR",
56+
"PCTSTR",
57+
"PTSTR",
58+
"TBYTE",
59+
"TCHAR"
60+
]
61+
)
62+
}
63+
}
64+
65+
class UnicodeMacro extends Macro {
66+
UnicodeMacro() { this.getName().toLowerCase().matches("%unicode%") }
67+
}
68+
69+
class UnicodeMacroInvocation extends MacroInvocation {
70+
UnicodeMacroInvocation() { this.getMacro() instanceof UnicodeMacro }
71+
}
72+
73+
/**
74+
* Holds when a expression whose type is UnicodeMacroDependentWidthType and
75+
* is observed to be guarded by a check involving a bitwise-and operation
76+
* with a UnicodeMacroInvocation.
77+
* Such expressions are assumed to be checked dynamically, i.e.,
78+
* the flag would indicate if UNICODE typing is set correctly to allow
79+
* or disallow a widening cast.
80+
*/
81+
predicate isLikelyDynamicallyChecked(Expr e) {
82+
e.getType() instanceof UnicodeMacroDependentWidthType and
83+
exists(GuardCondition gc, BitwiseAndExpr bai, UnicodeMacroInvocation umi |
84+
bai.getAnOperand() = umi.getExpr()
85+
|
86+
// bai == 0 is false when reaching `e.getBasicBlock()`.
87+
// That is, bai != 0 when reaching `e.getBasicBlock()`.
88+
gc.ensuresEq(bai, 0, e.getBasicBlock(), false)
89+
or
90+
// bai == k and k != 0 is true when reaching `e.getBasicBlock()`.
91+
gc.ensuresEq(bai, any(int k | k != 0), e.getBasicBlock(), true)
92+
)
93+
}
94+
3395
from Expr e1, Cast e2
3496
where
3597
e2 = e1.getConversion() and
@@ -42,7 +104,11 @@ where
42104
not e1.getType() instanceof UnlikelyToBeAStringType and
43105
// Avoid castings from 'new' expressions as typically these will be safe
44106
// Example: `__Type* ret = reinterpret_cast<__Type*>(New(m_pmo) char[num * sizeof(__Type)]);`
45-
not exists(NewOrNewArrayExpr newExpr | newExpr.getAChild*() = e1)
107+
not exists(NewOrNewArrayExpr newExpr | newExpr.getAChild*() = e1) and
108+
// Avoid cases where the cast is guarded by a check to determine if
109+
// unicode encoding is enabled in such a way to disallow the dangerous cast
110+
// at runtime.
111+
not isLikelyDynamicallyChecked(e1)
46112
select e1,
47113
"Conversion from " + e1.getType().toString() + " to " + e2.getType().toString() +
48114
". Use of invalid string can lead to undefined behavior."

‎cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,59 @@ void NonStringFalsePositiveTest2(unsigned char* buffer)
5353
{
5454
wchar_t *lpWchar = NULL;
5555
lpWchar = (LPWSTR)buffer; // Possible False Positive
56-
}
56+
}
57+
58+
typedef unsigned char BYTE;
59+
using FOO = BYTE*;
60+
61+
void NonStringFalsePositiveTest3(FOO buffer)
62+
{
63+
wchar_t *lpWchar = NULL;
64+
lpWchar = (LPWSTR)buffer; // GOOD
65+
}
66+
67+
#define UNICODE 0x8
68+
69+
// assume EMPTY_MACRO is tied to if UNICODE is enabled
70+
#ifdef EMPTY_MACRO
71+
typedef WCHAR* LPTSTR;
72+
#else
73+
typedef char* LPTSTR;
74+
#endif
75+
76+
void CheckedConversionFalsePositiveTest3(unsigned short flags, LPTSTR buffer)
77+
{
78+
wchar_t *lpWchar = NULL;
79+
if(flags & UNICODE)
80+
lpWchar = (LPWSTR)buffer; // GOOD
81+
else
82+
lpWchar = (LPWSTR)buffer; // BUG
83+
84+
if((flags & UNICODE) == 0x8)
85+
lpWchar = (LPWSTR)buffer; // GOOD
86+
else
87+
lpWchar = (LPWSTR)buffer; // BUG
88+
89+
if((flags & UNICODE) != 0x8)
90+
lpWchar = (LPWSTR)buffer; // BUG
91+
else
92+
lpWchar = (LPWSTR)buffer; // GOOD
93+
94+
// Bad operator precedence
95+
if(flags & UNICODE == 0x8)
96+
lpWchar = (LPWSTR)buffer; // BUG
97+
else
98+
lpWchar = (LPWSTR)buffer; // BUG
99+
100+
if((flags & UNICODE) != 0)
101+
lpWchar = (LPWSTR)buffer; // GOOD
102+
else
103+
lpWchar = (LPWSTR)buffer; // BUG
104+
105+
if((flags & UNICODE) == 0)
106+
lpWchar = (LPWSTR)buffer; // BUG
107+
else
108+
lpWchar = (LPWSTR)buffer; // GOOD
109+
110+
lpWchar = (LPWSTR)buffer; // BUG
111+
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,11 @@
33
| WcharCharConversion.cpp:24:22:24:27 | lpChar | Conversion from char * to wchar_t *. Use of invalid string can lead to undefined behavior. |
44
| WcharCharConversion.cpp:26:23:26:28 | lpChar | Conversion from char * to LPCWSTR. Use of invalid string can lead to undefined behavior. |
55
| WcharCharConversion.cpp:27:17:27:22 | lpChar | Conversion from char * to LPWSTR. Use of invalid string can lead to undefined behavior. |
6+
| WcharCharConversion.cpp:82:21:82:26 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
7+
| WcharCharConversion.cpp:87:21:87:26 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
8+
| WcharCharConversion.cpp:90:21:90:26 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
9+
| WcharCharConversion.cpp:96:21:96:26 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
10+
| WcharCharConversion.cpp:98:21:98:26 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
11+
| WcharCharConversion.cpp:103:21:103:26 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
12+
| WcharCharConversion.cpp:106:21:106:26 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |
13+
| WcharCharConversion.cpp:110:20:110:25 | buffer | Conversion from LPTSTR to LPWSTR. Use of invalid string can lead to undefined behavior. |

0 commit comments

Comments
 (0)