Merge pull request #1209 from geoffw0/gmtime

CPP: Add variants to PotentiallyDangerousFunction.ql

Author: jbj

change-notes/1.21/analysis-cpp.md

@@ -18,6 +18,7 @@
 | Memory is never freed (`cpp/memory-never-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
 | Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Resource allocation and deallocation functions are now determined more accurately. |
 | Comparison result is always the same | Fewer false positive results | The range analysis library is now more conservative about floating point values being possibly `NaN` |
+| Use of potentially dangerous function | More correct results | Calls to `localtime`, `ctime` and `asctime` are now detected by this query. |
 | 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. |
 

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

@@ -5,13 +5,15 @@
 <overview>
 <p>This rule finds calls to functions that are dangerous to
 use. Currently, it checks for calls
-to <code>gets</code> and <code>gmtime</code>. See <strong>Related rules</strong>
-below for rules that identify other dangerous functions.</p>
+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>
 
-<p>The <code>gmtime</code> function fills data into a <code>tm</code>
-struct in shared memory and then returns a pointer to that struct. If
+<p>The time related functions such as <code>gmtime</code>
+fill data into a <code>tm</code> struct or <code>char</code> array in
+shared memory and then returns a pointer to that memory. If
 the function is called from multiple places in the same program, and
 especially if it is called from multiple threads in the same program,
 then the calls will overwrite each other's data.</p>
@@ -26,6 +28,11 @@ With <code>gmtime_r</code>, the application code manages allocation of
 the <code>tm</code> struct. That way, separate calls to the function
 can use their own storage.</p>
 
+<p>Similarly replace calls to <code>localtime</code> with
+<code>localtime_r</code>, calls to <code>ctime</code> with
+<code>ctime_r</code> and calls to <code>asctime</code> with
+<code>asctime_r</code>.</p>
+
 </recommendation>
 <example>
 <p>The following example checks the local time in two ways:</p>

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

@@ -12,9 +12,14 @@
 import cpp
 
 predicate potentiallyDangerousFunction(Function f, string message) {
-  (
-    f.getQualifiedName() = "gmtime" and
-    message = "Call to gmtime is potentially dangerous"
+  exists(string name | name = f.getQualifiedName() |
+    (
+      name = "gmtime" or
+      name = "localtime" or
+      name = "ctime" or
+      name = "asctime"
+    ) and
+    message = "Call to " + name + " is potentially dangerous"
   ) or (
     f.getQualifiedName() = "gets" and
     message = "gets does not guard against buffer overflow"

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

@@ -1,3 +1,6 @@
-| test.c:28:22:28:27 | call to gmtime | Call to gmtime is potentially dangerous |
-| test.c:39:2:39:5 | call to gets | gets does not guard against buffer overflow |
-| test.c:40:6:40:9 | call to gets | gets does not guard against buffer overflow |
+| 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 |

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

@@ -21,6 +21,9 @@ struct tm {
 
 struct tm *gmtime(const time_t *timer);
 time_t time(time_t *timer);
+struct tm *localtime(const time_t *timer);
+char *ctime(const time_t *timer);
+char *asctime(const struct tm *timeptr);
 
 // Code under test
 
@@ -39,3 +42,10 @@ void testGets() {
 	gets(buf1); // BAD: use of gets
 	s = gets(buf2); // BAD: use of gets
 }
+
+void testTime()
+{
+	struct tm *now = localtime(time(NULL)); // BAD: localtime uses shared state
+	char *time_string = ctime(time(NULL)); // BAD: localtime uses shared state
+	char *time_string2 = asctime(now); // BAD: localtime uses shared state
+}