Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
4ca36af
Package IO4
mbaluda Aug 5, 2022
05f017c
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 9, 2022
70c643a
Rule FIO45-C
mbaluda Aug 9, 2022
0b5ec10
Rule FIO45-C
mbaluda Aug 9, 2022
b992077
Rule FIO47-C
mbaluda Aug 10, 2022
0ba98f4
Rule FIO47-C
mbaluda Aug 10, 2022
23b72f7
FIO47-C fix literal number mismatch.
mbaluda Aug 10, 2022
716c7a1
FIO47-C fix check/cross symbols in markdown.
mbaluda Aug 10, 2022
8f28a88
Rule ENV30-C and ENV31-C.
mbaluda Aug 11, 2022
76f3ba7
Updated rules.csv
mbaluda Aug 11, 2022
a931a68
Update rules.csv
mbaluda Aug 11, 2022
ab9d84d
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 11, 2022
a2cb74f
Format ENV31-C test case
mbaluda Aug 11, 2022
dcf239e
Update expected file for ENV31-C
mbaluda Aug 11, 2022
188fd30
Remove DF barriers
mbaluda Aug 11, 2022
787fb4c
consider increment operator
mbaluda Aug 11, 2022
cb469e6
Simplified ENV30-C
mbaluda Aug 11, 2022
7748bdf
Add a test for ENV30-C
mbaluda Aug 11, 2022
d74e6bd
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 12, 2022
b285364
IO4
mbaluda Aug 12, 2022
2ff8966
Merge branch 'mbaluda/IO4' of https://github.com/mbaluda-org/codeql-c…
mbaluda Aug 12, 2022
5a2a4d8
IO4
mbaluda Aug 12, 2022
87e606f
IO4
mbaluda Aug 12, 2022
4e32001
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 12, 2022
a790db4
Fixes typos in test
mbaluda Aug 15, 2022
46a2d2c
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 15, 2022
9ae51c7
Removed unneded implementation_scope
mbaluda Aug 15, 2022
602fa12
Fix implementation_scope
mbaluda Aug 16, 2022
b7a6e79
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 17, 2022
1bac284
Add comment on stdlib version of the query
mbaluda Aug 17, 2022
57b17fc
Update IO4.json
mbaluda Aug 17, 2022
6796f6f
Fix rule description in ql files
mbaluda Aug 17, 2022
4781c15
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 22, 2022
93a309e
Review fixes
mbaluda Aug 23, 2022
68217c7
Fixes for review
mbaluda Aug 23, 2022
c96f4c1
typo
mbaluda Aug 23, 2022
ce83546
Fix expected files
mbaluda Aug 23, 2022
5b2a405
Merge branch 'main' into mbaluda/IO4
mbaluda Aug 23, 2022
f82206d
use fcloseCall intead of closed that is overapproximated
mbaluda Aug 23, 2022
8c452d6
Improved FIO45-C implementation scope
mbaluda Aug 24, 2022
4a35ca3
Merge branch 'mbaluda/IO4' of https://github.com/mbaluda-org/codeql-c…
mbaluda Aug 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@
"IO1",
"IO2",
"IO3",
"IO4",
"Includes",
"Initialization",
"IntegerConversion",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
# ENV30-C: Do not modify the return value of certain functions

This query implements the CERT-C rule ENV30-C:

> Do not modify the object referenced by the return value of certain functions


## Description

Some functions return a pointer to an object that cannot be modified without causing [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). These functions include `getenv()`, `setlocale()`, `localeconv()`, `asctime()`, and `strerror()`. In such cases, the function call results must be treated as being `const`-qualified.

The C Standard, 7.22.4.6, paragraph 4 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], defines `getenv()` as follows:

> The `getenv` function returns a pointer to a string associated with the matched list member. The string pointed to shall not be modified by the program, but may be overwritten by a subsequent call to the `getenv` function. If the specified name cannot be found, a null pointer is returned.


If the string returned by `getenv()` must be altered, a local copy should be created. Altering the string returned by `getenv()` is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behavior 184](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_184).)

Similarly, subclause 7.11.1.1, paragraph 8 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], defines `setlocale()` as follows:

> The pointer to string returned by the `setlocale` function is such that a subsequent call with that string value and its associated category will restore that part of the program'€™s locale. The string pointed to shall not be modified by the program, but may be overwritten by a subsequent call to the `setlocale` function.


And subclause 7.11.2.1, paragraph 8 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], defines `localeconv()` as follows:

> The `localeconv` function returns a pointer to the filled-in object. The structure pointed to by the return value shall not be modified by the program, but may be overwritten by a subsequent call to the `localeconv` function. In addition, calls to the `setlocale` function with categories `LC_ALL`, `LC_MONETARY`, or `LC_NUMERIC` may overwrite the contents of the structure.


