Skip to content

Commit 6153f50

Browse files
MichaelRFairhurstMichaelRFairhurst
committed
Implement expressions2 package
1 parent ed70a09 commit 6153f50

10 files changed

+432
-18
lines changed

‎c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.md

+129
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
# EXP16-C: Do not compare function pointers to constant values
2+
3+
This query implements the CERT-C rule EXP16-C:
4+
5+
> Do not compare function pointers to constant values
6+
7+
8+
## Description
9+
10+
Comparing a function pointer to a value that is not a null function pointer of the same type will be diagnosed because it typically indicates programmer error and can result in [unexpected behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unexpectedbehavior). Implicit comparisons will be diagnosed, as well.
11+
12+
## Noncompliant Code Example
13+
14+
In this noncompliant code example, the addresses of the POSIX functions `getuid` and `geteuid` are compared for equality to 0. Because no function address shall be null, the first subexpression will always evaluate to false (0), and the second subexpression always to true (nonzero). Consequently, the entire expression will always evaluate to true, leading to a potential security vulnerability.
15+
16+
```cpp
17+
/* First the options that are allowed only for root */
18+
if (getuid == 0 || geteuid != 0) {
19+
/* ... */
20+
}
21+
22+
```
23+
24+
## Noncompliant Code Example
25+
26+
In this noncompliant code example, the function pointers `getuid` and `geteuid` are compared to 0. This example is from an actual [vulnerability](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) ([VU\#837857](http://www.kb.cert.org/vuls/id/837857)) discovered in some versions of the X Window System server. The vulnerability exists because the programmer neglected to provide the open and close parentheses following the `geteuid()` function identifier. As a result, the `geteuid` token returns the address of the function, which is never equal to 0. Consequently, the `or` condition of this `if` statement is always true, and access is provided to the protected block for all users. Many compilers issue a warning noting such pointless expressions. Therefore, this coding error is normally detected by adherence to [MSC00-C. Compile cleanly at high warning levels](https://wiki.sei.cmu.edu/confluence/display/c/MSC00-C.+Compile+cleanly+at+high+warning+levels).
27+
28+
```cpp
29+
/* First the options that are allowed only for root */
30+
if (getuid() == 0 || geteuid != 0) {
31+
/* ... */
32+
}
33+
34+
```
35+
36+
## Compliant Solution
37+
38+
The solution is to provide the open and close parentheses following the `geteuid` token so that the function is properly invoked:
39+
40+
```cpp
41+
/* First the options that are allowed only for root */
42+
if (getuid() == 0 || geteuid() != 0) {
43+
/* ... */
44+
}
45+
46+
```
47+
48+
## Compliant Solution
49+
50+
A function pointer can be compared to a null function pointer of the same type:
51+
52+
```cpp
53+
/* First the options that are allowed only for root */
54+
if (getuid == (uid_t(*)(void))0 || geteuid != (uid_t(*)(void))0) {
55+
/* ... */
56+
}
57+
58+
```
59+
This code should not be diagnosed by an analyzer.
60+
61+
## Noncompliant Code Example
62+
63+
In this noncompliant code example, the function pointer `do_xyz` is implicitly compared unequal to 0:
64+
65+
```cpp
66+
int do_xyz(void);
67+
68+
int f(void) {
69+
/* ... */
70+
if (do_xyz) {
71+
return -1; /* Indicate failure */
72+
}
73+
/* ... */
74+
return 0;
75+
}
76+
77+
```
78+
79+
## Compliant Solution
80+
81+
In this compliant solution, the function `do_xyz()` is invoked and the return value is compared to 0:
82+
83+
```cpp
84+
int do_xyz(void);
85+
86+
int f(void) {
87+
/* ... */
88+
if (do_xyz()) {
89+
return -1; /* Indicate failure */
90+
}
91+
/* ... */
92+
return 0;
93+
}
94+
95+
```
96+
97+
## Risk Assessment
98+
99+
Errors of omission can result in unintended program flow.
100+
101+
<table> <tbody> <tr> <th> Recommendation </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> EXP16-C </td> <td> Low </td> <td> Likely </td> <td> Medium </td> <td> <strong>P6</strong> </td> <td> <strong>L2</strong> </td> </tr> </tbody> </table>
102+
103+
104+
## Automated Detection
105+
106+
<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> Astrée </a> </td> <td> 24.04 </td> <td> <strong>function-name-constant-comparison</strong> </td> <td> Partially checked </td> </tr> <tr> <td> <a> Coverity </a> </td> <td> 2017.07 </td> <td> <strong>BAD_COMPARE</strong> </td> <td> Can detect the specific instance where the address of a function is compared against 0, such as in the case of <code>geteuid</code> versus <code>getuid()</code> in the implementation-specific details </td> </tr> <tr> <td> <a> GCC </a> </td> <td> 4.3.5 </td> <td> </td> <td> Can detect violations of this recommendation when the <code>-Wall</code> flag is used </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2024.4 </td> <td> <strong>C0428, C3004, C3344</strong> </td> <td> </td> </tr> <tr> <td> <a> Klocwork </a> </td> <td> 2024.4 </td> <td> <strong>CWARN.NULLCHECK.FUNCNAMECWARN.FUNCADDR</strong> </td> <td> </td> </tr> <tr> <td> <a> LDRA tool suite </a> </td> <td> 9.7.1 </td> <td> <strong>99 S</strong> </td> <td> Partially implemented </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2024.2 </td> <td> <strong>CERT_C-EXP16-a</strong> </td> <td> Function address should not be compared to zero </td> </tr> <tr> <td> <a> PC-lint Plus </a> </td> <td> 1.4 </td> <td> <strong>2440, 2441</strong> </td> <td> Partially supported: reports address of function, array, or variable directly or indirectly compared to null </td> </tr> <tr> <td> <a> PVS-Studio </a> </td> <td> 7.35 </td> <td> <strong><a>V516</a>, <a>V1058</a></strong> </td> <td> </td> </tr> <tr> <td> <a> RuleChecker </a> </td> <td> 24.04 </td> <td> <strong>function-name-constant-comparison</strong> </td> <td> Partially checked </td> </tr> </tbody> </table>
107+
108+
109+
## Related Vulnerabilities
110+
111+
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+EXP16-C).
112+
113+
## Related Guidelines
114+
115+
<table> <tbody> <tr> <td> <a> SEI CERT C++ Coding Standard </a> </td> <td> <a> VOID EXP16-CPP. Avoid conversions using void pointers </a> </td> </tr> <tr> <td> <a> ISO/IEC TR 24772:2013 </a> </td> <td> Likely incorrect expressions \[KOA\] </td> </tr> <tr> <td> <a> ISO/IEC TS 17961 </a> </td> <td> Comparing function addresses to zero \[funcaddr\] </td> </tr> <tr> <td> <a> MITRE CWE </a> </td> <td> <a> CWE-480 </a> , Use of incorrect operator <a> CWE-482 </a> , Comparing instead of assigning </td> </tr> </tbody> </table>
116+
117+
118+
## Bibliography
119+
120+
<table> <tbody> <tr> <td> \[ <a> Hatton 1995 </a> \] </td> <td> Section 2.7.2, "Errors of Omission and Addition" </td> </tr> </tbody> </table>
121+
122+
123+
## Implementation notes
124+
125+
None
126+
127+
## References
128+
129+
* CERT-C: [EXP16-C: Do not compare function pointers to constant values](https://wiki.sei.cmu.edu/confluence/display/c)

‎c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql

+101
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* @id c/cert/do-not-compare-function-pointers-to-constant-values
3+
* @name EXP16-C: Do not compare function pointers to constant values
4+
* @description Comparing function pointers to a constant value is not reliable and likely indicates
5+
* a programmer error.
6+
* @kind problem
7+
* @precision very-high
8+
* @problem.severity error
9+
* @tags external/cert/id/exp16-c
10+
* correctness
11+
* external/cert/obligation/rule
12+
*/
13+
14+
import cpp
15+
import codingstandards.c.cert
16+
import codingstandards.cpp.types.FunctionType
17+
import semmle.code.cpp.controlflow.IRGuards
18+
19+
class FunctionExpr extends Expr {
20+
Element function;
21+
string funcName;
22+
23+
FunctionExpr() {
24+
function = this.(FunctionAccess).getTarget() and
25+
funcName = "Function " + function.(Function).getName()
26+
or
27+
this.(VariableAccess).getUnderlyingType() instanceof FunctionType and
28+
function = this and
29+
funcName = "Function pointer variable " + this.(VariableAccess).getTarget().getName()
30+
or
31+
this.getUnderlyingType() instanceof FunctionType and
32+
not this instanceof FunctionAccess and
33+
not this instanceof VariableAccess and
34+
function = this and
35+
funcName = "Expression with function pointer type"
36+
}
37+
38+
Element getFunction() { result = function }
39+
40+
string getFuncName() { result = funcName }
41+
}
42+
43+
abstract class EffectivelyComparison extends Element {
44+
abstract string getExplanation();
45+
46+
abstract FunctionExpr getFunctionExpr();
47+
}
48+
49+
class ExplicitComparison extends EffectivelyComparison, ComparisonOperation {
50+
Expr constantExpr;
51+
FunctionExpr funcExpr;
52+
53+
ExplicitComparison() {
54+
funcExpr = getAnOperand() and
55+
constantExpr = getAnOperand() and
56+
exists(constantExpr.getValue()) and
57+
not funcExpr = constantExpr and
58+
not constantExpr.getExplicitlyConverted().getUnderlyingType() =
59+
funcExpr.getExplicitlyConverted().getUnderlyingType()
60+
}
61+
62+
override string getExplanation() { result = "$@ compared to constant value." }
63+
64+
override FunctionExpr getFunctionExpr() { result = funcExpr }
65+
}
66+
67+
class ImplicitComparison extends EffectivelyComparison, GuardCondition {
68+
ImplicitComparison() {
69+
this instanceof FunctionExpr and
70+
not getParent() instanceof ComparisonOperation
71+
}
72+
73+
override string getExplanation() { result = "$@ undergoes implicit constant comparison." }
74+
75+
override FunctionExpr getFunctionExpr() { result = this }
76+
}
77+
78+
from EffectivelyComparison comparison, FunctionExpr funcExpr, Element function, string funcName
79+
where
80+
not isExcluded(comparison,
81+
Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery()) and
82+
funcExpr = comparison.getFunctionExpr() and
83+
function = funcExpr.getFunction() and
84+
funcName = funcExpr.getFuncName()
85+
select comparison, comparison.getExplanation(), function, funcName
86+
//from
87+
// EqualityOperation equality, FunctionExpr funcExpr, Element function, string funcName,
88+
// Expr constantExpr
89+
//where
90+
// not isExcluded(equality, Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery()) and
91+
// funcExpr = equality.getAnOperand() and
92+
// constantExpr = equality.getAnOperand() and
93+
// exists(constantExpr.getValue()) and
94+
// function = funcExpr.getFunction() and
95+
// funcName = funcExpr.getFuncName() and
96+
// constantExpr.getFullyConverted().getUnderlyingType() =
97+
// funcExpr.getFullyConverted().getUnderlyingType()
98+
//select equality,
99+
// "Pointer to function $@ compared to constant value." +
100+
// constantExpr.getFullyConverted().getUnderlyingType().explain() + " / " +
101+
// funcExpr.getFullyConverted().getUnderlyingType().explain(), function, funcName

‎c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.expected

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
| test.c:17:7:17:13 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
2+
| test.c:20:7:20:12 | ... > ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
3+
| test.c:29:7:29:13 | ... == ... | $@ compared to constant value. | test.c:29:7:29:8 | g1 | Function pointer variable g1 |
4+
| test.c:32:7:32:16 | ... == ... | $@ compared to constant value. | test.c:32:7:32:8 | g2 | Function pointer variable g2 |
5+
| test.c:35:7:35:15 | ... != ... | $@ compared to constant value. | test.c:35:7:35:8 | g1 | Function pointer variable g1 |
6+
| test.c:38:7:38:8 | f1 | $@ undergoes implicit constant comparison. | test.c:3:5:3:6 | f1 | Function f1 |
7+
| test.c:41:7:41:8 | g1 | $@ undergoes implicit constant comparison. | test.c:41:7:41:8 | g1 | Function pointer variable g1 |
8+
| test.c:68:7:68:27 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
9+
| test.c:71:7:71:18 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
10+
| test.c:74:7:76:14 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
11+
| test.c:83:3:83:9 | ... == ... | $@ compared to constant value. | test.c:83:3:83:4 | l1 | Function pointer variable l1 |
12+
| test.c:84:3:84:12 | ... == ... | $@ compared to constant value. | test.c:84:3:84:4 | l1 | Function pointer variable l1 |
13+
| test.c:91:3:91:4 | g1 | $@ undergoes implicit constant comparison. | test.c:91:3:91:4 | g1 | Function pointer variable g1 |
14+
| test.c:96:7:96:18 | ... == ... | $@ compared to constant value. | test.c:96:9:96:10 | fp | Function pointer variable fp |
15+
| test.c:102:7:102:22 | ... == ... | $@ compared to constant value. | test.c:14:11:14:21 | get_handler | Function get_handler |
16+
| test.c:105:7:105:24 | ... == ... | $@ compared to constant value. | test.c:105:7:105:17 | call to get_handler | Expression with function pointer type |

‎c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.qlref

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql

‎c/cert/test/rules/EXP16-C/test.c

+113
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
#include <stdlib.h>
2+
3+
int f1();
4+
void (*g1)(void);
5+
int (*g2)(int);
6+
void *g3 = NULL;
7+
8+
struct S {
9+
int (*fp)(void);
10+
int x;
11+
};
12+
13+
typedef int (*handler_t)(void);
14+
handler_t get_handler(void);
15+
16+
void f2(void) {
17+
if (f1 == 0) // NON-COMPLIANT
18+
return;
19+
20+
if (f1 > 0) // NON-COMPLIANT
21+
return;
22+
23+
if (f1() == 0) // COMPLIANT
24+
return;
25+
26+
if (f1() > 0) // COMPLIANT
27+
return;
28+
29+
if (g1 == 0) // NON-COMPLIANT
30+
return;
31+
32+
if (g2 == NULL) // NON-COMPLIANT
33+
return;
34+
35+
if (g1 != 0x0) // NON-COMPLIANT
36+
return;
37+
38+
if (f1) // NON-COMPLIANT - implicit comparison
39+
return;
40+
41+
if (g1) // NON-COMPLIANT - implicit comparison
42+
return;
43+
}
44+
45+
void f3(void *p1) {
46+
if (g1 == p1) // COMPLIANT - comparing to variable
47+
return;
48+
49+
if (g2 == g3) // COMPLIANT - comparing to variable
50+
return;
51+
}
52+
53+
void f4(void) {
54+
int (*l1)(void) = 0;
55+
56+
if (f1 == f1) // COMPLIANT - comparing to constant value of same type
57+
return;
58+
59+
if (f1 == l1) // COMPLIANT - comparing to constant value of same type
60+
return;
61+
62+
if (f1 == (int (*)(void))0) // COMPLIANT - explicit cast
63+
return;
64+
65+
if (f1 == (int (*)(void))0) // COMPLIANT - explicit cast
66+
return;
67+
68+
if (f1 == (int (*)(int))0) // NON-COMPLIANT - explicit cast to wrong type
69+
return;
70+
71+
if (f1 == (int)0) // NON-COMPLIANT - cast to non-function pointer type
72+
return;
73+
74+
if (f1 ==
75+
(int)(int (*)(void))
76+
NULL) // NON-COMPLIANT - compliant cast subsumed by non-compliant cast
77+
return;
78+
}
79+
80+
typedef void (*func_t)(void);
81+
void f5(void) {
82+
func_t l1 = g1;
83+
l1 == 0; // NON-COMPLIANT
84+
l1 == NULL; // NON-COMPLIANT
85+
l1 == (func_t)0; // COMPLIANT - cast to function pointer type
86+
}
87+
88+
void f6(void) {
89+
g1 + 0; // COMPLIANT - not a comparison
90+
g1 == g2; // COMPLIANT - not comparing to constant
91+
g1 ? 1 : 0; // NON-COMPLIANT - implicit comparison
92+
}
93+
94+
void f7(void) {
95+
struct S s;
96+
if (s.fp == NULL) // NON-COMPLIANT
97+
return;
98+
99+
if (s.fp() == NULL) // COMPLIANT
100+
return;
101+
102+
if (get_handler == 0) // NON-COMPLIANT - missing parentheses
103+
return;
104+
105+
if (get_handler() == 0) // NON-COMPLIANT
106+
return;
107+
108+
if (get_handler() == (handler_t)0) // COMPLIANT
109+
return;
110+
111+
if (get_handler()() == 0) // COMPLIANT
112+
return;
113+
}

‎cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/
2+
import cpp
3+
import RuleMetadata
4+
import codingstandards.cpp.exclusions.RuleMetadata
5+
6+
newtype Expressions2Query = TDoNotCompareFunctionPointersToConstantValuesQuery()
7+
8+
predicate isExpressions2QueryMetadata(Query query, string queryId, string ruleId, string category) {
9+
query =
10+
// `Query` instance for the `doNotCompareFunctionPointersToConstantValues` query
11+
Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery() and
12+
queryId =
13+
// `@id` for the `doNotCompareFunctionPointersToConstantValues` query
14+
"c/cert/do-not-compare-function-pointers-to-constant-values" and
15+
ruleId = "EXP16-C" and
16+
category = "rule"
17+
}
18+
19+
module Expressions2Package {
20+
Query doNotCompareFunctionPointersToConstantValuesQuery() {
21+
//autogenerate `Query` type
22+
result =
23+
// `Query` type for `doNotCompareFunctionPointersToConstantValues` query
24+
TQueryC(TExpressions2PackageQuery(TDoNotCompareFunctionPointersToConstantValuesQuery()))
25+
}
26+
}

0 commit comments

Comments
 (0)