Merge pull request #254 from mbaluda-org/Contracts6

Contracts7

Author: mbaluda

c/cert/src/rules/MSC33-C/DoNotPassInvalidDataToTheAsctimeFunction.md

@@ -0,0 +1,133 @@
+# MSC33-C: Do not pass invalid data to the asctime() function
+
+This query implements the CERT-C rule MSC33-C:
+
+> Do not pass invalid data to the asctime() function
+
+
+## Description
+
+The C Standard, 7.27.3.1 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], provides the following sample implementation of the `asctime()` function:
+
+```cpp
+char *asctime(const struct tm *timeptr) {
+  static const char wday_name[7][3] = {
+    "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"
+  };
+  static const char mon_name[12][3] = {
+    "Jan", "Feb", "Mar", "Apr", "May", "Jun",
+    "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"
+  };
+  static char result[26];
+  sprintf(
+    result, 
+    "%.3s %.3s%3d %.2d:%.2d:%.2d %d\n",
+    wday_name[timeptr->tm_wday],
+    mon_name[timeptr->tm_mon],
+    timeptr->tm_mday, timeptr->tm_hour,
+    timeptr->tm_min, timeptr->tm_sec,
+    1900 + timeptr->tm_year
+  );
+  return result;
+}
+
+```
+This function is supposed to output a character string of 26 characters at most, including the terminating null character. If we count the length indicated by the format directives, we arrive at 25. Taking into account the terminating null character, the array size of the string appears sufficient.
+
+However, this [implementation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation) assumes that the values of the `struct tm` data are within normal ranges and does nothing to enforce the range limit. If any of the values print more characters than expected, the `sprintf()` function may overflow the `result` array. For example, if `tm_year` has the value `12345,` then 27 characters (including the terminating null character) are printed, resulting in a buffer overflow.
+
+The* POSIX<sup>®</sup> Base Specifications* \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\] says the following about the `asctime()` and `asctime_r()` functions:
+
+> These functions are included only for compatibility with older implementations. They have undefined behavior if the resulting string would be too long, so the use of these functions should be discouraged. On implementations that do not detect output string length overflow, it is possible to overflow the output buffers in such a way as to cause applications to fail, or possible system security violations. Also, these functions do not support localized date and time formats. To avoid these problems, applications should use `strftime()` to generate strings from broken-down times.
+
+
+The C Standard, Annex K, also defines `asctime_s()`, which can be used as a secure substitute for `asctime()`.
+
+The `asctime()` function appears in the list of obsolescent functions in [MSC24-C. Do not use deprecated or obsolescent functions](https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions).
+
+## Noncompliant Code Example
+
+This noncompliant code example invokes the `asctime()` function with potentially unsanitized data:
+
+```cpp
+#include <time.h>
+ 
+void func(struct tm *time_tm) {
+  char *time = asctime(time_tm);
+  /* ... */
+}
+```
+
+## Compliant Solution (strftime())
+
+The `strftime()` function allows the programmer to specify a more rigorous format and also to specify the maximum size of the resulting time string:
+
+```cpp
+#include <time.h>
+
+enum { maxsize = 26 };
+ 
+void func(struct tm *time) {
+  char s[maxsize];
+  /* Current time representation for locale */
+  const char *format = "%c";
+
+  size_t size = strftime(s, maxsize, format, time);
+}
+```
+This call has the same effects as `asctime()` but also ensures that no more than `maxsize` characters are printed, preventing buffer overflow.
+
+## Compliant Solution (asctime_s())
+
+The C Standard, Annex K, defines the `asctime_s()` function, which serves as a close replacement for the `asctime()` function but requires an additional argument that specifies the maximum size of the resulting time string:
+
+```cpp
+#define __STDC_WANT_LIB_EXT1__ 1
+#include <time.h>
+
+enum { maxsize = 26 };
+  
+void func(struct tm *time_tm) {
+  char buffer[maxsize];
+ 
+  if (asctime_s(buffer, maxsize, &time_tm)) {
+    /* Handle error */
+  }
+}
+```
+
+## Risk Assessment
+
+On [implementations](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation) that do not detect output-string-length overflow, it is possible to overflow the output buffers.
+
+<table> <tbody> <tr> <th> Rule </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> MSC33-C </td> <td> High </td> <td> Likely </td> <td> Low </td> <td> <strong>P27</strong> </td> <td> <strong>L1</strong> </td> </tr> </tbody> </table>
+
+
+## Automated Detection
+
+<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> Astrée </a> </td> <td> 22.04 </td> <td> </td> <td> Supported, but no explicit checker </td> </tr> <tr> <td> <a> Axivion Bauhaus Suite </a> </td> <td> 7.2.0 </td> <td> <strong>CertC-MSC33</strong> </td> <td> </td> </tr> <tr> <td> <a> CodeSonar </a> </td> <td> 7.2p0 </td> <td> <strong>BADFUNC.TIME_H</strong> </td> <td> Use of &lt;time.h&gt; Time/Date Function </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2022.4 </td> <td> <strong>C5032</strong> <strong>C++5030</strong> </td> <td> </td> </tr> <tr> <td> <a> Klocwork </a> </td> <td> 2022.4 </td> <td> <strong>CERT.MSC.ASCTIME</strong> </td> <td> </td> </tr> <tr> <td> <a> LDRA tool suite </a> </td> <td> 9.7.1 </td> <td> <strong>44 S</strong> </td> <td> Enhanced Enforcement </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2022.2 </td> <td> <strong>CERT_C-MSC33-a</strong> </td> <td> The 'asctime()' and 'asctime_r()' functions should not be used </td> </tr> <tr> <td> <a> PC-lint Plus </a> </td> <td> 1.4 </td> <td> <strong>586</strong> </td> <td> Fully supported </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> R2022b </td> <td> <a> CERT C: Rule MSC33-C </a> </td> <td> Checks for use of obsolete standard function (rule fully covered) </td> </tr> <tr> <td> <a> PRQA QA-C </a> </td> <td> 9.7 </td> <td> <strong>5032 </strong> </td> <td> </td> </tr> <tr> <td> <a> PRQA QA-C++ </a> </td> <td> 4.4 </td> <td> <strong>5030</strong> </td> <td> </td> </tr> <tr> <td> <a> RuleChecker </a> </td> <td> 22.04 </td> <td> </td> <td> Supported, but no explicit checker </td> </tr> </tbody> </table>
+
+
+## Related Vulnerabilities
+
+Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+MSC33-C).
+
+## Related Guidelines
+
+[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions)
+
+<table> <tbody> <tr> <th> Taxonomy </th> <th> Taxonomy item </th> <th> Relationship </th> </tr> <tr> <td> <a> CERT C Secure Coding Standard </a> </td> <td> <a> MSC24-C. Do not use deprecated or obsolescent functions </a> </td> <td> Prior to 2018-01-12: CERT: Unspecified Relationship </td> </tr> </tbody> </table>
+
+
+## Bibliography
+
+<table> <tbody> <tr> <td> \[ <a> IEEE Std 1003.1:2013 </a> \] </td> <td> XSH, System Interfaces, <code>asctime</code> </td> </tr> <tr> <td> \[ <a> ISO/IEC 9899:2011 </a> \] </td> <td> 7.27.3.1, "The <code>asctime</code> Function" </td> </tr> </tbody> </table>
+
+
+## Implementation notes
+
+None
+
+## References
+
+* CERT-C: [MSC33-C: Do not pass invalid data to the asctime() function](https://wiki.sei.cmu.edu/confluence/display/c)

c/cert/src/rules/MSC33-C/DoNotPassInvalidDataToTheAsctimeFunction.ql

@@ -0,0 +1,51 @@
+/**
+ * @id c/cert/do-not-pass-invalid-data-to-the-asctime-function
+ * @name MSC33-C: Do not pass invalid data to the asctime() function
+ * @description The data passed to the asctime() function is invalid. This can lead to buffer
+ *              overflow.
+ * @kind problem
+ * @precision very-high
+ * @problem.severity error
+ * @tags external/cert/id/msc33-c
+ *       security
+ *       correctness
+ *       external/cert/obligation/rule
+ */
+
+import cpp
+import codingstandards.c.cert
+import semmle.code.cpp.dataflow.DataFlow
+
+/**
+ * The argument of a call to `asctime`
+ */
+class AsctimeArg extends Expr {
+  AsctimeArg() {
+    this =
+      any(FunctionCall f | f.getTarget().hasGlobalName(["asctime", "asctime_r"])).getArgument(0)
+  }
+}
+
+/**
+ * Dataflow configuration for flow from a library function
+ * to a call of function `asctime`
+ */
+class TmStructSafeConfig extends DataFlow::Configuration {
+  TmStructSafeConfig() { this = "TmStructSafeConfig" }
+
+  override predicate isSource(DataFlow::Node src) {
+    src.asExpr()
+        .(FunctionCall)
+        .getTarget()
+        .hasGlobalName(["localtime", "localtime_r", "localtime_s", "gmtime", "gmtime_r", "gmtime_s"])
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof AsctimeArg }
+}
+
+from AsctimeArg fc, TmStructSafeConfig config
+where
+  not isExcluded(fc, Contracts7Package::doNotPassInvalidDataToTheAsctimeFunctionQuery()) and
+  not config.hasFlowToExpr(fc)
+select fc,
+  "The function `asctime` and `asctime_r` should be discouraged. Unsanitized input can overflow the output buffer."

c/cert/src/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.md

@@ -0,0 +1,120 @@
+# MSC39-C: Do not call va_arg() on a va_list that has an indeterminate value
+
+This query implements the CERT-C rule MSC39-C:
+
+> Do not call va_arg() on a va_list that has an indeterminate value
+
+
+## Description
+
+Variadic functions access their variable arguments by using `va_start()` to initialize an object of type `va_list`, iteratively invoking the `va_arg()` macro, and finally calling `va_end()`. The `va_list` may be passed as an argument to another function, but calling `va_arg()` within that function causes the `va_list` to have an [indeterminate value](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-indeterminatevalue) in the calling function. As a result, attempting to read variable arguments without reinitializing the `va_list` can have [unexpected behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unexpectedbehavior). According to the C Standard, 7.16, paragraph 3 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\],
+
+> If access to the varying arguments is desired, the called function shall declare an object (generally referred to as `ap` in this subclause) having type `va_list`. The object `ap` may be passed as an argument to another function; if that function invokes the `va_arg` macro with parameter `ap`, the value of `ap` in the calling function is indeterminate and shall be passed to the `va_end` macro prior to any further reference to `ap`.<sup>253</sup>253) It is permitted to create a pointer to a `va_list` and pass that pointer to another function, in which case the original function may take further use of the original list after the other function returns.
+
+
+## Noncompliant Code Example
+
+This noncompliant code example attempts to check that none of its variable arguments are zero by passing a `va_list` to helper function `contains_zero()`. After the call to `contains_zero()`, the value of `ap` is [indeterminate](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-indeterminatevalue).
+
+```cpp
+#include <stdarg.h>
+#include <stdio.h>
+ 
+int contains_zero(size_t count, va_list ap) {
+  for (size_t i = 1; i < count; ++i) {
+    if (va_arg(ap, double) == 0.0) {
+      return 1;
+    }
+  }
+  return 0;
+}
+
+int print_reciprocals(size_t count, ...) {
+  va_list ap;  
+  va_start(ap, count);
+
+  if (contains_zero(count, ap)) {
+    va_end(ap);
+    return 1;
+  }
+
+  for (size_t i = 0; i < count; ++i) {
+    printf("%f ", 1.0 / va_arg(ap, double));
+  }
+
+  va_end(ap);
+  return 0;
+}
+
+```
+
+## Compliant Solution
+
+The compliant solution modifies `contains_zero()` to take a pointer to a `va_list`. It then uses the `va_copy` macro to make a copy of the list, traverses the copy, and cleans it up. Consequently, the `print_reciprocals()` function is free to traverse the original `va_list`.
+
+```cpp
+#include <stdarg.h>
+#include <stdio.h>
+ 
+int contains_zero(size_t count, va_list *ap) {
+  va_list ap1;
+  va_copy(ap1, *ap);
+  for (size_t i = 1; i < count; ++i) {
+    if (va_arg(ap1, double) == 0.0) {
+      return 1;
+    }
+  }
+  va_end(ap1);
+  return 0;
+}
+ 
+int print_reciprocals(size_t count, ...) {
+  int status;
+  va_list ap;
+  va_start(ap, count);
+ 
+  if (contains_zero(count, &ap)) {
+    printf("0 in arguments!\n");
+    status = 1;
+  } else {
+    for (size_t i = 0; i < count; i++) {
+      printf("%f ", 1.0 / va_arg(ap, double));
+    }
+    printf("\n");
+    status = 0;
+  }
+ 
+  va_end(ap);
+  return status;
+}
+
+```
+
+## Risk Assessment
+
+Reading variable arguments using a `va_list` that has an [indeterminate value](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-indeterminatevalue) can have unexpected results.
+
+<table> <tbody> <tr> <th> Rule </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> MSC39-C </td> <td> Low </td> <td> Unlikely </td> <td> Low </td> <td> <strong>P3</strong> </td> <td> <strong>L3</strong> </td> </tr> </tbody> </table>
+
+
+## Automated Detection
+
+<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> CodeSonar </a> </td> <td> 7.2p0 </td> <td> <strong>BADMACRO.STDARG_H</strong> </td> <td> Use of &lt;stdarg.h&gt; Feature </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2022.4 </td> <td> <strong>C3497</strong> <strong>C++3146, C++3147, C++3148, C++3149, C++3167</strong> </td> <td> </td> </tr> <tr> <td> <a> Klocwork </a> </td> <td> 2022.4 </td> <td> <strong>VA.LIST.INDETERMINATE</strong> </td> <td> </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2022.2 </td> <td> <strong>CERT_C-MSC39-a</strong> </td> <td> Use macros for variable arguments correctly </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> R2022b </td> <td> <a> CERT C: Rule MSC39-C </a> </td> <td> Checks for: Invalid va_list argumentnvalid va_list argument, too many va_arg calls for current argument listoo many va_arg calls for current argument list. Rule partially covered. </td> </tr> <tr> <td> <a> PRQA QA-C </a> </td> <td> 9.7 </td> <td> <strong>3497</strong> </td> <td> Enforced by QAC </td> </tr> <tr> <td> <a> TrustInSoft Analyzer </a> </td> <td> 1.38 </td> <td> <strong>variadic</strong> </td> <td> Exhaustively verified. </td> </tr> </tbody> </table>
+
+
+## Related Vulnerabilities
+
+Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+MSC39-C).
+
+## Bibliography
+
+<table> <tbody> <tr> <td> \[ <a> ISO/IEC 9899:2011 </a> \] </td> <td> Subclause 7.16, "Variable Arguments <code>&lt;stdarg.h&gt;</code> " </td> </tr> </tbody> </table>
+
+
+## Implementation notes
+
+None
+
+## References
+
+* CERT-C: [MSC39-C: Do not call va_arg() on a va_list that has an indeterminate value](https://wiki.sei.cmu.edu/confluence/display/c)

c/cert/src/rules/MSC39-C/DoNotCallVaArgOnAVaListThatHasAnIndeterminateValue.ql

@@ -0,0 +1,82 @@
+/**
+ * @id c/cert/do-not-call-va-arg-on-a-va-list-that-has-an-indeterminate-value
+ * @name MSC39-C: Do not call va_arg() on a va_list that has an indeterminate value
+ * @description Do not call va_arg() on a va_list that has an indeterminate value.
+ * @kind problem
+ * @precision high
+ * @problem.severity error
+ * @tags external/cert/id/msc39-c
+ *       correctness
+ *       external/cert/obligation/rule
+ */
+
+import cpp
+import codingstandards.c.cert
+import codingstandards.cpp.Macro
+import semmle.code.cpp.dataflow.DataFlow
+
+abstract class VaAccess extends Expr { }
+
+/**
+ * The argument of a call to `va_arg`
+ */
+class VaArgArg extends VaAccess {
+  VaArgArg() { this = any(MacroInvocation m | m.getMacroName() = ["va_arg"]).getExpr().getChild(0) }
+}
+
+/**
+ * The argument of a call to `va_end`
+ */
+class VaEndArg extends VaAccess {
+  VaEndArg() { this = any(MacroInvocation m | m.getMacroName() = ["va_end"]).getExpr().getChild(0) }
+}
+
+/**
+ * Dataflow configuration for flow from a library function
+ * to a call of function `asctime`
+ */
+class VaArgConfig extends DataFlow::Configuration {
+  VaArgConfig() { this = "VaArgConfig" }
+
+  override predicate isSource(DataFlow::Node src) {
+    src.asUninitialized() =
+      any(VariableDeclarationEntry m | m.getType().hasName("va_list")).getVariable()
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof VaAccess }
+}
+
+/**
+ * Controlflow nodes preceeding a call to `va_arg`
+ */
+ControlFlowNode preceedsFC(VaAccess va_arg) {
+  result = va_arg
+  or
+  exists(ControlFlowNode mid |
+    result = mid.getAPredecessor() and
+    mid = preceedsFC(va_arg) and
+    // stop recursion on va_end on the same object
+    not result =
+      any(MacroInvocation m |
+        m.getMacroName() = ["va_start"] and
+        m.getExpr().getChild(0).(VariableAccess).getTarget() = va_arg.(VariableAccess).getTarget()
+      ).getExpr()
+  )
+}
+
+predicate sameSource(VaAccess e1, VaAccess e2) {
+  exists(VaArgConfig config, DataFlow::Node source |
+    config.hasFlow(source, DataFlow::exprNode(e1)) and
+    config.hasFlow(source, DataFlow::exprNode(e2))
+  )
+}
+
+from VaAccess va_acc, VaArgArg va_arg, FunctionCall fc
+where
+  not isExcluded(va_acc,
+    Contracts7Package::doNotCallVaArgOnAVaListThatHasAnIndeterminateValueQuery()) and
+  sameSource(va_acc, va_arg) and
+  fc = preceedsFC(va_acc) and
+  fc.getTarget().calls*(va_arg.getEnclosingFunction())
+select va_acc, "The value of " + va_acc.toString() + " is indeterminate after the $@.", fc,
+  fc.toString()

c/cert/test/rules/FIO42-C/test.c

@@ -82,7 +82,7 @@ int closing_helper(FILE *g) {
   return 0;
 }
 int f2inter(const char *filename) {
-  FILE *f = fopen(filename, "r"); // COMPLIANT (FALSE_POSITIVE)
+  FILE *f = fopen(filename, "r"); // COMPLIANT[FALSE_POSITIVE]
   if (NULL == f) {
     return -1;
   }