Altering the string returned by `setlocale()` or the structure returned by `localeconv()` are [undefined behaviors](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behaviors 120](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_120) and [121](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_121).) Furthermore, the C Standard imposes no requirements on the contents of the string by `setlocale()`. Consequently, no assumptions can be made as to the string's internal contents or structure.

Finally, subclause 7.24.6.2, paragraph 4 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states

> The `strerror` function returns a pointer to the string, the contents of which are locale-specific. The array pointed to shall not be modified by the program, but may be overwritten by a subsequent call to the `strerror` function.


Altering the string returned by `strerror()` is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behavior 184](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_184).)

## Noncompliant Code Example (getenv())

This noncompliant code example modifies the string returned by `getenv()` by replacing all double quotation marks (`"`) with underscores (`_`):

```cpp
#include <stdlib.h>

void trstr(char *c_str, char orig, char rep) {
while (*c_str != '\0') {
if (*c_str == orig) {
*c_str = rep;
}
++c_str;
}
}

void func(void) {
char *env = getenv("TEST_ENV");
if (env == NULL) {
/* Handle error */
}
trstr(env,'"', '_');
}

```

## Compliant Solution (getenv()) (Environment Not Modified)

If the programmer does not intend to modify the environment, this compliant solution demonstrates how to modify a copy of the return value:

```cpp
#include <stdlib.h>
#include <string.h>

void trstr(char *c_str, char orig, char rep) {
while (*c_str != '\0') {
if (*c_str == orig) {
*c_str = rep;
}
++c_str;
}
}

void func(void) {
const char *env;
char *copy_of_env;

env = getenv("TEST_ENV");
if (env == NULL) {
/* Handle error */
}

copy_of_env = (char *)malloc(strlen(env) + 1);
if (copy_of_env == NULL) {
/* Handle error */
}

strcpy(copy_of_env, env);
trstr(copy_of_env,'"', '_');
/* ... */
free(copy_of_env);
}
```

## Compliant Solution (getenv()) (Modifying the Environment in POSIX)

If the programmer's intent is to modify the environment, this compliant solution, which saves the altered string back into the environment by using the POSIX `setenv()` and `strdup()` functions, can be used:

```cpp
#include <stdlib.h>
#include <string.h>

void trstr(char *c_str, char orig, char rep) {
while (*c_str != '\0') {
if (*c_str == orig) {
*c_str = rep;
}
++c_str;
}
}

void func(void) {
const char *env;
char *copy_of_env;

env = getenv("TEST_ENV");
if (env == NULL) {
/* Handle error */
}

copy_of_env = strdup(env);
if (copy_of_env == NULL) {
/* Handle error */
}

trstr(copy_of_env,'"', '_');

if (setenv("TEST_ENV", copy_of_env, 1) != 0) {
/* Handle error */
}
/* ... */
free(copy_of_env);
}
```

## Noncompliant Code Example (localeconv())

In this noncompliant example, the object returned by `localeconv()` is directly modified:

```cpp
#include <locale.h>

void f2(void) {
struct lconv *conv = localeconv();

if ('\0' == conv->decimal_point[0]) {
conv->decimal_point = ".";
}
}

```

## Compliant Solution (localeconv()) (Copy)

This compliant solution modifies a copy of the object returned by `localeconv()`:

```cpp
#include <locale.h>
#include <stdlib.h>
#include <string.h>

void f2(void) {
const struct lconv *conv = localeconv();
if (conv == NULL) {
/* Handle error */
}

struct lconv *copy_of_conv = (struct lconv *)malloc(
sizeof(struct lconv));
if (copy_of_conv == NULL) {
/* Handle error */
}

memcpy(copy_of_conv, conv, sizeof(struct lconv));

if ('\0' == copy_of_conv->decimal_point[0]) {
copy_of_conv->decimal_point = ".";
}
/* ... */
free(copy_of_conv);
}
```

## Risk Assessment

Modifying the object pointed to by the return value of `getenv()`, `setlocale()`, `localeconv()`, `asctime()`, or `strerror()` is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). Even if the modification succeeds, the modified object can be overwritten by a subsequent call to the same function.

