Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions change-notes/1.19/analysis-cpp.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. |
| Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
| Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type, or makes a non-returning call to another function. |
| Static array access may cause overflow | More correct results | Data flow to the size argument of a buffer operation is now checked in this query. |
| Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. |
| Self comparison | Fewer false positive results | Code inside macro invocations is now excluded from the query. |
| Suspicious call to memset | Fewer false positive results | Types involving decltype are now correctly compared. |
Expand Down
23 changes: 16 additions & 7 deletions cpp/ql/src/Critical/OverflowStatic.ql
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,31 @@ class CallWithBufferSize extends FunctionCall
Expr buffer() {
exists(int i |
bufferAndSizeFunction(this.getTarget(), i, _) and
result = this.getArgument(i))
result = this.getArgument(i)
)
}
Expr statedSize() {
Expr statedSizeExpr() {
exists(int i |
bufferAndSizeFunction(this.getTarget(), _, i) and
result = this.getArgument(i))
result = this.getArgument(i)
)
}
int statedSizeValue() {
exists(Expr statedSizeSrc |
DataFlow::localFlow(DataFlow::exprNode(statedSizeSrc), DataFlow::exprNode(statedSizeExpr())) and
result = statedSizeSrc.getValue().toInt()
)
}
}

predicate wrongBufferSize(Expr error, string msg) {
exists(CallWithBufferSize call, int bufsize, Variable buf |
exists(CallWithBufferSize call, int bufsize, Variable buf, int statedSize |
staticBuffer(call.buffer(), buf, bufsize) and
call.statedSize().getValue().toInt() > bufsize and
error = call.statedSize() and
statedSize = min(call.statedSizeValue()) and
statedSize > bufsize and
error = call.statedSizeExpr() and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you've introduced data flow, we need to decide what to do when multiple values may flow to error. With this PR we'll start seeing false positives in code like

char buf10[10], buf20[20];
if (b) { size = 10; } else { size = 20; }
if (b) {
  memcpy(buf10, src, size); // false positive
} else {
  memcpy(buf20, src, size);
}

You can be more conservative, and still address CPP-299, by replacing call.statedSizeValue() with min(call.statedSizeValue()). That won't address https://github.com/MosheWagner/VulnC/blob/f787c51fbe09d2fb88403d3b01bca7709fd7ed9e/src/bad_read.c#L158-L169, but I would prefer to err on the side of false negatives until we run a CPP-Differences job and see that such FPs are rare in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes sense to be conservative at this time. Done.

msg = "Potential buffer-overflow: '" + buf.getName() +
"' has size " + bufsize.toString() + " not " + call.statedSize().getValue() + ".")
"' has size " + bufsize.toString() + " not " + statedSize + ".")
}

predicate outOfBounds(BufferAccess bufaccess, string msg)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
| test.cpp:20:3:20:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer2' has 3 elements. |
| test.cpp:24:27:24:27 | 4 | Potential buffer-overflow: 'buffer1' has size 3 not 4. |
| test.cpp:26:27:26:27 | 4 | Potential buffer-overflow: 'buffer2' has size 3 not 4. |
| test.cpp:40:22:40:27 | amount | Potential buffer-overflow: 'buffer' has size 100 not 101. |
21 changes: 21 additions & 0 deletions cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,24 @@ void f1(void)
memcpy(buffer2, buffer1, 3); // GOOD
memcpy(buffer2, buffer1, 4); // BAD
}

void f2(char *src)
{
char buffer[100];
char *ptr;
int amount;

amount = 100;
memcpy(buffer, src, amount); // GOOD
amount = amount + 1;
memcpy(buffer, src, amount); // BAD [NOT DETECTED]
amount = 101;
memcpy(buffer, src, amount); // BAD

ptr = buffer;
memcpy(ptr, src, 101); // BAD [NOT DETECTED]
ptr = &(buffer[0]);
memcpy(ptr, src, 101); // BAD [NOT DETECTED]
ptr = &(buffer[1]);
memcpy(ptr, src, 100); // BAD [NOT DETECTED]
}