Skip to content

C++: Simplify buffer.qll repair #11042

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
MathiasVP merged 3 commits into from
Nov 3, 2022
Merged

C++: Simplify buffer.qll repair #11042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa8214a
C++: Simplify 'Buffer.qll' by avoiding 'asIndirectExpr'. This removes…
MathiasVP Oct 30, 2022
1b50168
C++: Add an initial pruning stage to prevent this
MathiasVP Oct 30, 2022
30f1547
C++: Use 'max' instead of 'unique.'
MathiasVP Nov 1, 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
129 changes: 41 additions & 88 deletions cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,6 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
exists(ArrayType t | t = v.getUnspecifiedType() | not t.getArraySize() > 1)
}

/**
* Gets the expression associated with `n`. Unlike `n.asExpr()` this also gets the
* expression underlying an indirect dataflow node.
*/
private Expr getExpr(DataFlow::Node n, boolean isIndirect) {
result = n.asExpr() and isIndirect = false
or
result = n.asIndirectExpr() and isIndirect = true
}

private DataFlow::Node exprNode(Expr e, boolean isIndirect) { e = getExpr(result, isIndirect) }

/**
* Holds if `bufferExpr` is an allocation-like expression.
*
Expand Down Expand Up @@ -85,77 +73,19 @@ private int isSource(Expr bufferExpr, Element why) {
)
}

/** Holds if the value of `n2 + delta` may be equal to the value of `n1`. */
private predicate localFlowIncrStep(DataFlow::Node n1, DataFlow::Node n2, int delta) {
DataFlow::localFlowStep(n1, n2) and
(
exists(IncrementOperation incr |
n1.asIndirectExpr() = incr.getOperand() and
delta = -1
)
or
exists(DecrementOperation decr |
n1.asIndirectExpr() = decr.getOperand() and
delta = 1
)
or
exists(AddExpr add, Expr e1, Expr e2 |
add.hasOperands(e1, e2) and
n1.asIndirectExpr() = e1 and
delta = -e2.getValue().toInt()
)
or
exists(SubExpr add, Expr e1, Expr e2 |
add.hasOperands(e1, e2) and
n1.asIndirectExpr() = e1 and
delta = e2.getValue().toInt()
)
)
}

/**
* Holds if `n1` may flow to `n2` without passing through any back-edges.
*
* Back-edges are excluded to prevent infinite loops on examples like:
* ```
* while(true) { ++n; }
* ```
* which, when used in `localFlowStepRec`, would create infinite loop that continuously
* increments the `delta` parameter.
*/
private predicate localFlowNotIncrStep(DataFlow::Node n1, DataFlow::Node n2) {
not localFlowIncrStep(n1, n2, _) and
DataFlow::localFlowStep(n1, n2) and
not n1 = n2.(DataFlow::SsaPhiNode).getAnInput(true)
}

private predicate localFlowToExprStep(DataFlow::Node n1, DataFlow::Node n2) {
not exists([n1.asExpr(), n1.asIndirectExpr()]) and
localFlowNotIncrStep(n1, n2)
}

/** Holds if `mid2 + delta` may be equal to `n1`. */
private predicate localFlowStepRec0(DataFlow::Node n1, DataFlow::Node mid2, int delta) {
exists(DataFlow::Node mid1, int d1, int d2 |
// Or we take a number of steps that adds `d1` to the pointer
localFlowStepRec(n1, mid1, d1) and
// followed by a step that adds `d2` to the pointer
localFlowIncrStep(mid1, mid2, d2) and
delta = d1 + d2
)
not exists(n2.asExpr()) and
DataFlow::localFlowStep(n1, n2)
}

