C++: Add a query for detecting uses of expired stack pointers that escaped through global variables.

This commit is contained in:
Mathias Vorreiter Pedersen
2022-02-22 18:29:09 +00:00
parent 31d214d5ee
commit ea35f56212
9 changed files with 534 additions and 0 deletions

View File

@@ -31,6 +31,7 @@
+ semmlecode-cpp-queries/Critical/NewArrayDeleteMismatch.ql: /Correctness/Common Errors
+ semmlecode-cpp-queries/Critical/NewDeleteArrayMismatch.ql: /Correctness/Common Errors
+ semmlecode-cpp-queries/Critical/NewFreeMismatch.ql: /Correctness/Common Errors
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql: /Correctness/Common Errors
# Use of Libraries
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/SuspiciousCallToMemset.ql: /Correctness/Use of Libraries
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/SuspiciousSizeof.ql: /Correctness/Use of Libraries

View File

@@ -34,6 +34,7 @@
+ semmlecode-cpp-queries/Critical/NewArrayDeleteMismatch.ql: /Correctness/Common Errors
+ semmlecode-cpp-queries/Critical/NewDeleteArrayMismatch.ql: /Correctness/Common Errors
+ semmlecode-cpp-queries/Critical/NewFreeMismatch.ql: /Correctness/Common Errors
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql: /Correctness/Common Errors
# Exceptions
+ semmlecode-cpp-queries/Best Practices/Exceptions/AccidentalRethrow.ql: /Correctness/Exceptions
+ semmlecode-cpp-queries/Best Practices/Exceptions/CatchingByValue.ql: /Correctness/Exceptions

View File

@@ -0,0 +1,25 @@
static const int* xptr;
void localAddressEscapes() {
int x = 0;
xptr = &x;
}
void example1() {
localAddressEscapes();
const int* x = xptr; // BAD: This pointer points to expired stack allocated memory.
}
void localAddressDoesNotEscape() {
int x = 0;
xptr = &x;
// ...
// use `xptr`
// ...
xptr = nullptr;
}
void example2() {
localAddressEscapes();
const int* x = xptr; // GOOD: This pointer does not point to expired memory.
}

View File

@@ -0,0 +1,49 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
This rule finds uses of pointers that likely point to local variables in
expired stack frames. Such pointers to local variables is only valid
until the function returns, after which it becomes a dangling pointer.
</p>
</overview>
<recommendation>
<ol>
<li>
If it is necessary to take the address of a local variable, then make
sure that the address is only stored in memory that does not outlive
the local variable. For example, it is safe to store the address in
another local variable. Similarly, it is also safe to pass the address
of a local variable to another function provided that the other
function only uses it locally and does not store it in non-local
memory.
</li>
<li>
If it is necessary to store an address which will outlive the
current function scope, then it should be allocated on the heap. Care
should be taken to make sure that the memory is deallocated when it is
no longer needed, particularly when using low-level memory management
routines such as <tt>malloc</tt>/<tt>free</tt> or
<tt>new</tt>/<tt>delete</tt>. Modern C++ applications often use smart
pointers, such as <tt>std::shared_ptr</tt>, to reduce the chance of
a memory leak.
</li>
</ol>
</recommendation>
<example>
<sample src="UsingExpiredStackAddress.cpp" />
</example>
<references>
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Dangling_pointer">Dangling pointer</a>.</li>
</references>
</qhelp>

View File

