-
Notifications
You must be signed in to change notification settings - Fork 67
Implement expressions2 package #881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
MichaelRFairhurst
wants to merge
6
commits into
main
Choose a base branch
from
michaelrfairhurst/implement-expressions2-package
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
14bd937
Update cert-help-extraction.py to support CERT-C optional (recommenda…
MichaelRFairhurst d2e638e
Implement expressions2 package
MichaelRFairhurst f6d62ec
Format
MichaelRFairhurst 067e172
Change obligation
MichaelRFairhurst 010b7c7
Remove commented out code
MichaelRFairhurst b0033df
Merge branch 'main' into michaelrfairhurst/implement-expressions2-pac…
lcartey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
129 changes: 129 additions & 0 deletions
129
c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
# EXP16-C: Do not compare function pointers to constant values | ||
|
||
This query implements the CERT-C rule EXP16-C: | ||
|
||
> Do not compare function pointers to constant values | ||
|
||
|
||
## Description | ||
|
||
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. | ||
|
||
## Noncompliant Code Example | ||
|
||
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. | ||
|
||
```cpp | ||
/* First the options that are allowed only for root */ | ||
if (getuid == 0 || geteuid != 0) { | ||
/* ... */ | ||
} | ||
|
||
``` | ||
|
||
## Noncompliant Code Example | ||
|
||
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). | ||
|
||
```cpp | ||
/* First the options that are allowed only for root */ | ||
if (getuid() == 0 || geteuid != 0) { | ||
/* ... */ | ||
} | ||
|
||
``` | ||
|
||
## Compliant Solution | ||
|
||
The solution is to provide the open and close parentheses following the `geteuid` token so that the function is properly invoked: | ||
|
||
```cpp | ||
/* First the options that are allowed only for root */ | ||
if (getuid() == 0 || geteuid() != 0) { | ||
/* ... */ | ||
} | ||
|
||
``` | ||
|
||
## Compliant Solution | ||
|
||
A function pointer can be compared to a null function pointer of the same type: | ||
|
||
```cpp | ||
/* First the options that are allowed only for root */ | ||
if (getuid == (uid_t(*)(void))0 || geteuid != (uid_t(*)(void))0) { | ||
/* ... */ | ||
} | ||
|
||
``` | ||
This code should not be diagnosed by an analyzer. | ||
|
||
## Noncompliant Code Example | ||
|
||
In this noncompliant code example, the function pointer `do_xyz` is implicitly compared unequal to 0: | ||
|
||
```cpp | ||
int do_xyz(void); | ||
|
||
int f(void) { | ||
/* ... */ | ||
if (do_xyz) { | ||
return -1; /* Indicate failure */ | ||
} | ||
/* ... */ | ||
return 0; | ||
} | ||
|
||
``` | ||
|
||
## Compliant Solution | ||
|
||
In this compliant solution, the function `do_xyz()` is invoked and the return value is compared to 0: | ||
|
||
```cpp | ||
int do_xyz(void); | ||
|
||
int f(void) { | ||
/* ... */ | ||
if (do_xyz()) { | ||
return -1; /* Indicate failure */ | ||
} | ||
/* ... */ | ||
return 0; | ||
} | ||
|
||
``` | ||
|
||
## Risk Assessment | ||
|
||
Errors of omission can result in unintended program flow. | ||
|
||
<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> | ||
|
||
|
||
## 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> 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> | ||
|
||
|
||
## 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+EXP16-C). | ||
|
||
## Related Guidelines | ||
|
||
<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> | ||
|
||
|
||
## Bibliography | ||
|
||
<table> <tbody> <tr> <td> \[ <a> Hatton 1995 </a> \] </td> <td> Section 2.7.2, "Errors of Omission and Addition" </td> </tr> </tbody> </table> | ||
|
||
|
||
## Implementation notes | ||
|
||
None | ||
|
||
## References | ||
|
||
* CERT-C: [EXP16-C: Do not compare function pointers to constant values](https://wiki.sei.cmu.edu/confluence/display/c) |
85 changes: 85 additions & 0 deletions
85
c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
/** | ||
* @id c/cert/do-not-compare-function-pointers-to-constant-values | ||
* @name EXP16-C: Do not compare function pointers to constant values | ||
* @description Comparing function pointers to a constant value is not reliable and likely indicates | ||
* a programmer error. | ||
* @kind problem | ||
* @precision very-high | ||
* @problem.severity error | ||
* @tags external/cert/id/exp16-c | ||
* correctness | ||
* external/cert/obligation/recommendation | ||
*/ | ||
|
||
import cpp | ||
import codingstandards.c.cert | ||
import codingstandards.cpp.types.FunctionType | ||
import semmle.code.cpp.controlflow.IRGuards | ||
|
||
class FunctionExpr extends Expr { | ||
Element function; | ||
string funcName; | ||
|
||
FunctionExpr() { | ||
function = this.(FunctionAccess).getTarget() and | ||
funcName = "Function " + function.(Function).getName() | ||
or | ||
this.(VariableAccess).getUnderlyingType() instanceof FunctionType and | ||
function = this and | ||
funcName = "Function pointer variable " + this.(VariableAccess).getTarget().getName() | ||
or | ||
this.getUnderlyingType() instanceof FunctionType and | ||
not this instanceof FunctionAccess and | ||
not this instanceof VariableAccess and | ||
function = this and | ||
funcName = "Expression with function pointer type" | ||
} | ||
|
||
Element getFunction() { result = function } | ||
|
||
string getFuncName() { result = funcName } | ||
} | ||
|
||
abstract class EffectivelyComparison extends Element { | ||
abstract string getExplanation(); | ||
|
||
abstract FunctionExpr getFunctionExpr(); | ||
} | ||
|
||
class ExplicitComparison extends EffectivelyComparison, ComparisonOperation { | ||
Expr constantExpr; | ||
FunctionExpr funcExpr; | ||
|
||
ExplicitComparison() { | ||
funcExpr = getAnOperand() and | ||
constantExpr = getAnOperand() and | ||
exists(constantExpr.getValue()) and | ||
not funcExpr = constantExpr and | ||
not constantExpr.getExplicitlyConverted().getUnderlyingType() = | ||
funcExpr.getExplicitlyConverted().getUnderlyingType() | ||
} | ||
|
||
override string getExplanation() { result = "$@ compared to constant value." } | ||
|
||
override FunctionExpr getFunctionExpr() { result = funcExpr } | ||
} | ||
|
||
class ImplicitComparison extends EffectivelyComparison, GuardCondition { | ||
ImplicitComparison() { | ||
this instanceof FunctionExpr and | ||
not getParent() instanceof ComparisonOperation | ||
} | ||
|
||
override string getExplanation() { result = "$@ undergoes implicit constant comparison." } | ||
|
||
override FunctionExpr getFunctionExpr() { result = this } | ||
} | ||
|
||
from EffectivelyComparison comparison, FunctionExpr funcExpr, Element function, string funcName | ||
where | ||
not isExcluded(comparison, | ||
Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery()) and | ||
funcExpr = comparison.getFunctionExpr() and | ||
function = funcExpr.getFunction() and | ||
funcName = funcExpr.getFuncName() | ||
select comparison, comparison.getExplanation(), function, funcName |
16 changes: 16 additions & 0 deletions
16
c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.expected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
| test.c:17:7:17:13 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 | | ||
| test.c:20:7:20:12 | ... > ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 | | ||
| test.c:29:7:29:13 | ... == ... | $@ compared to constant value. | test.c:29:7:29:8 | g1 | Function pointer variable g1 | | ||
| test.c:32:7:32:16 | ... == ... | $@ compared to constant value. | test.c:32:7:32:8 | g2 | Function pointer variable g2 | | ||
| test.c:35:7:35:15 | ... != ... | $@ compared to constant value. | test.c:35:7:35:8 | g1 | Function pointer variable g1 | | ||
| test.c:38:7:38:8 | f1 | $@ undergoes implicit constant comparison. | test.c:3:5:3:6 | f1 | Function f1 | | ||
| test.c:41:7:41:8 | g1 | $@ undergoes implicit constant comparison. | test.c:41:7:41:8 | g1 | Function pointer variable g1 | | ||
| test.c:68:7:68:27 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 | | ||
| test.c:71:7:71:18 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 | | ||
| test.c:74:7:76:14 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 | | ||
| test.c:83:3:83:9 | ... == ... | $@ compared to constant value. | test.c:83:3:83:4 | l1 | Function pointer variable l1 | | ||
| test.c:84:3:84:12 | ... == ... | $@ compared to constant value. | test.c:84:3:84:4 | l1 | Function pointer variable l1 | | ||
| test.c:91:3:91:4 | g1 | $@ undergoes implicit constant comparison. | test.c:91:3:91:4 | g1 | Function pointer variable g1 | | ||
| test.c:96:7:96:18 | ... == ... | $@ compared to constant value. | test.c:96:9:96:10 | fp | Function pointer variable fp | | ||
| test.c:102:7:102:22 | ... == ... | $@ compared to constant value. | test.c:14:11:14:21 | get_handler | Function get_handler | | ||
| 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 | |
1 change: 1 addition & 0 deletions
1
c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.qlref
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
#include <stdlib.h> | ||
|
||
int f1(); | ||
void (*g1)(void); | ||
int (*g2)(int); | ||
void *g3 = NULL; | ||
|
||
struct S { | ||
int (*fp)(void); | ||
int x; | ||
}; | ||
|
||
typedef int (*handler_t)(void); | ||
handler_t get_handler(void); | ||
|
||
void f2(void) { | ||
if (f1 == 0) // NON-COMPLIANT | ||
return; | ||
|
||
if (f1 > 0) // NON-COMPLIANT | ||
return; | ||
|
||
if (f1() == 0) // COMPLIANT | ||
return; | ||
|
||
if (f1() > 0) // COMPLIANT | ||
return; | ||
|
||
if (g1 == 0) // NON-COMPLIANT | ||
return; | ||
|
||
if (g2 == NULL) // NON-COMPLIANT | ||
return; | ||
|
||
if (g1 != 0x0) // NON-COMPLIANT | ||
return; | ||
|
||
if (f1) // NON-COMPLIANT - implicit comparison | ||
return; | ||
|
||
if (g1) // NON-COMPLIANT - implicit comparison | ||
return; | ||
} | ||
|
||
void f3(void *p1) { | ||
if (g1 == p1) // COMPLIANT - comparing to variable | ||
return; | ||
|
||
if (g2 == g3) // COMPLIANT - comparing to variable | ||
return; | ||
} | ||
|
||
void f4(void) { | ||
int (*l1)(void) = 0; | ||
|
||
if (f1 == f1) // COMPLIANT - comparing to constant value of same type | ||
return; | ||
|
||
if (f1 == l1) // COMPLIANT - comparing to constant value of same type | ||
return; | ||
|
||
if (f1 == (int (*)(void))0) // COMPLIANT - explicit cast | ||
return; | ||
|
||
if (f1 == (int (*)(void))0) // COMPLIANT - explicit cast | ||
return; | ||
|
||
if (f1 == (int (*)(int))0) // NON-COMPLIANT - explicit cast to wrong type | ||
return; | ||
|
||
if (f1 == (int)0) // NON-COMPLIANT - cast to non-function pointer type | ||
return; | ||
|
||
if (f1 == | ||
(int)(int (*)(void)) | ||
NULL) // NON-COMPLIANT - compliant cast subsumed by non-compliant cast | ||
return; | ||
} | ||
|
||
typedef void (*func_t)(void); | ||
void f5(void) { | ||
func_t l1 = g1; | ||
l1 == 0; // NON-COMPLIANT | ||
l1 == NULL; // NON-COMPLIANT | ||
l1 == (func_t)0; // COMPLIANT - cast to function pointer type | ||
} | ||
|
||
void f6(void) { | ||
g1 + 0; // COMPLIANT - not a comparison | ||
g1 == g2; // COMPLIANT - not comparing to constant | ||
g1 ? 1 : 0; // NON-COMPLIANT - implicit comparison | ||
} | ||
|
||
void f7(void) { | ||
struct S s; | ||
if (s.fp == NULL) // NON-COMPLIANT | ||
return; | ||
|
||
if (s.fp() == NULL) // COMPLIANT | ||
return; | ||
|
||
if (get_handler == 0) // NON-COMPLIANT - missing parentheses | ||
return; | ||
|
||
if (get_handler() == 0) // NON-COMPLIANT | ||
return; | ||
|
||
if (get_handler() == (handler_t)0) // COMPLIANT | ||
return; | ||
|
||
if (get_handler()() == 0) // COMPLIANT | ||
return; | ||
} |
26 changes: 26 additions & 0 deletions
26
cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ | ||
import cpp | ||
import RuleMetadata | ||
import codingstandards.cpp.exclusions.RuleMetadata | ||
|
||
newtype Expressions2Query = TDoNotCompareFunctionPointersToConstantValuesQuery() | ||
|
||
predicate isExpressions2QueryMetadata(Query query, string queryId, string ruleId, string category) { | ||
query = | ||
// `Query` instance for the `doNotCompareFunctionPointersToConstantValues` query | ||
Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery() and | ||
queryId = | ||
// `@id` for the `doNotCompareFunctionPointersToConstantValues` query | ||
"c/cert/do-not-compare-function-pointers-to-constant-values" and | ||
ruleId = "EXP16-C" and | ||
category = "recommendation" | ||
} | ||
|
||
module Expressions2Package { | ||
Query doNotCompareFunctionPointersToConstantValuesQuery() { | ||
//autogenerate `Query` type | ||
result = | ||
// `Query` type for `doNotCompareFunctionPointersToConstantValues` query | ||
TQueryC(TExpressions2PackageQuery(TDoNotCompareFunctionPointersToConstantValuesQuery())) | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if we should permit a comparison in the case that it guards a call to the same function, e.g.
Looking at the results on MRVA I see this is a common pattern for implementing "optional" functions.