CPP: Split PotentiallyDangerousFunction.ql

change-notes/1.21/analysis-cpp.md

@@ -9,6 +9,7 @@
 | `()`-declared function called with too few arguments (`cpp/too-few-arguments`) | Correctness | Find all cases where the number of arguments is less than the number of parameters of the function, provided the function is also properly declared/defined elsewhere. |
 | `()`-declared function called with mismatched arguments (`cpp/mismatched-function-arguments`) | Correctness | Find all cases where the types of arguments do not match the types of parameters of the function, provided the function is also properly declared/defined elsewhere. |
 | Call to alloca in a loop (`cpp/alloca-in-loop`) | reliability, correctness, external/cwe/cwe-770 | Finds calls to `alloca` in loops, which can lead to stack overflow if the number of iterations is large.  Newly displayed on LGTM. |
+| Use of dangerous function (`dangerous-function-overflow`) | reliability, security, external/cwe/cwe-242 | Finds calls to `gets`, which does not guard against buffer overflow.  These results were previously detected by the `cpp/potentially-dangerous-function` query. |
 
 ## Changes to existing queries
 
@@ -28,6 +29,7 @@
 | Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | More correct results and fewer false positive results | This query now more accurately identifies wide and non-wide string/character format arguments on different platforms.  Platform detection has also been made more accurate for the purposes of this query. |
 | Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | Fewer false positive results | Non-standard uses of %L are now understood. |
 | `()`-declared function called with too many arguments (`cpp/futile-params`) | Improved coverage | Query has been generalized to find all cases where the number of arguments exceedes the number of parameters of the function, provided the function is also properly declared/defined elsewhere. |
+| Use of potentially dangerous function (`cpp/potentially-dangerous-function`) | Fewer results | Results relating to the standard library `gets` function have been moved into a new query (`dangerous-function-overflow`). |
 
 ## Changes to QL libraries
 - The predicate `Declaration.hasGlobalName` now only holds for declarations that are not nested in a class. For example, it no longer holds for a member function `MyClass::myFunction` or a constructor `MyClass::MyClass`, whereas previously it would classify those two declarations as global names.

cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.c

@@ -0,0 +1,15 @@
+#define BUFFERSIZE (1024)
+
+// BAD: using gets
+void echo_bad() {
+    char buffer[BUFFERSIZE];
+    gets(buffer);
+    printf("Input was: '%s'\n", buffer);
+}
+
+// GOOD: using fgets
+void echo_good() {
+    char buffer[BUFFERSIZE];
+    fgets(buffer, BUFFERSIZE, stdin);
+    printf("Input was: '%s'\n", buffer);
+}

cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>This rule finds calls to the <code>gets</code> function, which is dangerous and
+should not be used. See <strong>Related
+rules</strong> below for rules that identify other dangerous functions.</p>
+
+<p>The <code>gets</code> function is one of the vulnerabilities exploited by the Internet Worm of 1988, one of the first computer worms to spread through the Internet.  The <code>gets</code> function provides no way to limit the amount of data that is read and stored, so without prior knowledge of the input it is impossible to use it safely with any size of buffer.</p>
+
+</overview>
+<recommendation>
+
+<p>Replace calls to <code>gets</code> with <code>fgets</code>, specifying the maximum length to copy.  This will prevent the buffer overflow.</p>
+
+</recommendation>
+<example>
+<p>The following example gets a string from standard input in two ways:</p>
+<sample src="DangerousUseOfGets" />
+
+<p>The first version uses <code>gets</code> and will overflow if the input
+is longer than the buffer. The second version of the code
+uses <code>fgets</code> and will not overflow, because the amount of data
+written is limited by the length parameter.</p>
+</example>
+<section title="Related rules">
+<p>Other dangerous functions identified by CWE-676 ("Use of
+Potentially Dangerous Function") include <code>strcpy</code>
+and <code>strcat</code>. Use of these functions is highlighted by
+rules for the following CWEs:</p>
+<ul>
+<li><a href="https://cwe.mitre.org/data/definitions/120.html">CWE-120 Classic Buffer Overflow</a>.
+</li><li><a href="https://cwe.mitre.org/data/definitions/131.html">CWE-131 Incorrect Calculation of Buffer Size</a>.
+</li></ul>
+
+</section>
+<references>
+<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Morris_worm">Morris worm</a>.</li>
+<li>E. Spafford. <i>The Internet Worm Program: An Analysis</i>. Purdue Technical Report CSD-TR-823, <a href="http://www.textfiles.com/100/tr823.txt">(online)</a>, 1988.</li>
+</references>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-676/DangerousFunctionOverflow.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Use of dangerous function
+ * @description Use of a standard library function that does not guard against buffer overflow.
+ * @kind problem
+ * @problem.severity error
+ * @precision very-high
+ * @id cpp/dangerous-function-overflow
+ * @tags reliability
+ *       security
+ *       external/cwe/cwe-242
+ */
+import cpp
+
+from FunctionCall call, Function target
+where
+  call.getTarget() = target and
+  target.hasGlobalName("gets")
+select call, "gets does not guard against buffer overflow"

cpp/ql/src/Security/CWE/CWE-676/PotentiallyDangerousFunction.qhelp

@@ -5,11 +5,8 @@
 <overview>
 <p>This rule finds calls to functions that are dangerous to
 use. Currently, it checks for calls
-to <code>gets</code>, <code>gmtime</code>, <code>localtime</code>,
-<code>ctime</code> and <code>asctime</code>. See <strong>Related
-rules</strong> below for rules that identify other dangerous functions.</p>
-
-<p>The <code>gets</code> function is one of the vulnerabilities exploited by the Internet Worm of 1988, one of the first computer worms to spread through the Internet.  The <code>gets</code> function provides no way to limit the amount of data that is read and stored, so without prior knowledge of the input it is impossible to use it safely with any size of buffer.</p>
+to <code>gmtime</code>, <code>localtime</code>,
+<code>ctime</code> and <code>asctime</code>.
 
 <p>The time related functions such as <code>gmtime</code>
 fill data into a <code>tm</code> struct or <code>char</code> array in
@@ -21,8 +18,6 @@ then the calls will overwrite each other's data.</p>
 </overview>
 <recommendation>
 
-<p>Replace calls to <code>gets</code> with <code>fgets</code>, specifying the maximum length to copy.  This will prevent the buffer overflow.</p>
-
 <p>Replace calls to <code>gmtime</code> with <code>gmtime_r</code>.
 With <code>gmtime_r</code>, the application code manages allocation of
 the <code>tm</code> struct. That way, separate calls to the function
@@ -47,20 +42,7 @@ struct on every call, it is immune to other calls to <code>gmtime</code>
 or <code>gmtime_r</code>.</p>
 
 </example>
-<section title="Related rules">
-<p>Other dangerous functions identified by CWE-676 ("Use of
-Potentially Dangerous Function") include <code>strcpy</code>
-and <code>strcat</code>. Use of these functions is highlighted by
-rules for the following CWEs:</p>
-<ul>
-<li>CWE-120 Classic Buffer Overflow
-</li><li>CWE-131 Incorrect Calculation of Buffer Size
-</li></ul>
-
-</section>
 <references>
-<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Morris_worm">Morris worm</a>.</li>
-<li>E. Spafford. <i>The Internet Worm Program: An Analysis</i>. Purdue Technical Report CSD-TR-823, <a href="http://www.textfiles.com/100/tr823.txt">(online)</a>, 1988.</li>
 <li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/CON33-C.+Avoid+race+conditions+when+using+library+functions">CON33-C. Avoid race conditions when using library functions</a>.</li>
 </references>
 </qhelp>

cpp/ql/src/Security/CWE/CWE-676/PotentiallyDangerousFunction.ql

@@ -1,13 +1,13 @@
 /**
  * @name Use of potentially dangerous function
- * @description Certain standard library functions are dangerous to call.
+ * @description Use of a standard library function that is not thread-safe.
  * @kind problem
- * @problem.severity error
+ * @problem.severity warning
  * @precision high
  * @id cpp/potentially-dangerous-function
  * @tags reliability
  *       security
- *       external/cwe/cwe-242
+ *       external/cwe/cwe-676
  */
 import cpp
 
@@ -20,9 +20,6 @@ predicate potentiallyDangerousFunction(Function f, string message) {
       name = "asctime"
     ) and
     message = "Call to " + name + " is potentially dangerous"
-  ) or (
-    f.hasGlobalName("gets") and
-    message = "gets does not guard against buffer overflow"
   )
 }
 

cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/DangerousFunctionOverflow.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-676/DangerousFunctionOverflow.ql

cpp/ql/test/query-tests/Security/CWE/CWE-676/semmle/PotentiallyDangerousFunction/DangerousFunctionOverflow.expected

@@ -0,0 +1,2 @@
+| test.c:42:2:42:5 | call to gets | gets does not guard against buffer overflow |
+| test.c:43:6:43:9 | call to gets | gets does not guard against buffer overflow |

cpp/ql/test/query-tests/Security/CWE/CWE-676/semmle/PotentiallyDangerousFunction/DangerousFunctionOverflow.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-676/DangerousFunctionOverflow.ql

cpp/ql/test/query-tests/Security/CWE/CWE-676/semmle/PotentiallyDangerousFunction/PotentiallyDangerousFunction.expected

@@ -1,6 +1,4 @@
 | test.c:31:22:31:27 | call to gmtime | Call to gmtime is potentially dangerous |
-| test.c:42:2:42:5 | call to gets | gets does not guard against buffer overflow |
-| test.c:43:6:43:9 | call to gets | gets does not guard against buffer overflow |
 | test.c:48:19:48:27 | call to localtime | Call to localtime is potentially dangerous |
 | test.c:49:22:49:26 | call to ctime | Call to ctime is potentially dangerous |
 | test.c:50:23:50:29 | call to asctime | Call to asctime is potentially dangerous |