@@ -0,0 +1,244 @@
/**
* @name Use of expired stack-address.
* @description Accessing the stack-allocated memory of a function
* after it has returned can lead to memory corruption.
* @kind problem
* @problem.severity warning
* @security-severity 9.3
* @precision high
* @id cpp/using-expired-stack-address
* @tags reliability
* security
* external/cwe/cwe-825
*/
import cpp
import semmle.code.cpp.ir.ValueNumbering
import semmle.code.cpp.ir.IR
/**
* Holds if `source` is the base address of an address computation whose
* result is stored in `address`.
*/
predicate stackPointerFlowsToUse(Instruction address, VariableAddressInstruction source) {
exists(VariableAddressInstruction var |
var = address and
var = source and
var.getASTVariable() instanceof StackVariable and
// Pointer-to-member types aren't properly handled in the dbscheme.
not var.getResultType() instanceof PointerToMemberType and
// Rule out FPs caused by extraction errors.
not any(ErrorExpr e).getEnclosingFunction() = var.getEnclosingFunction()
)
or
stackPointerFlowsToUse(address.(CopyInstruction).getSourceValue(), source)
or
stackPointerFlowsToUse(address.(ConvertInstruction).getUnary(), source)
or
stackPointerFlowsToUse(address.(CheckedConvertOrNullInstruction).getUnary(), source)
or
stackPointerFlowsToUse(address.(InheritanceConversionInstruction).getUnary(), source)
or
stackPointerFlowsToUse(address.(FieldAddressInstruction).getObjectAddress(), source)
or
stackPointerFlowsToUse(address.(PointerOffsetInstruction).getLeft(), source)
}
/**
* A HashCons-like table for comparing addresses that are
* computed relative to some global variable.
*/
newtype TGlobalAddress =
TGlobalVariable(GlobalOrNamespaceVariable v) {
// Pointer-to-member types aren't properly handled in the dbscheme.
not v.getUnspecifiedType() instanceof PointerToMemberType
} or
TLoad(TGlobalAddress address) {
address = globalValueNumber(any(LoadInstruction load).getSourceAddress())
} or
TConversion(string kind, TGlobalAddress address, Type fromType, Type toType) {
kind = "unchecked" and
exists(ConvertInstruction convert |
uncheckedConversionTypes(convert, fromType, toType) and
address = globalValueNumber(convert.getUnary())
)
or
kind = "checked" and
exists(CheckedConvertOrNullInstruction convert |
checkedConversionTypes(convert, fromType, toType) and
address = globalValueNumber(convert.getUnary())
)
or
kind = "inheritance" and
exists(InheritanceConversionInstruction convert |
inheritanceConversionTypes(convert, fromType, toType) and
address = globalValueNumber(convert.getUnary())
)
} or
TFieldAddress(TGlobalAddress address, Field f) {
exists(FieldAddressInstruction fai |
fai.getField() = f and
address = globalValueNumber(fai.getObjectAddress())
)
}
pragma[noinline]
predicate uncheckedConversionTypes(ConvertInstruction convert, Type fromType, Type toType) {
fromType = convert.getUnary().getResultType() and
toType = convert.getResultType()
}
pragma[noinline]
predicate checkedConversionTypes(CheckedConvertOrNullInstruction convert, Type fromType, Type toType) {
fromType = convert.getUnary().getResultType() and
toType = convert.getResultType()
}
pragma[noinline]
predicate inheritanceConversionTypes(
InheritanceConversionInstruction convert, Type fromType, Type toType
) {
fromType = convert.getUnary().getResultType() and
toType = convert.getResultType()
}
/** Gets the HashCons value of an address computed by `instr`, if any. */
TGlobalAddress globalValueNumber(Instruction instr) {
result = TGlobalVariable(instr.(VariableAddressInstruction).getASTVariable())
or
not instr instanceof LoadInstruction and
result = globalValueNumber(instr.(CopyInstruction).getSourceValue())
or
exists(LoadInstruction load | instr = load |
result = TLoad(globalValueNumber(load.getSourceAddress()))
)
or
exists(ConvertInstruction convert, Type fromType, Type toType | instr = convert |
uncheckedConversionTypes(convert, fromType, toType) and
result = TConversion("unchecked", globalValueNumber(convert.getUnary()), fromType, toType)
)
or
exists(CheckedConvertOrNullInstruction convert, Type fromType, Type toType | instr = convert |
checkedConversionTypes(convert, fromType, toType) and
result = TConversion("checked", globalValueNumber(convert.getUnary()), fromType, toType)
)
or
exists(InheritanceConversionInstruction convert, Type fromType, Type toType | instr = convert |
inheritanceConversionTypes(convert, fromType, toType) and
result = TConversion("inheritance", globalValueNumber(convert.getUnary()), fromType, toType)
)
or
exists(FieldAddressInstruction fai | instr = fai |
result = TFieldAddress(globalValueNumber(fai.getObjectAddress()), fai.getField())
)
or
result = globalValueNumber(instr.(PointerOffsetInstruction).getLeft())
}
/** Gets a `StoreInstruction` that may be executed after executing `store`. */
pragma[inline]
StoreInstruction getAStoreStrictlyAfter(StoreInstruction store) {
exists(IRBlock block, int index1, int index2 |
block.getInstruction(index1) = store and
block.getInstruction(index2) = result and
index2 > index1
)
or
exists(IRBlock block1, IRBlock block2 |
store.getBlock() = block1 and
result.getBlock() = block2 and
block1.getASuccessor+() = block2
)
}
/**
* Holds if `store` copies the address of `f`'s local variable `var`
* into th address `globalAddress`.
*/
predicate stackAddressEscapes(
StoreInstruction store, StackVariable var, TGlobalAddress globalAddress, Function f
) {
exists(VariableAddressInstruction vai |
stackPointerFlowsToUse(store.getSourceValue(), vai) and
globalAddress = globalValueNumber(store.getDestinationAddress()) and
f = vai.getEnclosingFunction() and
var = vai.getASTVariable()
) and
// Ensure there's no subsequent store that overrides the global address.
not globalAddress = globalValueNumber(getAStoreStrictlyAfter(store).getDestinationAddress())
}
predicate blockStoresToAddress(
IRBlock block, int index, StoreInstruction store, TGlobalAddress globalAddress
) {
block.getInstruction(index) = store and
globalAddress = globalValueNumber(store.getDestinationAddress())
}
predicate blockLoadsFromAddress(
IRBlock block, int index, LoadInstruction load, TGlobalAddress globalAddress
) {
block.getInstruction(index) = load and
globalAddress = globalValueNumber(load.getSourceAddress())
}
predicate globalAddressPointsToStack(
StoreInstruction store, StackVariable var, CallInstruction call, IRBlock block,
TGlobalAddress globalAddress, boolean isCallBlock, boolean isStoreBlock
) {
(
if blockStoresToAddress(block, _, _, globalAddress)
then isStoreBlock = true
else isStoreBlock = false
) and
(
isCallBlock = true and
exists(Function f |
stackAddressEscapes(store, var, globalAddress, f) and
call.getStaticCallTarget() = f and
call.getBlock() = block
)
or
isCallBlock = false and
exists(IRBlock mid |
mid.immediatelyDominates(block) and
// Only recurse if there is no store to `globalAddress` in `mid`.
globalAddressPointsToStack(store, var, call, mid, globalAddress, _, false)
)
)
}
from
StoreInstruction store, StackVariable var, LoadInstruction load, CallInstruction call,
IRBlock block, boolean isCallBlock, TGlobalAddress address, boolean isStoreBlock
where
globalAddressPointsToStack(store, var, call, block, address, isCallBlock, isStoreBlock) and
block.getAnInstruction() = load and
globalValueNumber(load.getSourceAddress()) = address and
(
// We know that we have a sequence:
// (1) store to `address` -> (2) return from `f` -> (3) load from `address`.
// But if (2) and (3) happen in the sam block we need to check the
// block indices to nsure that (3) happens after (2).
if isCallBlock = true
then
// If so, the load must happen after the call.
exists(int callIndex, int loadIndex |
blockLoadsFromAddress(_, loadIndex, load, _) and
block.getInstruction(callIndex) = call and
callIndex < loadIndex
)
else any()
) and
// If there is a store to the address we need to make sure that the load we found was
// before that store (So that the load doesn't read an overwritten value).
if isStoreBlock = true
then
exists(int storeIndex, int loadIndex |
blockStoresToAddress(block, storeIndex, _, address) and
block.getInstruction(loadIndex) = load and
loadIndex < storeIndex
)
else any()
select load, "Stack variable $@ escapes $@ and is used after it has expired.", var, var.toString(),
store, "here"

