Merge pull request #923 from geoffw0/potentialbufferoverflow

jbj · web-flow · commit c49c23068a0c · 2019-03-04T08:11:27.000Z
CPP: Deprecate PotentialBufferOverflow.ql
diff --git 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
 
diff --git a/cpp/config/suites/security/cwe-242 b/cpp/config/suites/security/cwe-242
diff --git 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"
diff --git a/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql b/cpp/ql/src/Likely Bugs/Memory Management/PotentialBufferOverflow.ql
@@ -9,6 +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.
  */
 import cpp
 import semmle.code.cpp.commons.Buffer
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
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
@@ -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
@@ -0,0 +1 @@
+Security/CWE/CWE-120/OverrunWrite.ql
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
@@ -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
@@ -0,0 +1 @@
+Security/CWE/CWE-120/OverrunWriteFloat.ql
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
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
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
@@ -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]
+}
