From 7194121eae70c8de62c9b6e6f757e0740d7bb945 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 7 Feb 2019 18:25:19 +0000 Subject: [PATCH 1/7] CPP: Expand the test cases covering PotentialBufferOverflow.ql. --- .../tests/PotentialBufferOverflow.expected | 1 + .../Security/CWE/CWE-242/semmle/tests/tests.cpp | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected index 60bded30c72f..a48e7323fdd5 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected @@ -2,3 +2,4 @@ | tests.cpp:259:2:259:8 | call to sprintf | This conversion may yield a string of length 17, which exceeds the allocated buffer size of 10 | | tests.cpp:272:2:272:8 | call to sprintf | This conversion may yield a string of length 9, which exceeds the allocated buffer size of 8 | | tests.cpp:273:2:273:8 | call to sprintf | This conversion may yield a string of length 9, which exceeds the allocated buffer size of 8 | +| tests.cpp:287:2:287:8 | call to sprintf | This conversion may yield a string of length 318, which exceeds the allocated buffer size of 64 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/tests.cpp index c5f4cd4d37be..5cf53554a272 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/tests.cpp @@ -272,3 +272,20 @@ void test4() sprintf(buffer8, "12345678"); // BAD: buffer overflow sprintf(buffer8_ptr, "12345678"); // BAD: buffer overflow } + +typedef void *va_list; +int vsprintf(char *s, const char *format, va_list arg); + +void test5(va_list args, float f) +{ + char buffer10[10], buffer64[64]; + char *buffer4 = new char[4 * sizeof(char)]; + + vsprintf(buffer10, "123456789", args); // GOOD + vsprintf(buffer10, "1234567890", args); // BAD: buffer overflow [NOT DETECTED] + + sprintf(buffer64, "%f", f); // BAD: potential buffer overflow + + vsprintf(buffer4, "123", args); // GOOD + vsprintf(buffer4, "1234", args); // BAD: buffer overflow [NOT DETECTED] +} From 8a5bc24b364351b1b1611a8028bb8d08e25695cd Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 8 Feb 2019 09:45:30 +0000 Subject: [PATCH 2/7] CPP: Replace PotentialBufferOverflow with OverrunWrite in the test. --- .../Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected | 4 ++++ .../Security/CWE/CWE-242/semmle/tests/OverrunWrite.qlref | 1 + .../CWE/CWE-242/semmle/tests/OverrunWriteFloat.expected | 1 + .../CWE/CWE-242/semmle/tests/OverrunWriteFloat.qlref | 1 + .../CWE-242/semmle/tests/PotentialBufferOverflow.expected | 5 ----- .../CWE/CWE-242/semmle/tests/PotentialBufferOverflow.qlref | 1 - 6 files changed, 7 insertions(+), 6 deletions(-) create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.qlref create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.qlref delete mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected delete mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.qlref 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 new file mode 100644 index 000000000000..0467d3509c95 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected @@ -0,0 +1,4 @@ +| 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. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.qlref new file mode 100644 index 000000000000..f6c962c1a7b4 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-120/OverrunWrite.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.expected new file mode 100644 index 000000000000..1ca5ad49c3fd --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.expected @@ -0,0 +1 @@ +| tests.cpp:287:2:287:8 | call to sprintf | This 'call to sprintf' operation may require 318 bytes because of float conversions, but the target is only 64 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.qlref new file mode 100644 index 000000000000..757d1592e830 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWriteFloat.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-120/OverrunWriteFloat.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected deleted file mode 100644 index a48e7323fdd5..000000000000 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.expected +++ /dev/null @@ -1,5 +0,0 @@ -| tests.cpp:258:2:258:8 | call to sprintf | This conversion may yield a string of length 17, which exceeds the allocated buffer size of 10 | -| tests.cpp:259:2:259:8 | call to sprintf | This conversion may yield a string of length 17, which exceeds the allocated buffer size of 10 | -| tests.cpp:272:2:272:8 | call to sprintf | This conversion may yield a string of length 9, which exceeds the allocated buffer size of 8 | -| tests.cpp:273:2:273:8 | call to sprintf | This conversion may yield a string of length 9, which exceeds the allocated buffer size of 8 | -| tests.cpp:287:2:287:8 | call to sprintf | This conversion may yield a string of length 318, which exceeds the allocated buffer size of 64 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.qlref deleted file mode 100644 index e51b6bfd42e2..000000000000 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/PotentialBufferOverflow.qlref +++ /dev/null @@ -1 +0,0 @@ -Likely Bugs/Memory Management/PotentialBufferOverflow.ql \ No newline at end of file From 45315cda90362bcfb719dd3b0a068239fb35369d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 8 Feb 2019 09:54:29 +0000 Subject: [PATCH 3/7] CPP: Deprecate PotentialBufferOverflow.ql. --- .../Likely Bugs/Memory Management/PotentialBufferOverflow.ql | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql b/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql index 90786a87be5b..b83c3dbdee44 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql @@ -9,6 +9,10 @@ * @tags reliability * security * external/cwe/cwe-676 + * @deprecated + * + * This query is deprecated, use Security/CWE/CWE-120/OverrunWrite.ql and + * Security/CWE/CWE-120/OverrunWriteFloat.ql instead. */ import cpp import semmle.code.cpp.commons.Buffer From 3f2e90291224d10b70da8435d72bf2b0fad187b3 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 8 Feb 2019 11:21:58 +0000 Subject: [PATCH 4/7] CPP: Remove it from the security dashboard (OverrunWrite.ql is already on there). --- cpp/config/suites/security/cwe-242 | 3 --- cpp/config/suites/security/default | 1 - 2 files changed, 4 deletions(-) delete mode 100644 cpp/config/suites/security/cwe-242 diff --git a/cpp/config/suites/security/cwe-242 b/cpp/config/suites/security/cwe-242 deleted file mode 100644 index 0a08d9620bfa..000000000000 --- a/cpp/config/suites/security/cwe-242 +++ /dev/null @@ -1,3 +0,0 @@ -# CWE-242: Use of Inherently Dangerous Function -+ semmlecode-cpp-queries/Likely Bugs/Memory Management/PotentialBufferOverflow.ql: /CWE/CWE-242 - @name Use of inherently dangerous function (CWE-242) diff --git a/cpp/config/suites/security/default b/cpp/config/suites/security/default index 9ce1f2e8fc92..7257ece5dec9 100644 --- a/cpp/config/suites/security/default +++ b/cpp/config/suites/security/default @@ -12,7 +12,6 @@ @import "cwe-134" @import "cwe-170" @import "cwe-190" -@import "cwe-242" @import "cwe-253" @import "cwe-290" @import "cwe-311" From 74f7379ab9f132bd7cb0dc453a8112d1d4c7be71 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 8 Feb 2019 11:32:00 +0000 Subject: [PATCH 5/7] CPP: Change note. --- change-notes/1.20/analysis-cpp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.20/analysis-cpp.md b/change-notes/1.20/analysis-cpp.md index 67c4b36f931a..f671abf4aa7a 100644 --- a/change-notes/1.20/analysis-cpp.md +++ b/change-notes/1.20/analysis-cpp.md @@ -33,6 +33,7 @@ | Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | More correct results | Data flow through global variables for this query has been improved. | | Use of inherently dangerous function (`cpp/potential-buffer-overflow`) | Cleaned up | This query no longer catches uses of `gets`, and has been renamed 'Potential buffer overflow'. | | Use of potentially dangerous function (`cpp/potentially-dangerous-function`) | More correct results | This query now catches uses of `gets`. | +| Potential buffer overflow (`cpp/potential-buffer-overflow`) | Deprecated | This query has been deprecated. Use Potentially overrunning write (`cpp/overrunning-write`) and Potentially overrunning write with float to string conversion (`cpp/overrunning-write-with-float`) instead. | ## Changes to QL libraries From f0356bb83bbcbf4329d889472400ae2be632214f Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 27 Feb 2019 13:18:29 +0000 Subject: [PATCH 6/7] CPP: Reformat @deprecated message. --- .../Memory Management/PotentialBufferOverflow.ql | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql b/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql index b83c3dbdee44..99e771d0fbfa 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql @@ -9,10 +9,9 @@ * @tags reliability * security * external/cwe/cwe-676 - * @deprecated - * - * This query is deprecated, use Security/CWE/CWE-120/OverrunWrite.ql and - * Security/CWE/CWE-120/OverrunWriteFloat.ql instead. + * @deprecated This query is deprecated, use + * Security/CWE/CWE-120/OverrunWrite.ql and + * Security/CWE/CWE-120/OverrunWriteFloat.ql instead. */ import cpp import semmle.code.cpp.commons.Buffer From 25a5ff5e5574078e31b87090970624c9c02c849c Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 27 Feb 2019 13:20:24 +0000 Subject: [PATCH 7/7] CPP: Similarly update other @deprecated messages. --- cpp/ql/src/Likely Bugs/OO/NonVirtualDestructor.ql | 6 +++--- cpp/ql/src/PointsTo/Debug.ql | 4 +--- cpp/ql/src/PointsTo/PreparedStagedPointsTo.ql | 4 +--- cpp/ql/src/PointsTo/Stats.ql | 4 +--- cpp/ql/src/PointsTo/TaintedFormatStrings.ql | 4 +--- 5 files changed, 7 insertions(+), 15 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/OO/NonVirtualDestructor.ql b/cpp/ql/src/Likely Bugs/OO/NonVirtualDestructor.ql index 94d81e55eb5e..2cb6d111c9bd 100644 --- a/cpp/ql/src/Likely Bugs/OO/NonVirtualDestructor.ql +++ b/cpp/ql/src/Likely Bugs/OO/NonVirtualDestructor.ql @@ -7,11 +7,11 @@ * @id cpp/non-virtual-destructor * @problem.severity warning * @tags reliability - * @deprecated + * @deprecated This query is deprecated, and replaced by + * jsf/4.10 Classes/AV Rule 78.ql, which has far fewer false + * positives on typical code. */ -// This query is deprecated, and replaced by jsf/4.10 Classes/AV Rule 78.ql, which has far fewer false positives on typical code. - import cpp from Class base, Destructor d1, Class derived, Destructor d2 diff --git a/cpp/ql/src/PointsTo/Debug.ql b/cpp/ql/src/PointsTo/Debug.ql index 20fe5ea978f2..dce39ff83eb6 100644 --- a/cpp/ql/src/PointsTo/Debug.ql +++ b/cpp/ql/src/PointsTo/Debug.ql @@ -3,11 +3,9 @@ * @description Query to help investigate mysterious results with ReturnStackAllocatedObject * @kind table * @id cpp/points-to/debug - * @deprecated + * @deprecated This query is not suitable for production use and has been deprecated. */ -// This query is not suitable for production use and has been deprecated. - import cpp import semmle.code.cpp.pointsto.PointsTo diff --git a/cpp/ql/src/PointsTo/PreparedStagedPointsTo.ql b/cpp/ql/src/PointsTo/PreparedStagedPointsTo.ql index ec612bc544a2..e0290e047345 100644 --- a/cpp/ql/src/PointsTo/PreparedStagedPointsTo.ql +++ b/cpp/ql/src/PointsTo/PreparedStagedPointsTo.ql @@ -3,11 +3,9 @@ * @description Query to force evaluation of staged points-to predicates * @kind table * @id cpp/points-to/prepared-staged-points-to - * @deprecated + * @deprecated This query is not suitable for production use and has been deprecated. */ -// This query is not suitable for production use and has been deprecated. - import semmle.code.cpp.pointsto.PointsTo select count(int set, Element location | setlocations(set, unresolveElement(location))), diff --git a/cpp/ql/src/PointsTo/Stats.ql b/cpp/ql/src/PointsTo/Stats.ql index a982e0807da9..a57a0f398322 100644 --- a/cpp/ql/src/PointsTo/Stats.ql +++ b/cpp/ql/src/PointsTo/Stats.ql @@ -3,11 +3,9 @@ * @description Count the number points-to sets with 0 or 1 incoming flow edges, and the total number of points-to sets * @kind table * @id cpp/points-to/stats - * @deprecated + * @deprecated This query is not suitable for production use and has been deprecated. */ -// This query is not suitable for production use and has been deprecated. - import cpp import semmle.code.cpp.pointsto.PointsTo diff --git a/cpp/ql/src/PointsTo/TaintedFormatStrings.ql b/cpp/ql/src/PointsTo/TaintedFormatStrings.ql index 5a577f9a38bc..a681fc3e22f6 100644 --- a/cpp/ql/src/PointsTo/TaintedFormatStrings.ql +++ b/cpp/ql/src/PointsTo/TaintedFormatStrings.ql @@ -2,11 +2,9 @@ * @name Taint test * @kind table * @id cpp/points-to/tainted-format-strings - * @deprecated + * @deprecated This query is not suitable for production use and has been deprecated. */ -// This query is not suitable for production use and has been deprecated. - import cpp import semmle.code.cpp.pointsto.PointsTo import semmle.code.cpp.pointsto.CallGraph