View File

@@ -0,0 +1,6 @@
---
category: newQuery
---
- A new query titled "Use of expired stack-address" (`cpp/using-expired-stack-address`) has been added.
This query finds accesses to expired stack-allocated memory that escaped via a global variable.

View File

@@ -0,0 +1,24 @@
| test.cpp:15:16:15:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:9:7:9:7 | x | x | test.cpp:10:3:10:13 | Store: ... = ... | here |
| test.cpp:58:16:58:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:51:36:51:36 | x | x | test.cpp:52:3:52:13 | Store: ... = ... | here |
| test.cpp:73:16:73:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:62:7:62:7 | x | x | test.cpp:68:3:68:13 | Store: ... = ... | here |
| test.cpp:98:15:98:15 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:92:8:92:8 | s | s | test.cpp:93:3:93:15 | Store: ... = ... | here |
| test.cpp:111:16:111:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:102:7:102:7 | x | x | test.cpp:106:3:106:14 | Store: ... = ... | here |
| test.cpp:161:16:161:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:136:3:136:12 | Store: ... = ... | here |
| test.cpp:162:16:162:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:137:3:137:16 | Store: ... = ... | here |
| test.cpp:164:16:164:17 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:139:3:139:12 | Store: ... = ... | here |
| test.cpp:165:16:165:17 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:139:3:139:12 | Store: ... = ... | here |
| test.cpp:166:17:166:18 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:140:3:140:16 | Store: ... = ... | here |
| test.cpp:167:16:167:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:141:3:141:15 | Store: ... = ... | here |
| test.cpp:168:17:168:18 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:142:3:142:19 | Store: ... = ... | here |
| test.cpp:170:16:170:17 | Load: p3 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:144:3:144:12 | Store: ... = ... | here |
| test.cpp:171:17:171:18 | Load: p3 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:145:3:145:16 | Store: ... = ... | here |
| test.cpp:172:18:172:19 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:146:3:146:15 | Store: ... = ... | here |
| test.cpp:173:18:173:19 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:147:3:147:19 | Store: ... = ... | here |
| test.cpp:174:18:174:19 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:142:3:142:19 | Store: ... = ... | here |
| test.cpp:175:16:175:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:148:3:148:18 | Store: ... = ... | here |
| test.cpp:177:14:177:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:151:3:151:15 | Store: ... = ... | here |
| test.cpp:178:14:178:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:152:3:152:19 | Store: ... = ... | here |
| test.cpp:179:14:179:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:153:3:153:18 | Store: ... = ... | here |
| test.cpp:180:14:180:19 | Load: * ... | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:154:3:154:22 | Store: ... = ... | here |
| test.cpp:181:13:181:20 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:155:3:155:21 | Store: ... = ... | here |
| test.cpp:182:14:182:19 | Load: * ... | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:156:3:156:25 | Store: ... = ... | here |