<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> ENV30-C </td> <td> Low </td> <td> Probable </td> <td> Medium </td> <td> <strong>P4</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> Astrée </a> </td> <td> 22.04 </td> <td> <strong>stdlib-const-pointer-assign</strong> </td> <td> Partially checked </td> </tr> <tr> <td> <a> Axivion Bauhaus Suite </a> </td> <td> 7.2.0 </td> <td> <strong>CertC-ENV30</strong> </td> <td> </td> </tr> <tr> <td> <a> CodeSonar </a> </td> <td> 7.0p0 </td> <td> <strong>BADFUNC.GETENV</strong> </td> <td> Use of getenv </td> </tr> <tr> <td> <a> Compass/ROSE </a> </td> <td> </td> <td> </td> <td> Can detect violations of this rule. In particular, it ensures that the result of <code>getenv()</code> is stored in a <code>const</code> variable </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2022.2 </td> <td> <strong>C1492, C1493, C1494, C4751, C4752, C4753</strong> <strong>C++4751, C++4752, C++4753</strong> </td> <td> </td> </tr> <tr> <td> <a> Klocwork </a> </td> <td> 2022.2 </td> <td> <strong>MISRA.STDLIB.CTYPE.RANGE.2012_AMD1</strong> <strong>MISRA.STDLIB.ILLEGAL_REUSE.2012_AMD1</strong> <strong>MISRA.STDLIB.ILLEGAL_WRITE.2012_AMD1</strong> </td> <td> </td> </tr> <tr> <td> <a> LDRA tool suite </a> </td> <td> 9.7.1 </td> <td> <strong>107 D</strong> </td> <td> Partially Implemented </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2022.1 </td> <td> <strong>CERT_C-ENV30-a</strong> </td> <td> The pointers returned by the Standard Library functions 'localeconv', 'getenv', 'setlocale' or, 'strerror' shall only be used as if they have pointer to const-qualified type </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> R2022a </td> <td> <a> CERT C: Rule ENV30-C </a> </td> <td> Checks for modification of internal buffer returned from nonreentrant standard function (rule fully covered) </td> </tr> <tr> <td> <a> PRQA QA-C </a> </td> <td> 9.7 </td> <td> <strong>1492, 1493, 1494 </strong> </td> <td> </td> </tr> <tr> <td> <a> PVS-Studio </a> </td> <td> 7.20 </td> <td> <strong><a>V675</a></strong> </td> <td> </td> </tr> <tr> <td> <a> RuleChecker </a> </td> <td> 22.04 </td> <td> <strong>stdlib-const-pointer-assign</strong> </td> <td> Partially checked </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+ENV30-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> ISO/IEC TS 17961:2013 </a> </td> <td> Modifying the string returned by <code>getenv</code> , <code>localeconv</code> , <code>setlocale</code> , and <code>strerror</code> \[libmod\] </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>getenv</code> XSH, System Interfaces, <code>setlocale</code> XSH, System Interfaces, <code>localeconv</code> </td> </tr> <tr> <td> \[ <a> ISO/IEC 9899:2011 </a> \] </td> <td> 7.11.1.1, "The <code>setlocale</code> Function" 7.11.2.1, "The <code>localeconv</code> Function" 7.22.4.6, "The <code>getenv</code> Function" 7.24.6.2, "The <code>strerror</code> Function" </td> </tr> </tbody> </table>


## Implementation notes

None

## References

* CERT-C: [ENV30-C: Do not modify the object referenced by the return value of certain functions](https://wiki.sei.cmu.edu/confluence/display/c)
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
* @id c/cert/do-not-modify-the-return-value-of-certain-functions
* @name ENV30-C: Do not modify the return value of certain functions
* @description Return value of getenv and similar functions cannot be modified.
* @kind path-problem
* @precision very-high
* @problem.severity error
* @tags external/cert/id/env30-c
* correctness
* external/cert/obligation/rule
*/

import cpp
import codingstandards.c.cert
import semmle.code.cpp.dataflow.DataFlow
import DataFlow::PathGraph

/*
* Call to functions that return pointers to environment objects that should not be modified.
*/

class NotModifiableCall extends FunctionCall {
NotModifiableCall() {
this.getTarget()
.hasGlobalOrStdName(["getenv", "setlocale", "localeconv", "asctime", "strerror"])
Comment thread
knewbury01 marked this conversation as resolved.
Outdated
}
}

/*
* An expression that modifies an object.
*/

class ObjectWrite extends Expr {
Comment thread
knewbury01 marked this conversation as resolved.
ObjectWrite() {
// the pointed object is reassigned
exists(Expr e |
e = [any(AssignExpr ae).getLValue(), any(CrementOperation co).getOperand()] and
(
this = e.(PointerDereferenceExpr).getOperand()
or
this = e.(PointerFieldAccess).getQualifier()
)
)
}
}

/**
* DF configuration for flows from a `NotModifiableCall` to a object modifications.
*/
class DFConf extends DataFlow::Configuration {
Comment thread
knewbury01 marked this conversation as resolved.
DFConf() { this = "DFConf" }

override predicate isSource(DataFlow::Node source) {
source.asExpr() instanceof NotModifiableCall
}

override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ObjectWrite }
}

from DataFlow::PathNode source, DataFlow::PathNode sink
where
not isExcluded(sink.getNode().asExpr(),
Contracts1Package::doNotModifyTheReturnValueOfCertainFunctionsQuery()) and
// the modified object comes from a call to one of the ENV functions
any(DFConf d).hasFlowPath(source, sink)
select sink.getNode(), source, sink,
"The object returned by the function " +
source.getNode().asExpr().(FunctionCall).getTarget().getName() + " should no be modified."
Loading