Skip to content

C++: Fix missing bounds in range analysis #10555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 23, 2022
Merged

C++: Fix missing bounds in range analysis #10555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d06234
C++: Add testcase demonstrating missing result for 'cpp/invalid-point…
MathiasVP Sep 23, 2022
ac03242
C++: Add an SSAVariable for pointer-arithmetic expressions in guards.
MathiasVP Sep 23, 2022
494afdd
C++: Accept test changes.
MathiasVP Sep 23, 2022
f3212fe
C++: Autoformat.
MathiasVP Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ private import semmle.code.cpp.ir.IR as IR
private import Semantic
private import experimental.semmle.code.cpp.rangeanalysis.Bound as IRBound
private import semmle.code.cpp.controlflow.IRGuards as IRGuards
private import semmle.code.cpp.ir.ValueNumbering

module SemanticExprConfig {
class Location = Cpp::Location;
Expand Down Expand Up @@ -120,7 +121,15 @@ module SemanticExprConfig {

newtype TSsaVariable =
TSsaInstruction(IR::Instruction instr) { instr.hasMemoryResult() } or
TSsaOperand(IR::Operand op) { op.isDefinitionInexact() }
TSsaOperand(IR::Operand op) { op.isDefinitionInexact() } or
TSsaPointerArithmeticGuard(IR::PointerArithmeticInstruction instr) {
exists(Guard g, IR::Operand use | use = instr.getAUse() |
g.comparesLt(use, _, _, _, _) or
g.comparesLt(_, use, _, _, _) or
g.comparesEq(use, _, _, _, _) or
g.comparesEq(_, use, _, _, _)
)
}

class SsaVariable extends TSsaVariable {
string toString() { none() }
Expand All @@ -129,6 +138,8 @@ module SemanticExprConfig {

IR::Instruction asInstruction() { none() }

IR::PointerArithmeticInstruction asPointerArithGuard() { none() }

IR::Operand asOperand() { none() }
}

Expand All @@ -144,6 +155,18 @@ module SemanticExprConfig {
final override IR::Instruction asInstruction() { result = instr }
}

class SsaPointerArithmeticGuard extends SsaVariable, TSsaPointerArithmeticGuard {
IR::PointerArithmeticInstruction instr;

SsaPointerArithmeticGuard() { this = TSsaPointerArithmeticGuard(instr) }

final override string toString() { result = instr.toString() }

final override Location getLocation() { result = instr.getLocation() }

final override IR::PointerArithmeticInstruction asPointerArithGuard() { result = instr }
}

class SsaOperand extends SsaVariable, TSsaOperand {
IR::Operand op;

Expand All @@ -168,7 +191,11 @@ module SemanticExprConfig {
)
}

Expr getAUse(SsaVariable v) { result.(IR::LoadInstruction).getSourceValue() = v.asInstruction() }
Expr getAUse(SsaVariable v) {
result.(IR::LoadInstruction).getSourceValue() = v.asInstruction()
or
result = valueNumber(v.asPointerArithGuard()).getAnInstruction()
}

SemType getSsaVariableType(SsaVariable v) {
result = getSemanticType(v.asInstruction().getResultIRType())
Expand Down Expand Up @@ -208,7 +235,9 @@ module SemanticExprConfig {

final override predicate hasRead(SsaVariable v) {
exists(IR::Operand operand |
operand.getDef() = v.asInstruction() and
operand.getDef() = v.asInstruction() or
operand.getDef() = valueNumber(v.asPointerArithGuard()).getAnInstruction()
|
not operand instanceof IR::PhiInputOperand and
operand.getUse().getBlock() = block
)
Expand All @@ -227,7 +256,9 @@ module SemanticExprConfig {

final override predicate hasRead(SsaVariable v) {
exists(IR::PhiInputOperand operand |
operand.getDef() = v.asInstruction() and
operand.getDef() = v.asInstruction() or
operand.getDef() = valueNumber(v.asPointerArithGuard()).getAnInstruction()
|
operand.getPredecessorBlock() = pred and
operand.getUse().getBlock() = succ
)
Expand Down
2 changes: 1 addition & 1 deletion cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticSSA.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class SemSsaVariable instanceof Specific::SsaVariable {

final Specific::Location getLocation() { result = super.getLocation() }

final SemLoadExpr getAUse() { result = Specific::getAUse(this) }
final SemExpr getAUse() { result = Specific::getAUse(this) }

final SemType getType() { result = Specific::getSsaVariableType(this) }

Expand Down
70 changes: 70 additions & 0 deletions .../experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,74 @@ edges
| test.cpp:180:19:180:28 | call to mk_array_p indirection [begin] | test.cpp:165:29:165:31 | arr indirection [begin] |
| test.cpp:180:19:180:28 | call to mk_array_p indirection [end] | test.cpp:165:29:165:31 | arr indirection [end] |
| test.cpp:188:15:188:20 | call to malloc | test.cpp:189:15:189:15 | Load |
| test.cpp:194:23:194:28 | call to malloc | test.cpp:195:17:195:17 | Load |
| test.cpp:194:23:194:28 | call to malloc | test.cpp:197:8:197:8 | Load |
| test.cpp:194:23:194:28 | call to malloc | test.cpp:201:5:201:5 | Load |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
| test.cpp:195:17:195:23 | ... + ... | test.cpp:195:17:195:23 | Store |
| test.cpp:195:17:195:23 | ... + ... | test.cpp:195:17:195:23 | Store |
| test.cpp:195:17:195:23 | ... + ... | test.cpp:197:20:197:22 | Load |
| test.cpp:195:17:195:23 | ... + ... | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:195:17:195:23 | ... + ... | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:195:17:195:23 | Store | test.cpp:197:20:197:22 | Load |
| test.cpp:195:17:195:23 | Store | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:195:17:195:23 | Store | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:197:20:197:22 | Load | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:197:20:197:22 | Load | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:201:5:201:12 | access to array | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:201:5:201:12 | access to array | test.cpp:201:5:201:19 | Store: ... = ... |
| test.cpp:205:23:205:28 | call to malloc | test.cpp:206:17:206:17 | Load |
| test.cpp:205:23:205:28 | call to malloc | test.cpp:208:15:208:15 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
| test.cpp:206:17:206:23 | ... + ... | test.cpp:206:17:206:23 | Store |
| test.cpp:206:17:206:23 | ... + ... | test.cpp:206:17:206:23 | Store |
| test.cpp:206:17:206:23 | ... + ... | test.cpp:209:12:209:14 | Load |
| test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:206:17:206:23 | Store | test.cpp:209:12:209:14 | Load |
| test.cpp:206:17:206:23 | Store | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:206:17:206:23 | Store | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:209:12:209:14 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:209:12:209:14 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:213:5:213:6 | * ... | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:213:5:213:6 | * ... | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:213:6:213:6 | Load | test.cpp:213:5:213:6 | * ... |
| test.cpp:213:6:213:6 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
| test.cpp:213:6:213:6 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
#select
| test.cpp:6:14:6:15 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:6:14:6:15 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
| test.cpp:8:14:8:21 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:8:14:8:21 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
Expand All @@ -625,3 +693,5 @@ edges
| test.cpp:110:9:110:14 | Store: ... = ... | test.cpp:82:17:82:22 | call to malloc | test.cpp:110:9:110:14 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:82:17:82:22 | call to malloc | call to malloc | test.cpp:83:27:83:30 | size | size |
| test.cpp:157:9:157:14 | Store: ... = ... | test.cpp:143:18:143:23 | call to malloc | test.cpp:157:9:157:14 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:143:18:143:23 | call to malloc | call to malloc | test.cpp:144:29:144:32 | size | size |
| test.cpp:171:9:171:14 | Store: ... = ... | test.cpp:143:18:143:23 | call to malloc | test.cpp:171:9:171:14 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:143:18:143:23 | call to malloc | call to malloc | test.cpp:144:29:144:32 | size | size |
| test.cpp:201:5:201:19 | Store: ... = ... | test.cpp:194:23:194:28 | call to malloc | test.cpp:201:5:201:19 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:194:23:194:28 | call to malloc | call to malloc | test.cpp:195:21:195:23 | len | len |
| test.cpp:213:5:213:13 | Store: ... = ... | test.cpp:205:23:205:28 | call to malloc | test.cpp:213:5:213:13 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:205:23:205:28 | call to malloc | call to malloc | test.cpp:206:21:206:23 | len | len |
25 changes: 24 additions & 1 deletion cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,4 +188,27 @@ void test11(unsigned size) {
char *p = malloc(size);
char *q = p + size - 1;
deref_plus_one(q);
}
}

void test12(unsigned len, unsigned index) {
char* p = (char *)malloc(len);
char* end = p + len;

if(p + index > end) {
return;
}

p[index] = '\0'; // BAD
}

void test13(unsigned len, unsigned index) {
char* p = (char *)malloc(len);
char* end = p + len;

char* q = p + index;
if(q > end) {
return;
}

*q = '\0'; // BAD
}