Skip to content

Commit 24d7446

Browse files
geoffw0geoffw0
committed
C++: Basic model of '&' and '>>' in SimpleRangeAnalysis.
1 parent 2acbdec commit 24d7446

File tree

3 files changed

+102
-26
lines changed

3 files changed

+102
-26
lines changed

‎cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

Lines changed: 97 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -118,36 +118,74 @@ private string getValue(Expr e) {
118118
)
119119
}
120120

121+
/**
122+
* A bitwise `&` expression in which both operands are unsigned, or are effectively
123+
* unsigned due to being a non-negative constant.
124+
*/
125+
private class UnsignedBitwiseAndExpr extends BitwiseAndExpr {
126+
UnsignedBitwiseAndExpr() {
127+
(
128+
getLeftOperand().getType().getUnderlyingType().(IntegralType).isUnsigned() or
129+
getLeftOperand().getValue().toInt() >= 0
130+
) and
131+
(
132+
getRightOperand().getType().getUnderlyingType().(IntegralType).isUnsigned() or
133+
getRightOperand().getValue().toInt() >= 0
134+
)
135+
}
136+
}
137+
121138
/** Set of expressions which we know how to analyze. */
122139
private predicate analyzableExpr(Expr e) {
123140
// The type of the expression must be arithmetic. We reuse the logic in
124141
// `exprMinVal` to check this.
125142
exists(exprMinVal(e)) and
126143
(
127-
exists(getValue(e).toFloat()) or
128-
e instanceof UnaryPlusExpr or
129-
e instanceof UnaryMinusExpr or
130-
e instanceof MinExpr or
131-
e instanceof MaxExpr or
132-
e instanceof ConditionalExpr or
133-
e instanceof AddExpr or
134-
e instanceof SubExpr or
135-
e instanceof AssignExpr or
136-
e instanceof AssignAddExpr or
137-
e instanceof AssignSubExpr or
138-
e instanceof CrementOperation or
139-
e instanceof RemExpr or
140-
e instanceof CommaExpr or
141-
e instanceof StmtExpr or
144+
exists(getValue(e).toFloat())
145+
or
146+
e instanceof UnaryPlusExpr
147+
or
148+
e instanceof UnaryMinusExpr
149+
or
150+
e instanceof MinExpr
151+
or
152+
e instanceof MaxExpr
153+
or
154+
e instanceof ConditionalExpr
155+
or
156+
e instanceof AddExpr
157+
or
158+
e instanceof SubExpr
159+
or
160+
e instanceof AssignExpr
161+
or
162+
e instanceof AssignAddExpr
163+
or
164+
e instanceof AssignSubExpr
165+
or
166+
e instanceof CrementOperation
167+
or
168+
e instanceof RemExpr
169+
or
170+
e instanceof CommaExpr
171+
or
172+
e instanceof StmtExpr
173+
or
142174
// A conversion is analyzable, provided that its child has an arithmetic
143175
// type. (Sometimes the child is a reference type, and so does not get
144176
// any bounds.) Rather than checking whether the type of the child is
145177
// arithmetic, we reuse the logic that is already encoded in
146178
// `exprMinVal`.
147-
exists(exprMinVal(e.(Conversion).getExpr())) or
179+
exists(exprMinVal(e.(Conversion).getExpr()))
180+
or
148181
// Also allow variable accesses, provided that they have SSA
149182
// information.
150183
exists(RangeSsaDefinition def, StackVariable v | e = def.getAUse(v))
184+
or
185+
e instanceof UnsignedBitwiseAndExpr
186+
or
187+
// `>>` by a constant
188+
exists(e.(RShiftExpr).getRightOperand().getValue())
151189
)
152190
}
153191

@@ -245,6 +283,19 @@ private predicate exprDependsOnDef(Expr e, RangeSsaDefinition srcDef, StackVaria
245283
or
246284
exists(Conversion convExpr | e = convExpr | exprDependsOnDef(convExpr.getExpr(), srcDef, srcVar))
247285
or
286+
// unsigned `&`
287+
exists(UnsignedBitwiseAndExpr andExpr |
288+
andExpr = e and
289+
exprDependsOnDef(andExpr.getAnOperand(), srcDef, srcVar)
290+
)
291+
or
292+
// `>>` by a constant
293+
exists(RShiftExpr rs |
294+
rs = e and
295+
exists(rs.getRightOperand().getValue()) and
296+
exprDependsOnDef(rs.getLeftOperand(), srcDef, srcVar)
297+
)
298+
or
248299
e = srcDef.getAUse(srcVar)
249300
}
250301

