From a0059202db33b7476e7fbd2acf41858ace55854a Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 14 Dec 2021 10:01:53 +0000 Subject: [PATCH] C++: split `cpp/overrunning-write` into two This splits the `cpp/overruning-write` into two separate queries based off on the reason for the estimation. If the overrun is detected based on non-trivial range analysis, the results are now marked by the new `cpp/very-likely-overruning-write` high precision query. If it is based on less precise, usually type based bounds, then it will still be marked by `cpp/overruning-write` which remains at medium precision. --- .../src/Security/CWE/CWE-120/OverrunWrite.ql | 4 +-- .../CWE/CWE-120/VeryLikelyOverrunWrite.ql | 34 +++++++++++++++++++ .../SAMATE/VeryLikelyOverrunWrite.expected | 0 .../SAMATE/VeryLikelyOverrunWrite.qlref | 1 + .../tests/VeryLikelyOverrunWrite.expected | 0 .../semmle/tests/VeryLikelyOverrunWrite.qlref | 1 + .../semmle/tests/OverrunWrite.expected | 21 ------------ .../tests/VeryLikelyOverrunWrite.expected | 21 ++++++++++++ .../semmle/tests/VeryLikelyOverrunWrite.qlref | 1 + .../semmle/tests/OverrunWrite.expected | 14 -------- .../tests/VeryLikelyOverrunWrite.expected | 14 ++++++++ .../semmle/tests/VeryLikelyOverrunWrite.qlref | 1 + 12 files changed, 75 insertions(+), 37 deletions(-) create mode 100644 cpp/ql/src/Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.qlref create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/VeryLikelyOverrunWrite.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/VeryLikelyOverrunWrite.qlref create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.qlref create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.qlref diff --git a/cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql b/cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql index 59c790a0c3a..e62df8c3f0d 100644 --- a/cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql +++ b/cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql @@ -21,12 +21,12 @@ import semmle.code.cpp.commons.Alloc * See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases. */ -from BufferWrite bw, Expr dest, int destSize, int estimated +from BufferWrite bw, Expr dest, int destSize, int estimated, TypeBoundsAnalysis reason where not bw.hasExplicitLimit() and // has no explicit size limit dest = bw.getDest() and destSize = getBufferSize(dest, _) and - estimated = bw.getMaxDataLimited(_) and + estimated = bw.getMaxDataLimited(reason) and // we can deduce that too much data may be copied (even without // long '%f' conversions) estimated > destSize diff --git a/cpp/ql/src/Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql b/cpp/ql/src/Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql new file mode 100644 index 00000000000..a67e1a58fbb --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql @@ -0,0 +1,34 @@ +/** + * @name Likely overrunning write based on non-trivial analysis of value ranges + * @description Buffer write operations that do not control the length + * of data written may overflow + * @kind problem + * @problem.severity error + * @security-severity 9.3 + * @precision high + * @id cpp/very-likely-overrunning-write + * @tags reliability + * security + * external/cwe/cwe-120 + * external/cwe/cwe-787 + * external/cwe/cwe-805 + */ + +import semmle.code.cpp.security.BufferWrite +import semmle.code.cpp.commons.Alloc + +/* + * See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases. + */ + +from BufferWrite bw, Expr dest, int destSize, int estimated, ValueFlowAnalysis reason +where + not bw.hasExplicitLimit() and // has no explicit size limit + dest = bw.getDest() and + destSize = getBufferSize(dest, _) and + estimated = bw.getMaxDataLimited(reason) and + // we can deduce from non-trivial range analysis that too much data may be copied + estimated > destSize +select bw, + "This '" + bw.getBWDesc() + "' operation requires " + estimated + + " bytes but the destination is only " + destSize + " bytes." diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.qlref new file mode 100644 index 00000000000..94b53951c4b --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/VeryLikelyOverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/VeryLikelyOverrunWrite.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/VeryLikelyOverrunWrite.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/VeryLikelyOverrunWrite.qlref new file mode 100644 index 00000000000..94b53951c4b --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/VeryLikelyOverrunWrite.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected index b21bb0c556b..e69de29bb2d 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected @@ -1,21 +0,0 @@ -| tests2.cpp:17:3:17:8 | call to wcscpy | This 'call to wcscpy' operation requires 12 bytes but the destination is only 8 bytes. | -| tests2.cpp:22:3:22:8 | call to wcscpy | This 'call to wcscpy' operation requires 16 bytes but the destination is only 12 bytes. | -| tests2.cpp:27:3:27:8 | call to wcscpy | This 'call to wcscpy' operation requires 20 bytes but the destination is only 16 bytes. | -| tests2.cpp:31:3:31:8 | call to wcscpy | This 'call to wcscpy' operation requires 24 bytes but the destination is only 20 bytes. | -| tests2.cpp:36:3:36:8 | call to wcscpy | This 'call to wcscpy' operation requires 28 bytes but the destination is only 24 bytes. | -| tests2.cpp:41:3:41:8 | call to wcscpy | This 'call to wcscpy' operation requires 32 bytes but the destination is only 28 bytes. | -| tests2.cpp:46:3:46:8 | call to wcscpy | This 'call to wcscpy' operation requires 36 bytes but the destination is only 32 bytes. | -| tests.c:54:3:54:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | -| tests.c:58:3:58:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | -| tests.c:62:17:62:24 | buffer10 | This 'scanf string argument' operation requires 11 bytes but the destination is only 10 bytes. | -| tests.c:63:17:63:24 | buffer10 | This 'scanf string argument' operation requires 12 bytes but the destination is only 10 bytes. | -| tests.c:86:3:86:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | -| tests.c:93:3:93:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | -| tests.c:120:3:120:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 1 bytes. | -| tests.c:121:3:121:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 16 bytes. | -| tests.c:136:2:136:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | -| unions.c:26:2:26:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | -| unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 15 bytes. | -| unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | -| unions.c:32:2:32:7 | call to strcpy | This 'call to strcpy' operation requires 31 bytes but the destination is only 25 bytes. | -| var_size_struct.cpp:22:3:22:8 | call to strcpy | This 'call to strcpy' operation requires 10 bytes but the destination is only 9 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected new file mode 100644 index 00000000000..b21bb0c556b --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected @@ -0,0 +1,21 @@ +| tests2.cpp:17:3:17:8 | call to wcscpy | This 'call to wcscpy' operation requires 12 bytes but the destination is only 8 bytes. | +| tests2.cpp:22:3:22:8 | call to wcscpy | This 'call to wcscpy' operation requires 16 bytes but the destination is only 12 bytes. | +| tests2.cpp:27:3:27:8 | call to wcscpy | This 'call to wcscpy' operation requires 20 bytes but the destination is only 16 bytes. | +| tests2.cpp:31:3:31:8 | call to wcscpy | This 'call to wcscpy' operation requires 24 bytes but the destination is only 20 bytes. | +| tests2.cpp:36:3:36:8 | call to wcscpy | This 'call to wcscpy' operation requires 28 bytes but the destination is only 24 bytes. | +| tests2.cpp:41:3:41:8 | call to wcscpy | This 'call to wcscpy' operation requires 32 bytes but the destination is only 28 bytes. | +| tests2.cpp:46:3:46:8 | call to wcscpy | This 'call to wcscpy' operation requires 36 bytes but the destination is only 32 bytes. | +| tests.c:54:3:54:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | +| tests.c:58:3:58:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | +| tests.c:62:17:62:24 | buffer10 | This 'scanf string argument' operation requires 11 bytes but the destination is only 10 bytes. | +| tests.c:63:17:63:24 | buffer10 | This 'scanf string argument' operation requires 12 bytes but the destination is only 10 bytes. | +| tests.c:86:3:86:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | +| tests.c:93:3:93:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | +| tests.c:120:3:120:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 1 bytes. | +| tests.c:121:3:121:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 16 bytes. | +| tests.c:136:2:136:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | +| unions.c:26:2:26:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | +| unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 15 bytes. | +| unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | +| unions.c:32:2:32:7 | call to strcpy | This 'call to strcpy' operation requires 31 bytes but the destination is only 25 bytes. | +| var_size_struct.cpp:22:3:22:8 | call to strcpy | This 'call to strcpy' operation requires 10 bytes but the destination is only 9 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.qlref new file mode 100644 index 00000000000..94b53951c4b --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected index 89090ed53fb..c63801051ff 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected @@ -1,19 +1,5 @@ | tests.cpp:258:2:258:8 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 10 bytes. | | tests.cpp:259:2:259:8 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 10 bytes. | -| tests.cpp:272:2:272:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. | -| tests.cpp:273:2:273:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. | -| tests.cpp:308:3:308:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. | | tests.cpp:315:2:315:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. | | tests.cpp:316:2:316:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. | -| tests.cpp:321:2:321:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. | -| tests.cpp:324:3:324:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. | | tests.cpp:327:2:327:8 | call to sprintf | This 'call to sprintf' operation requires 12 bytes but the destination is only 4 bytes. | -| tests.cpp:329:3:329:9 | call to sprintf | This 'call to sprintf' operation requires 12 bytes but the destination is only 4 bytes. | -| tests.cpp:341:2:341:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | -| tests.cpp:343:2:343:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | -| tests.cpp:345:2:345:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 2 bytes. | -| tests.cpp:347:2:347:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | -| tests.cpp:350:2:350:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. | -| tests.cpp:354:2:354:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. | -| tests.cpp:358:2:358:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. | -| tests.cpp:363:2:363:8 | call to sprintf | This 'call to sprintf' operation requires 5 bytes but the destination is only 4 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.expected new file mode 100644 index 00000000000..7042c091732 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.expected @@ -0,0 +1,14 @@ +| tests.cpp:272:2:272:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. | +| tests.cpp:273:2:273:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. | +| tests.cpp:308:3:308:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. | +| tests.cpp:321:2:321:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. | +| tests.cpp:324:3:324:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. | +| tests.cpp:329:3:329:9 | call to sprintf | This 'call to sprintf' operation requires 12 bytes but the destination is only 4 bytes. | +| tests.cpp:341:2:341:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | +| tests.cpp:343:2:343:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | +| tests.cpp:345:2:345:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 2 bytes. | +| tests.cpp:347:2:347:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | +| tests.cpp:350:2:350:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. | +| tests.cpp:354:2:354:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. | +| tests.cpp:358:2:358:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. | +| tests.cpp:363:2:363:8 | call to sprintf | This 'call to sprintf' operation requires 5 bytes but the destination is only 4 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.qlref new file mode 100644 index 00000000000..94b53951c4b --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql \ No newline at end of file