diff --git a/cpp/change-notes/2021-05-18-static-buffer-overflow.md b/cpp/change-notes/2021-05-18-static-buffer-overflow.md new file mode 100644 index 00000000000..f56040fe8aa --- /dev/null +++ b/cpp/change-notes/2021-05-18-static-buffer-overflow.md @@ -0,0 +1,2 @@ +lgtm +* The "Static buffer overflow" query (cpp/static-buffer-overflow) has been improved to produce fewer false positives. diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index 833ee45499e..011c38ff734 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -14,6 +14,7 @@ import cpp import semmle.code.cpp.commons.Buffer +import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis import LoopBounds private predicate staticBufferBase(VariableAccess access, Variable v) { @@ -51,6 +52,8 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) { loop.getStmt().getAChild*() = bufaccess.getEnclosingStmt() and loop.limit() >= bufaccess.bufferSize() and loop.counter().getAnAccess() = bufaccess.getArrayOffset() and + // Ensure that we don't have an upper bound on the array index that's less than the buffer size. + not upperBound(bufaccess.getArrayOffset().getFullyConverted()) < bufaccess.bufferSize() and msg = "Potential buffer-overflow: counter '" + loop.counter().toString() + "' <= " + loop.limit().toString() + " but '" + bufaccess.buffer().getName() + "' has " + @@ -94,17 +97,22 @@ class CallWithBufferSize extends FunctionCall { } int statedSizeValue() { - exists(Expr statedSizeSrc | - DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) and - result = statedSizeSrc.getValue().toInt() - ) + // `upperBound(e)` defaults to `exprMaxVal(e)` when `e` isn't analyzable. So to get a meaningful + // result in this case we pick the minimum value obtainable from dataflow and range analysis. + result = + upperBound(statedSizeExpr()) + .minimum(min(Expr statedSizeSrc | + DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) + | + statedSizeSrc.getValue().toInt() + )) } } predicate wrongBufferSize(Expr error, string msg) { exists(CallWithBufferSize call, int bufsize, Variable buf, int statedSize | staticBuffer(call.buffer(), buf, bufsize) and - statedSize = min(call.statedSizeValue()) and + statedSize = call.statedSizeValue() and statedSize > bufsize and error = call.statedSizeExpr() and msg = diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index c4ccdfad8b3..4f8bcc0fa59 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -586,6 +586,23 @@ void test21(bool cond) if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1] } +void test22(bool b, const char* source) { + char buffer[16]; + int k; + for (k = 0; k <= 100; k++) { + if(k < 16) { + buffer[k] = 'x'; // GOOD + } + } + + char dest[128]; + int n = b ? 1024 : 132; + if (n >= 128) { + return; + } + memcpy(dest, source, n); // GOOD +} + int main(int argc, char *argv[]) { long long arr17[19]; @@ -609,6 +626,7 @@ int main(int argc, char *argv[]) test19(argc == 0); test20(); test21(argc == 0); + test22(argc == 0, argv[0]); return 0; }