/** Holds if `n2 + delta` may be equal to `n1`. */
private predicate localFlowStepRec(DataFlow::Node n1, DataFlow::Node n2, int delta) {
// Either we take one or more steps that doesn't modify the size of the buffer
localFlowNotIncrStep+(n1, n2) and
delta = 0
or
exists(DataFlow::Node mid2 |
// Or we step from `n1` to `mid2 + delta`
localFlowStepRec0(n1, mid2, delta) and
// and finally to the next `ExprNode`.
localFlowToExprStep*(mid2, n2)
private predicate localFlowStepToExpr(Expr e1, Expr e2) {
getBufferSizeCand0(e1) and
exists(DataFlow::Node n1, DataFlow::Node mid, DataFlow::Node n2 |
n1.asExpr() = e1 and
localFlowToExprStep*(n1, mid) and
DataFlow::localFlowStep(mid, n2) and
n2.asExpr() = e2
)
}

Expand All @@ -165,6 +95,7 @@ private predicate localFlowStepRec(DataFlow::Node n1, DataFlow::Node n2, int del
* expression.
*/
private predicate step(Expr e1, Expr e2, int delta) {
getBufferSizeCand0(e1) and
exists(Variable bufferVar, Class parentClass, VariableAccess parentPtr, int bufferSize |
e1 = parentPtr
|
Expand All @@ -181,20 +112,42 @@ private predicate step(Expr e1, Expr e2, int delta) {
delta = bufferSize - parentClass.getSize()
)
or
exists(boolean isIndirect |
localFlowStepRec(exprNode(e1, isIndirect), exprNode(e2, isIndirect), delta)
localFlowStepToExpr(e1, e2) and
delta = 0
}

pragma[nomagic]
private predicate getBufferSizeCand0(Expr e) {
exists(isSource(e, _))
or
exists(Expr e0 |
getBufferSizeCand0(e0) and
step(e0, e, _)
)
}

/**
* Get the size in bytes of the buffer pointed to by an expression (if this can be determined).
*
* NOTE: There can be multiple `(result, why)` for a given `bufferExpr`.
*/
private int getBufferSizeCand(Expr bufferExpr, Element why) {
getBufferSizeCand0(bufferExpr) and
(
result = isSource(bufferExpr, why)
or
exists(Expr e0, int delta, int size |
size = getBufferSizeCand(e0, why) and
step(e0, bufferExpr, delta) and
result = size + delta
)
)
}

/**
* Get the size in bytes of the buffer pointed to by an expression (if this can be determined).
*/
int getBufferSize(Expr bufferExpr, Element why) {
result = isSource(bufferExpr, why)
or
exists(Expr e0, int delta, int size |
size = getBufferSize(e0, why) and
delta = unique(int cand | step(e0, bufferExpr, cand) | cand) and
result = size + delta
)
result = max( | | getBufferSizeCand(bufferExpr, _)) and
result = getBufferSizeCand(bufferExpr, why)
}
1 change: 0 additions & 1 deletion cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +0,0 @@
| tests.cpp:668:9:668:14 | call to strcpy | This 'call to strcpy' operation requires 11 bytes but the destination is only 10 bytes. |
12 changes: 0 additions & 12 deletions cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
| overflowdestination.cpp:46:2:46:7 | call to memcpy | This 'memcpy' operation accesses 128 bytes but the $@ is only 64 bytes. | overflowdestination.cpp:40:7:40:10 | dest | destination buffer |
| tests.cpp:23:2:23:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | destination buffer |
| tests.cpp:23:2:23:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | source buffer |
| tests.cpp:25:2:25:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | destination buffer |
| tests.cpp:25:2:25:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | source buffer |
| tests.cpp:172:23:172:31 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:170:17:170:41 | {...} | array |
| tests.cpp:176:23:176:30 | access to array | This array indexing operation accesses byte offset 31 but the $@ is only 24 bytes. | tests.cpp:170:17:170:41 | {...} | array |
| tests.cpp:222:3:222:8 | call to memset | This 'memset' operation accesses 33 bytes but the $@ is only 32 bytes. | tests.cpp:214:8:214:14 | buffer1 | destination buffer |
Expand Down Expand Up @@ -38,36 +36,26 @@
| tests.cpp:376:3:376:13 | access to array | This array indexing operation accesses byte offset 101 but the $@ is only 101 bytes. | tests.cpp:369:47:369:52 | call to malloc | array |
| tests.cpp:446:3:446:24 | access to array | This array indexing operation accesses a negative index -3 on the $@. | tests.cpp:444:7:444:14 | intArray | array |
| tests.cpp:454:3:454:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
| tests.cpp:455:3:455:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
| tests.cpp:456:3:456:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
| tests.cpp:456:3:456:15 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
| tests.cpp:457:3:457:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
| tests.cpp:459:3:459:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
| tests.cpp:461:3:461:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
| tests.cpp:476:2:476:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
| tests.cpp:477:2:477:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
| tests.cpp:479:2:479:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
| tests.cpp:481:2:481:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
| tests.cpp:485:2:485:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
| tests.cpp:487:2:487:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
| tests.cpp:489:2:489:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
| tests.cpp:491:2:491:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
| tests.cpp:495:2:495:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
| tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:502:15:502:20 | call to malloc | destination buffer |
| tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:510:16:510:21 | call to malloc | destination buffer |
| tests.cpp:520:3:520:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:503:15:503:20 | call to malloc | destination buffer |
| tests.cpp:541:6:541:10 | call to fread | This 'fread' operation may access 101 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
| tests.cpp:546:6:546:10 | call to fread | This 'fread' operation may access 400 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
| tests.cpp:569:6:569:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
| tests.cpp:575:7:575:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
| tests.cpp:577:7:577:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
| tests.cpp:579:6:579:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
| tests.cpp:586:6:586:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
| tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer |
| unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer |
| unions.cpp:27:2:27:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
| unions.cpp:29:2:29:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
| unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
| unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
| unions.cpp:34:2:34:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:16:7:16:11 | large | destination buffer |
| var_size_struct.cpp:71:3:71:8 | call to memset | This 'memset' operation accesses 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:67:35:67:40 | call to malloc | destination buffer |
Expand Down
1 change: 0 additions & 1 deletion cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
| overflowdestination.cpp:46:20:46:30 | sizeof(<expr>) | Potential buffer-overflow: 'dest' has size 64 not 128. |
| tests.cpp:23:33:23:49 | sizeof(<expr>) | Potential buffer-overflow: 'bigbuffer' has size 10 not 20. |
| tests.cpp:25:33:25:49 | sizeof(<expr>) | Potential buffer-overflow: 'smallbuffer' has size 10 not 20. |
| tests.cpp:163:3:163:11 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |
| tests.cpp:164:8:164:16 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |
Expand Down
12 changes: 6 additions & 6 deletions cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ void test1()
char bigbuffer[20];

memcpy(bigbuffer, smallbuffer, sizeof(smallbuffer)); // GOOD
memcpy(bigbuffer, smallbuffer, sizeof(bigbuffer)); // BAD: over-read
memcpy(bigbuffer, smallbuffer, sizeof(bigbuffer)); // BAD: over-read [NOT DETECTED]
memcpy(smallbuffer, bigbuffer, sizeof(smallbuffer)); // GOOD
memcpy(smallbuffer, bigbuffer, sizeof(bigbuffer)); // BAD: over-write
}
Expand Down Expand Up @@ -454,7 +454,7 @@ void test17(long long *longArray)
multi[-5][5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
multi[5][-5] = 0; // DUBIOUS: underrun write (this one is still within the bounds of the whole array)
multi[-5][-5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
multi[0][-5] = 0; // BAD: underrun write
multi[0][-5] = 0; // BAD: underrun write [NOT DETECTED]

multi[15][5] = 0; // BAD: overrun write
multi[5][15] = 0; // DUBIOUS: overrun write (this one is still within the bounds of the whole array)
Expand All @@ -476,7 +476,7 @@ void test18()
p1[-1] = 0; // BAD: underrun write
p2[-1] = 0; // BAD: underrun write
p2++;
p2[-1] = 0; // GOOD [FALSE POSITIVE]
p2[-1] = 0; // GOOD

p3[-1] = 0; // BAD
while (*p3 != 0) {
Expand All @@ -486,7 +486,7 @@ void test18()

p4[-1] = 0; // BAD: underrun write
p4++;
p4[-1] = 0; // GOOD [FALSE POSITIVE]
p4[-1] = 0; // GOOD

p5[-1] = 0; // BAD
while (*p5 != 0) {
Expand Down Expand Up @@ -517,7 +517,7 @@ void test19(bool b)
if (b)
{
memset(p1, 0, 20); // BAD
memset(p2, 0, 20); // GOOD [FALSE POSITIVE]
memset(p2, 0, 20); // GOOD
memset(p3, 0, 20); // GOOD
}
}
Expand Down Expand Up @@ -572,7 +572,7 @@ void test21(bool cond)
if (cond)
{
ptr++;
if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0] [FALSE POSITIVE]
if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0]
} else {
if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1]
}
Expand Down