From 6d0623404875da442378f87747813f54d6f5393c Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 23 Sep 2022 11:41:16 +0100 Subject: [PATCH 1/4] C++: Add testcase demonstrating missing result for 'cpp/invalid-pointer-deref' query. --- .../InvalidPointerDeref.expected | 41 +++++++++++++++++++ .../CWE/CWE-193/pointer-deref/test.cpp | 25 ++++++++++- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected index 494713b124b..cd5f29018b3 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected @@ -609,6 +609,46 @@ 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: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 | @@ -625,3 +665,4 @@ 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: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 | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp index 809c348c0b0..af62b14d773 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp @@ -188,4 +188,27 @@ void test11(unsigned size) { char *p = malloc(size); char *q = p + size - 1; deref_plus_one(q); -} \ No newline at end of file +} + +void test12(unsigned len, unsigned index) { + char* p = (char *)malloc(len); + char* end = p + len; + + if(p + index > end) { + return; + } + + p[index] = '\0'; // BAD [NOT DETECTED] +} + +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 +} From ac03242cfcd2c778eea7f703896b5280c368b990 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 23 Sep 2022 12:10:09 +0100 Subject: [PATCH 2/4] C++: Add an SSAVariable for pointer-arithmetic expressions in guards. --- .../cpp/semantic/SemanticExprSpecific.qll | 42 ++++++++++++++++--- .../semmle/code/cpp/semantic/SemanticSSA.qll | 2 +- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll b/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll index dc178f77547..25de37d096a 100644 --- a/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll +++ b/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll @@ -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; @@ -119,8 +120,17 @@ module SemanticExprConfig { int getBasicBlockUniqueId(BasicBlock block) { idOf(block.getFirstInstruction().getAst(), result) } newtype TSsaVariable = - TSsaInstruction(IR::Instruction instr) { instr.hasMemoryResult() } or - TSsaOperand(IR::Operand op) { op.isDefinitionInexact() } + TSsaInstruction(IR::Instruction instr) { + instr.hasMemoryResult() + } or + 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() } @@ -129,6 +139,8 @@ module SemanticExprConfig { IR::Instruction asInstruction() { none() } + IR::PointerArithmeticInstruction asPointerArithGuard() { none() } + IR::Operand asOperand() { none() } } @@ -144,6 +156,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; @@ -168,7 +192,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()) @@ -208,7 +236,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 ) @@ -227,7 +257,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 ) diff --git a/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticSSA.qll b/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticSSA.qll index 80e1d6c84a6..307f6e386b5 100644 --- a/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticSSA.qll +++ b/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticSSA.qll @@ -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) } From 494afdde961515b5970c5d8660a7de801604f59d Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 23 Sep 2022 12:11:33 +0100 Subject: [PATCH 3/4] C++: Accept test changes. --- .../InvalidPointerDeref.expected | 29 +++++++++++++++++++ .../CWE/CWE-193/pointer-deref/test.cpp | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected index cd5f29018b3..b42b69cb1cd 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected @@ -612,6 +612,34 @@ edges | 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 | ... + ... | @@ -665,4 +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 | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp index af62b14d773..a4cc8b3830c 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp @@ -198,7 +198,7 @@ void test12(unsigned len, unsigned index) { return; } - p[index] = '\0'; // BAD [NOT DETECTED] + p[index] = '\0'; // BAD } void test13(unsigned len, unsigned index) { From f3212fe01cc5b0c5c918b7de38d201fed64b7eaf Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 23 Sep 2022 13:00:22 +0100 Subject: [PATCH 4/4] C++: Autoformat. --- .../semmle/code/cpp/semantic/SemanticExprSpecific.qll | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll b/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll index 25de37d096a..f7e5abd25b7 100644 --- a/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll +++ b/cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll @@ -120,16 +120,15 @@ module SemanticExprConfig { int getBasicBlockUniqueId(BasicBlock block) { idOf(block.getFirstInstruction().getAst(), result) } newtype TSsaVariable = - TSsaInstruction(IR::Instruction instr) { - instr.hasMemoryResult() - } or + TSsaInstruction(IR::Instruction instr) { instr.hasMemoryResult() } or 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, _, _, _)) + g.comparesEq(_, use, _, _, _) + ) } class SsaVariable extends TSsaVariable { @@ -238,7 +237,7 @@ module SemanticExprConfig { exists(IR::Operand operand | operand.getDef() = v.asInstruction() or operand.getDef() = valueNumber(v.asPointerArithGuard()).getAnInstruction() - | + | not operand instanceof IR::PhiInputOperand and operand.getUse().getBlock() = block ) @@ -259,7 +258,7 @@ module SemanticExprConfig { exists(IR::PhiInputOperand operand | operand.getDef() = v.asInstruction() or operand.getDef() = valueNumber(v.asPointerArithGuard()).getAnInstruction() - | + | operand.getPredecessorBlock() = pred and operand.getUse().getBlock() = succ )