View File

@@ -0,0 +1 @@
Likely Bugs/Memory Management/UsingExpiredStackAddress.ql

View File

@@ -0,0 +1,183 @@
struct S100 {
int i;
int* p;
};
static struct S100 s101;
void escape1() {
int x;
s101.p = &x;
}
int simple_field_bad() {
escape1();
return *s101.p; // BAD
}
int simple_field_good() {
escape1();
return s101.i; // GOOD
}
int deref_p() {
return *s101.p;
}
int field_indirect_bad() {
escape1();
return deref_p(); // BAD [NOT DETECTED]
}
int deref_i() {
return s101.i;
}
int field_indirect_good() {
escape1();
return deref_i(); // GOOD
}
void store_argument(int *p) {
s101.p = p;
}
int store_argument_value() {
int x;
store_argument(&x);
return *s101.p; // GOOD
}
void store_address_of_argument(int x) {
s101.p = &x;
}
int store_argument_address() {
int x;
store_address_of_argument(x);
return *s101.p; // BAD
}
void address_escapes_through_pointer_arith() {
int x = 0;
int* p0 = &x;
int* p1 = p0 + 1;
int* p2 = p1 - 1;
int* p3 = 1 + p2;
p3++;
s101.p = p3;
}
int test_pointer_arith_bad() {
address_escapes_through_pointer_arith();
return *s101.p; // BAD
}
int test_pointer_arith_good_1() {
int x;
address_escapes_through_pointer_arith();
s101.p = &x;
return *s101.p; // GOOD [FALSE POSITIVE]
}
int test_pointer_arith_good_2(bool b) {
int x;
if(b) {
address_escapes_through_pointer_arith();
}
return *s101.p; // GOOD (we can't say for sure that this is a local address)
}
void field_address_escapes() {
S100 s;
s101.p = &s.i;
}
int test_field_address_escapes() {
field_address_escapes();
return s101.p[0]; // BAD
}
void escape_through_reference() {
int x = 0;
int& r0 = x;
int& r1 = r0;
r1++;
s101.p = &r1;
}
int test_escapes_through_reference() {
escape_through_reference();
return *s101.p; // BAD
}
struct S300 {
int a1[15];
int a2[14][15];
int a3[13][14][15];
int *p1;
int (*p2)[15];
int (*p3)[14][15];
int** pp;
};
S300 s1;
S300 s2;
S300 s3;
S300 s4;
S300 s5;
S300 s6;
void escape_through_arrays() {
int b1[15];
int b2[14][15];
int b3[13][14][15];
s1.p1 = b1;
s2.p1 = &b1[1];
s1.p2 = b2;
s2.p2 = &b2[1];
s3.p1 = b2[1];
s4.p1 = &b2[1][2];
s1.p3 = b3;
s2.p3 = &b3[1];
s3.p2 = b3[1];
s4.p2 = &b3[1][2];
s5.p1 = b3[1][2];
s6.p1 = &b3[1][2][3];
s1.pp[0] = b1;
s2.pp[0] = &b1[1];
s3.pp[0] = b2[1];
s4.pp[0] = &b2[1][2];
s5.pp[0] = b3[1][2];
s6.pp[0] = &b3[1][2][3];
}
void test_escape_through_arrays() {
escape_through_arrays();
int x1 = *s1.p1; // BAD
int x2 = *s2.p1; // BAD
int* x3 = s1.p2[1]; // BAD
int x4 = *s1.p2[1]; // BAD
int* x5 = *s2.p2; // BAD
int* x6 = s3.p1; // BAD
int x7 = *&s4.p1[1]; // BAD
int x8 = *s1.p3[1][2]; // BAD
int x9 = (*s2.p3[0])[0]; // BAD
int x10 = **s3.p2; // BAD
int x11 = **s4.p2; // BAD
int x12 = (*s4.p1); // BAD
int x13 = s5.p1[1]; // BAD
int* x14 = s1.pp[0]; // BAD
int x15 = *s2.pp[0]; // BAD
int x16 = *s3.pp[0]; // BAD
int x17 = **s4.pp; // BAD
int x18 = s5.pp[0][0]; // BAD
int x19 = (*s6.pp)[0]; // BAD
}