@@ -641,6 +692,20 @@ private float getLowerBoundsImpl(Expr expr) {
641692
exists(RangeSsaDefinition def, StackVariable v | expr = def.getAUse(v) |
642693
result = getDefLowerBounds(def, v)
643694
)
695+
or
696+
// unsigned `&` (tighter bounds may exist)
697+
exists(UnsignedBitwiseAndExpr andExpr |
698+
andExpr = expr and
699+
result = 0.0
700+
)
701+
or
702+
// `>>` by a constant
703+
exists(RShiftExpr rsExpr, float left, int right |
704+
rsExpr = expr and
705+
left = getFullyConvertedLowerBounds(rsExpr.getLeftOperand()) and
706+
right = rsExpr.getRightOperand().getValue().toInt() and
707+
result = left / 2.pow(right)
708+
)
644709
}
645710

646711
/** Only to be called by `getTruncatedUpperBounds`. */
@@ -794,6 +859,22 @@ private float getUpperBoundsImpl(Expr expr) {
794859
exists(RangeSsaDefinition def, StackVariable v | expr = def.getAUse(v) |
795860
result = getDefUpperBounds(def, v)
796861
)
862+
or
863+
// unsigned `&` (tighter bounds may exist)
864+
exists(UnsignedBitwiseAndExpr andExpr, float left, float right |
865+
andExpr = expr and
866+
left = getFullyConvertedUpperBounds(andExpr.getLeftOperand()) and
867+
right = getFullyConvertedUpperBounds(andExpr.getRightOperand()) and
868+
result = left.minimum(right)
869+
)
870+
or
871+
// `>>` by a constant
872+
exists(RShiftExpr rsExpr, float left, int right |
873+
rsExpr = expr and
874+
left = getFullyConvertedUpperBounds(rsExpr.getLeftOperand()) and
875+
right = rsExpr.getRightOperand().getValue().toInt() and
876+
result = left / 2.pow(right)
877+
)
797878
}
798879

799880
/**

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/ComparisonWithWiderType.expected

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,8 @@
66
| test.c:91:14:91:23 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type int. | test.c:83:16:83:16 | c | c | test.c:91:18:91:23 | 65280 | 65280 |
77
| test.c:93:14:93:25 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type int. | test.c:83:16:83:16 | c | c | test.c:93:18:93:25 | 16711680 | 16711680 |
88
| test.c:95:14:95:27 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:95:18:95:27 | 4278190080 | 4278190080 |
9-
| test.c:97:14:97:27 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:97:19:97:26 | ... & ... | ... & ... |
109
| test.c:99:14:99:29 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:99:19:99:28 | ... & ... | ... & ... |
1110
| test.c:101:14:101:31 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:101:19:101:30 | ... & ... | ... & ... |
1211
| test.c:103:14:103:33 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:103:19:103:32 | ... & ... | ... & ... |
1312
| test.c:105:14:105:25 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:105:19:105:24 | ... >> ... | ... >> ... |
1413
| test.c:107:14:107:26 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:107:19:107:25 | ... >> ... | ... >> ... |
15-
| test.c:109:14:109:26 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:109:19:109:25 | ... >> ... | ... >> ... |
16-
| test.c:111:14:111:36 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:111:19:111:35 | ... >> ... | ... >> ... |
17-
| test.c:113:14:113:39 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:113:19:113:38 | ... >> ... | ... >> ... |
18-
| test.c:115:14:115:41 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:115:19:115:40 | ... >> ... | ... >> ... |

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/test.c

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ void test12() {
9494
x = get_a_uint();
9595
for (c = 0; c < 0xFF000000; c++) {} // BAD
9696
x = get_a_uint();
97-
for (c = 0; c < (x & 0xFF); c++) {} // GOOD [FALSE POSITIVE]
97+
for (c = 0; c < (x & 0xFF); c++) {} // GOOD
9898
x = get_a_uint();
9999
for (c = 0; c < (x & 0xFF00); c++) {} // BAD
100100
x = get_a_uint();
@@ -106,11 +106,11 @@ void test12() {
106106
x = get_a_uint();
107107
for (c = 0; c < (x >> 16); c++) {} // BAD
108108
x = get_a_uint();
109-
for (c = 0; c < (x >> 24); c++) {} // GOOD (assuming 32-bit ints) [FALSE POSITIVE]
109+
for (c = 0; c < (x >> 24); c++) {} // GOOD (assuming 32-bit ints)
110110
x = get_a_uint();
111-
for (c = 0; c < ((x & 0xFF00) >> 8); c++) {} // GOOD [FALSE POSITIVE]
111+
for (c = 0; c < ((x & 0xFF00) >> 8); c++) {} // GOOD
112112
x = get_a_uint();
113-
for (c = 0; c < ((x & 0xFF0000) >> 16); c++) {} // GOOD [FALSE POSITIVE]
113+
for (c = 0; c < ((x & 0xFF0000) >> 16); c++) {} // GOOD
114114
x = get_a_uint();
115-
for (c = 0; c < ((x & 0xFF000000) >> 24); c++) {} // GOOD [FALSE POSITIVE]
115+
for (c = 0; c < ((x & 0xFF000000) >> 24); c++) {} // GOOD
116116
}

0 commit comments

Comments
 (0)