From 35cfb0f1e1e57a71a3a1d9537903a7c16aab304e Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Sat, 29 Mar 2025 15:54:57 -0700 Subject: [PATCH 01/10] Add additional CERT-C rules to rules.csv --- rules.csv | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rules.csv b/rules.csv index 3f7961b63..2db1d7a66 100644 --- a/rules.csv +++ b/rules.csv @@ -515,6 +515,7 @@ c,CERT-C,ERR30-C,Yes,Rule,,,Take care when reading errno,M19-3-1,Contracts4,Hard c,CERT-C,ERR32-C,Yes,Rule,,,Do not rely on indeterminate values of errno,,Contracts5,Hard, c,CERT-C,ERR33-C,Yes,Rule,,,Detect and handle standard library errors,MEM52-CPP,Contracts5,Hard, c,CERT-C,ERR34-C,OutOfScope,Rule,,,Detect errors when converting a string to a number,,,, +c,CERT-C,EXP16-C,Yes,Rule,,,Do not compare function pointers to constant values,,Expressions2,Medium, c,CERT-C,EXP30-C,Yes,Rule,,,Do not depend on the order of evaluation for side effects,EXP50-CPP,SideEffects1,Easy, c,CERT-C,EXP32-C,Yes,Rule,,,Do not access a volatile object through a nonvolatile reference,,Pointers3,Easy, c,CERT-C,EXP33-C,Yes,Rule,,,Do not read uninitialized memory,EXP53-CPP,InvalidMemory1,Import, @@ -530,6 +531,8 @@ c,CERT-C,EXP44-C,Yes,Rule,,,"Do not rely on side effects in operands to sizeof, c,CERT-C,EXP45-C,Yes,Rule,,,Do not perform assignments in selection statements,M6-2-1,SideEffects1,Medium, c,CERT-C,EXP46-C,Yes,Rule,,,Do not use a bitwise operator with a Boolean-like operand,,Expressions,Easy, c,CERT-C,EXP47-C,OutOfScope,Rule,,,Do not call va_arg with an argument of the incorrect type,,,, +c,CERT-C,FIO03-C,Yes,Rule,,,Do not make assumptions about fopen() and file creation,,IO5,Hard, +c,CERT-C,FIO21-C,Yes,Rule,,,Do not create temporary files in shared directories,,IO5,Easy, c,CERT-C,FIO30-C,Yes,Rule,,,Exclude user input from format strings,A27-0-1,IO1,Import, c,CERT-C,FIO32-C,Yes,Rule,,,Do not perform operations on devices that are only appropriate for files,,IO3,Medium, c,CERT-C,FIO34-C,Yes,Rule,,,Distinguish between characters read from a file and EOF or WEOF,,IO1,Hard, @@ -569,7 +572,7 @@ c,CERT-C,MSC38-C,Yes,Rule,,,Do not treat a predefined identifier as an object if c,CERT-C,MSC39-C,Yes,Rule,,,Do not call va_arg() on a va_list that has an indeterminate value,,Contracts7,Hard, c,CERT-C,MSC40-C,Yes,Rule,,,Do not violate constraints,,Contracts,Very Hard, c,CERT-C,MSC41-C,OutOfScope,Rule,,,Never hard code sensitive information,,,, -c,CERT-C,POS30-C,OutOfScope,Rule,,,Use the readlink() function properly,,,, +c,CERT-C,POS30-C,Yes,Rule,,,Use the readlink() function properly,,IO5,Hard, c,CERT-C,POS34-C,OutOfScope,Rule,,,Do not call putenv() with a pointer to an automatic variable as the argument,,,, c,CERT-C,POS35-C,OutOfScope,Rule,,,Avoid race conditions while checking for the existence of a symbolic link,,,, c,CERT-C,POS36-C,OutOfScope,Rule,,,Observe correct revocation order while relinquishing privileges,,,, From 315276902e53b55bfbb6ecd458f04d6a8b4abc27 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Wed, 9 Apr 2025 19:25:48 -0700 Subject: [PATCH 02/10] Update obligation level of new CERT-C rules --- rules.csv | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rules.csv b/rules.csv index 2db1d7a66..68049625e 100644 --- a/rules.csv +++ b/rules.csv @@ -515,7 +515,7 @@ c,CERT-C,ERR30-C,Yes,Rule,,,Take care when reading errno,M19-3-1,Contracts4,Hard c,CERT-C,ERR32-C,Yes,Rule,,,Do not rely on indeterminate values of errno,,Contracts5,Hard, c,CERT-C,ERR33-C,Yes,Rule,,,Detect and handle standard library errors,MEM52-CPP,Contracts5,Hard, c,CERT-C,ERR34-C,OutOfScope,Rule,,,Detect errors when converting a string to a number,,,, -c,CERT-C,EXP16-C,Yes,Rule,,,Do not compare function pointers to constant values,,Expressions2,Medium, +c,CERT-C,EXP16-C,Yes,Recommendation,,,Do not compare function pointers to constant values,,Expressions2,Medium, c,CERT-C,EXP30-C,Yes,Rule,,,Do not depend on the order of evaluation for side effects,EXP50-CPP,SideEffects1,Easy, c,CERT-C,EXP32-C,Yes,Rule,,,Do not access a volatile object through a nonvolatile reference,,Pointers3,Easy, c,CERT-C,EXP33-C,Yes,Rule,,,Do not read uninitialized memory,EXP53-CPP,InvalidMemory1,Import, @@ -531,8 +531,8 @@ c,CERT-C,EXP44-C,Yes,Rule,,,"Do not rely on side effects in operands to sizeof, c,CERT-C,EXP45-C,Yes,Rule,,,Do not perform assignments in selection statements,M6-2-1,SideEffects1,Medium, c,CERT-C,EXP46-C,Yes,Rule,,,Do not use a bitwise operator with a Boolean-like operand,,Expressions,Easy, c,CERT-C,EXP47-C,OutOfScope,Rule,,,Do not call va_arg with an argument of the incorrect type,,,, -c,CERT-C,FIO03-C,Yes,Rule,,,Do not make assumptions about fopen() and file creation,,IO5,Hard, -c,CERT-C,FIO21-C,Yes,Rule,,,Do not create temporary files in shared directories,,IO5,Easy, +c,CERT-C,FIO03-C,Yes,Recommendation,,,Do not make assumptions about fopen() and file creation,,IO5,Hard, +c,CERT-C,FIO21-C,Yes,Recommendation,,,Do not create temporary files in shared directories,,IO5,Easy, c,CERT-C,FIO30-C,Yes,Rule,,,Exclude user input from format strings,A27-0-1,IO1,Import, c,CERT-C,FIO32-C,Yes,Rule,,,Do not perform operations on devices that are only appropriate for files,,IO3,Medium, c,CERT-C,FIO34-C,Yes,Rule,,,Distinguish between characters read from a file and EOF or WEOF,,IO1,Hard, From 64ad8fb8955f3e749ce61c8aca33c0272657b30d Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Wed, 9 Apr 2025 23:30:12 -0700 Subject: [PATCH 03/10] Add new query suites for CERT-C recommendations --- c/cert/src/codeql-suites/cert-c-default.qls | 10 ++++++++++ c/cert/src/codeql-suites/cert-c-recommendation.qls | 10 ++++++++++ c/cert/src/codeql-suites/cert-default.qls | 11 ++--------- c/cert/src/qlpack.yml | 1 + ...5-04-09-new-cert-c-recommendation-query-suite.md | 9 +++++++++ cpp/cert/src/codeql-suites/cert-cpp-default.qls | 9 +++++++++ .../cert-cpp-single-translation-unit.qls | 11 +++++++++++ cpp/cert/src/codeql-suites/cert-default.qls | 11 ++--------- .../codeql-suites/cert-single-translation-unit.qls | 13 ++----------- cpp/cert/src/qlpack.yml | 1 + 10 files changed, 57 insertions(+), 29 deletions(-) create mode 100644 c/cert/src/codeql-suites/cert-c-default.qls create mode 100644 c/cert/src/codeql-suites/cert-c-recommendation.qls create mode 100644 change_notes/2025-04-09-new-cert-c-recommendation-query-suite.md create mode 100644 cpp/cert/src/codeql-suites/cert-cpp-default.qls create mode 100644 cpp/cert/src/codeql-suites/cert-cpp-single-translation-unit.qls diff --git a/c/cert/src/codeql-suites/cert-c-default.qls b/c/cert/src/codeql-suites/cert-c-default.qls new file mode 100644 index 000000000..348d2f37a --- /dev/null +++ b/c/cert/src/codeql-suites/cert-c-default.qls @@ -0,0 +1,10 @@ +- description: CERT C 2016 (Default) +- qlpack: codeql/cert-c-coding-standards +- include: + kind: + - problem + - path-problem + - external/cert/obligation/rule +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/c/cert/src/codeql-suites/cert-c-recommendation.qls b/c/cert/src/codeql-suites/cert-c-recommendation.qls new file mode 100644 index 000000000..59ac5e9c2 --- /dev/null +++ b/c/cert/src/codeql-suites/cert-c-recommendation.qls @@ -0,0 +1,10 @@ +- description: CERT C 2016 (Recommendations) +- qlpack: codeql/cert-c-coding-standards +- include: + kind: + - problem + - path-problem + - external/cert/obligation/recommendation +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/c/cert/src/codeql-suites/cert-default.qls b/c/cert/src/codeql-suites/cert-default.qls index 1e11a0afc..c093b31fa 100644 --- a/c/cert/src/codeql-suites/cert-default.qls +++ b/c/cert/src/codeql-suites/cert-default.qls @@ -1,9 +1,2 @@ -- description: CERT C 2016 (Default) -- qlpack: codeql/cert-c-coding-standards -- include: - kind: - - problem - - path-problem -- exclude: - tags contain: - - external/cert/default-disabled +- description: "DEPRECATED - CERT C 2016 - use cert-c-default.qls instead" +- import: codeql-suites/cert-c-default.qls diff --git a/c/cert/src/qlpack.yml b/c/cert/src/qlpack.yml index 631639301..e3664d75b 100644 --- a/c/cert/src/qlpack.yml +++ b/c/cert/src/qlpack.yml @@ -3,6 +3,7 @@ version: 2.44.0-dev description: CERT C 2016 suites: codeql-suites license: MIT +default-suite-file: codeql-suites/cert-c-default.qls dependencies: codeql/common-c-coding-standards: '*' codeql/cpp-all: 2.1.1 diff --git a/change_notes/2025-04-09-new-cert-c-recommendation-query-suite.md b/change_notes/2025-04-09-new-cert-c-recommendation-query-suite.md new file mode 100644 index 000000000..910433e35 --- /dev/null +++ b/change_notes/2025-04-09-new-cert-c-recommendation-query-suite.md @@ -0,0 +1,9 @@ + - The following query suites have been added or modified for CERT C: + - A new query suite has been created `cert-c-default.qls` to avoid confusion with the CERT C++ query suites. The `cert-default.qls` suite has been deprecated, and will be removed in a future releases, and is replaced by the `cert-c-default.qls` suite. + - The `cert-c-default.qls` suite has been specified as the default for the pack, and will include our most up-to-date coverage for CERT C. + - One new query suite, `cert-c-recommended.qls` has been added to enable running CERT recommendations (as opposed to rules) that will be added in the future. + - The default query suite, `cert-c-default.qls` has been set to exclude CERT recommendations (as opposed to rules) that will be added in the future. + - The following query suites have been added or modified for CERT C++: + - A new query suite has been created `cert-cpp-default.qls` to avoid confusion with the CERT C query suites. The `cert-default.qls` suite has been deprecated, and will be removed in a future releases, and is replaced by the `cert-cpp-default.qls` suite. + - The `cert-cpp-default.qls` suite has been specified as the default for the pack, and will include our most up-to-date coverage for CERT C. + - A new query suite has been created `cert-cpp-single-translation-unit.qls` to avoid confusion with the CERT C query suites. The `cert-single-translation-unit.qls` suite has been deprecated, and will be removed in a future releases, and is replaced by the `cert-cpp-single-translation-unit.qls` suite. \ No newline at end of file diff --git a/cpp/cert/src/codeql-suites/cert-cpp-default.qls b/cpp/cert/src/codeql-suites/cert-cpp-default.qls new file mode 100644 index 000000000..e9211246b --- /dev/null +++ b/cpp/cert/src/codeql-suites/cert-cpp-default.qls @@ -0,0 +1,9 @@ +- description: CERT C++ 2016 (Default) +- qlpack: codeql/cert-cpp-coding-standards +- include: + kind: + - problem + - path-problem +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/cpp/cert/src/codeql-suites/cert-cpp-single-translation-unit.qls b/cpp/cert/src/codeql-suites/cert-cpp-single-translation-unit.qls new file mode 100644 index 000000000..2f09815e0 --- /dev/null +++ b/cpp/cert/src/codeql-suites/cert-cpp-single-translation-unit.qls @@ -0,0 +1,11 @@ +- description: CERT C++ 2016 (Single Translation Unit) +- qlpack: codeql/cert-cpp-coding-standards +- include: + kind: + - problem + - path-problem + tags contain: + - scope/single-translation-unit +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/cpp/cert/src/codeql-suites/cert-default.qls b/cpp/cert/src/codeql-suites/cert-default.qls index e9211246b..66599b60f 100644 --- a/cpp/cert/src/codeql-suites/cert-default.qls +++ b/cpp/cert/src/codeql-suites/cert-default.qls @@ -1,9 +1,2 @@ -- description: CERT C++ 2016 (Default) -- qlpack: codeql/cert-cpp-coding-standards -- include: - kind: - - problem - - path-problem -- exclude: - tags contain: - - external/cert/default-disabled +- description: "DEPRECATED - CERT C++ 2016 - use cert-cpp-default.qls instead" +- import: codeql-suites/cert-cpp-default.qls \ No newline at end of file diff --git a/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls b/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls index 2f09815e0..496664839 100644 --- a/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls +++ b/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls @@ -1,11 +1,2 @@ -- description: CERT C++ 2016 (Single Translation Unit) -- qlpack: codeql/cert-cpp-coding-standards -- include: - kind: - - problem - - path-problem - tags contain: - - scope/single-translation-unit -- exclude: - tags contain: - - external/cert/default-disabled +- description: "DEPRECATED - CERT C++ 2016 (Single Translation Unit) - use cert-cpp-single-translation-unit.qls instead" +- import: codeql-suites/cert-cpp-single-translation-unit.qls \ No newline at end of file diff --git a/cpp/cert/src/qlpack.yml b/cpp/cert/src/qlpack.yml index f44646cdb..97976d30c 100644 --- a/cpp/cert/src/qlpack.yml +++ b/cpp/cert/src/qlpack.yml @@ -3,6 +3,7 @@ version: 2.44.0-dev description: CERT C++ 2016 suites: codeql-suites license: MIT +default-suite-file: codeql-suites/cert-cpp-default.qls dependencies: codeql/cpp-all: 2.1.1 codeql/common-cpp-coding-standards: '*' From 8843822ec1b46b0a913edfc5a5f7372d1e4752a8 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 10 Apr 2025 08:45:07 -0700 Subject: [PATCH 04/10] Add 'recommendation' to CERT schema --- schemas/rule-package.schema.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/schemas/rule-package.schema.json b/schemas/rule-package.schema.json index b4f729afe..f9f65d679 100644 --- a/schemas/rule-package.schema.json +++ b/schemas/rule-package.schema.json @@ -94,7 +94,8 @@ "obligation": { "type": "string", "enum": [ - "rule" + "rule", + "recommendation" ] } }, From d39ec8d75696f4d5ee0493f97b55102f47d5da52 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 10 Apr 2025 09:25:26 -0700 Subject: [PATCH 05/10] Fix, move 'recommendation' from CERT-C++ to CERT-C in schema --- schemas/rule-package.schema.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/schemas/rule-package.schema.json b/schemas/rule-package.schema.json index f9f65d679..f8c3f028e 100644 --- a/schemas/rule-package.schema.json +++ b/schemas/rule-package.schema.json @@ -94,8 +94,7 @@ "obligation": { "type": "string", "enum": [ - "rule", - "recommendation" + "rule" ] } }, @@ -142,7 +141,8 @@ "obligation": { "type": "string", "enum": [ - "rule" + "rule", + "recommendation" ] } }, From 14bd93788a59b7265a730626fe44178b0f56d8a1 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Sun, 30 Mar 2025 18:30:36 -0700 Subject: [PATCH 06/10] Update cert-help-extraction.py to support CERT-C optional (recommendation) rules --- scripts/help/cert-help-extraction.py | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/scripts/help/cert-help-extraction.py b/scripts/help/cert-help-extraction.py index 6bd1abccd..f785b0955 100755 --- a/scripts/help/cert-help-extraction.py +++ b/scripts/help/cert-help-extraction.py @@ -1,5 +1,6 @@ #!/usr/bin/env python3 from argparse import ArgumentParser +from typing import Generator import tempfile import re import urllib.request @@ -23,6 +24,7 @@ CERT_WIKI = "https://wiki.sei.cmu.edu" RULES_LIST_C = "/confluence/display/c/2+Rules" +RECOMMENDED_LIST_C = "/confluence/display/c/3+Recommendations" RULES_LIST_CPP = "/confluence/display/cplusplus/2+Rules" cache_path = script_path.parent / '.cache' @@ -47,16 +49,22 @@ def soupify(url: str) -> BeautifulSoup: return BeautifulSoup(content, 'html.parser') - -def get_rules(): - rules = [] - for soup in [soupify(f"{CERT_WIKI}{RULES_LIST_C}"), soupify(f"{CERT_WIKI}{RULES_LIST_CPP}")]: +def get_rule_listings() -> Generator[Tag, None, None]: + for rule_list_id in [RULES_LIST_C, RULES_LIST_CPP]: + soup = soupify(f"{CERT_WIKI}{rule_list_id}") if soup == None: - return None - - rule_listing_start = soup.find( + continue + + yield soup.find( "h1", string="Rule Listing") + soup = soupify(f"{CERT_WIKI}{RECOMMENDED_LIST_C}") + if soup != None: + yield soup.find("h1", string="Recommendation Listing") + +def get_rules(): + rules = [] + for rule_listing_start in get_rule_listings(): for link in rule_listing_start.next_element.next_element.find_all('a'): if '-C' in link.string: rule, title = map(str.strip, link.string.split('.', 1)) @@ -214,6 +222,8 @@ def helper(node): # Fix a broken url present in many CERT-C pages if node.name == 'a' and 'href' in node.attrs and node['href'] == "http://BB. Definitions#vulnerability": node['href'] = "https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability" + elif node.name == 'a' and 'href' in node.attrs and node['href'] == "http://BB. Definitions#unexpected behavior": + node['href'] = "https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unexpectedbehavior" # Turn relative URLs into absolute URLS elif node.name == 'a' and 'href' in node.attrs and node['href'].startswith("/confluence"): node['href'] = f"{CERT_WIKI}{node['href']}" From d2e638eedab86f076b1cbd04cd81383e09cb5611 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Sun, 30 Mar 2025 18:27:49 -0700 Subject: [PATCH 07/10] Implement expressions2 package --- ...CompareFunctionPointersToConstantValues.md | 129 ++++++++++++++++++ ...CompareFunctionPointersToConstantValues.ql | 101 ++++++++++++++ ...eFunctionPointersToConstantValues.expected | 16 +++ ...pareFunctionPointersToConstantValues.qlref | 1 + c/cert/test/rules/EXP16-C/test.c | 113 +++++++++++++++ .../cpp/exclusions/c/Expressions2.qll | 26 ++++ .../cpp/exclusions/c/RuleMetadata.qll | 3 + .../codingstandards/cpp/types/Compatible.qll | 19 +-- .../cpp/types/FunctionType.qll | 19 +++ rule_packages/c/Expressions2.json | 23 ++++ 10 files changed, 432 insertions(+), 18 deletions(-) create mode 100644 c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.md create mode 100644 c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql create mode 100644 c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.expected create mode 100644 c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.qlref create mode 100644 c/cert/test/rules/EXP16-C/test.c create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll create mode 100644 cpp/common/src/codingstandards/cpp/types/FunctionType.qll create mode 100644 rule_packages/c/Expressions2.json diff --git a/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.md b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.md new file mode 100644 index 000000000..b5a7c98bd --- /dev/null +++ b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.md @@ -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. + +
Recommendation Severity Likelihood Remediation Cost Priority Level
EXP16-C Low Likely Medium P6 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 24.04 function-name-constant-comparison Partially checked
Coverity 2017.07 BAD_COMPARE Can detect the specific instance where the address of a function is compared against 0, such as in the case of geteuid versus getuid() in the implementation-specific details
GCC 4.3.5 Can detect violations of this recommendation when the -Wall flag is used
Helix QAC 2024.4 C0428, C3004, C3344
Klocwork 2024.4 CWARN.NULLCHECK.FUNCNAMECWARN.FUNCADDR
LDRA tool suite 9.7.1 99 S Partially implemented
Parasoft C/C++test 2024.2 CERT_C-EXP16-a Function address should not be compared to zero
PC-lint Plus 1.4 2440, 2441 Partially supported: reports address of function, array, or variable directly or indirectly compared to null
PVS-Studio 7.35 V516, V1058
RuleChecker 24.04 function-name-constant-comparison Partially checked
+ + +## 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 + +
SEI CERT C++ Coding Standard VOID EXP16-CPP. Avoid conversions using void pointers
ISO/IEC TR 24772:2013 Likely incorrect expressions \[KOA\]
ISO/IEC TS 17961 Comparing function addresses to zero \[funcaddr\]
MITRE CWE CWE-480 , Use of incorrect operator CWE-482 , Comparing instead of assigning
+ + +## Bibliography + +
\[ Hatton 1995 \] Section 2.7.2, "Errors of Omission and Addition"
+ + +## Implementation notes + +None + +## References + +* CERT-C: [EXP16-C: Do not compare function pointers to constant values](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql new file mode 100644 index 000000000..561374d66 --- /dev/null +++ b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql @@ -0,0 +1,101 @@ +/** + * @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/rule + */ + +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 +//from +// EqualityOperation equality, FunctionExpr funcExpr, Element function, string funcName, +// Expr constantExpr +//where +// not isExcluded(equality, Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery()) and +// funcExpr = equality.getAnOperand() and +// constantExpr = equality.getAnOperand() and +// exists(constantExpr.getValue()) and +// function = funcExpr.getFunction() and +// funcName = funcExpr.getFuncName() and +// constantExpr.getFullyConverted().getUnderlyingType() = +// funcExpr.getFullyConverted().getUnderlyingType() +//select equality, +// "Pointer to function $@ compared to constant value." + +// constantExpr.getFullyConverted().getUnderlyingType().explain() + " / " + +// funcExpr.getFullyConverted().getUnderlyingType().explain(), function, funcName diff --git a/c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.expected b/c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.expected new file mode 100644 index 000000000..a18f0d32f --- /dev/null +++ b/c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.expected @@ -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 | diff --git a/c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.qlref b/c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.qlref new file mode 100644 index 000000000..7d99fa987 --- /dev/null +++ b/c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.qlref @@ -0,0 +1 @@ +rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql \ No newline at end of file diff --git a/c/cert/test/rules/EXP16-C/test.c b/c/cert/test/rules/EXP16-C/test.c new file mode 100644 index 000000000..afc1b1b53 --- /dev/null +++ b/c/cert/test/rules/EXP16-C/test.c @@ -0,0 +1,113 @@ +#include + +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; +} \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll new file mode 100644 index 000000000..cc22a5ce0 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll @@ -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 = "rule" +} + +module Expressions2Package { + Query doNotCompareFunctionPointersToConstantValuesQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotCompareFunctionPointersToConstantValues` query + TQueryC(TExpressions2PackageQuery(TDoNotCompareFunctionPointersToConstantValuesQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index ae035b9c5..b574f7551 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -38,6 +38,7 @@ import Declarations9 import EssentialTypes import EssentialTypes2 import Expressions +import Expressions2 import FloatingTypes import FloatingTypes2 import FunctionTypes @@ -127,6 +128,7 @@ newtype TCQuery = TEssentialTypesPackageQuery(EssentialTypesQuery q) or TEssentialTypes2PackageQuery(EssentialTypes2Query q) or TExpressionsPackageQuery(ExpressionsQuery q) or + TExpressions2PackageQuery(Expressions2Query q) or TFloatingTypesPackageQuery(FloatingTypesQuery q) or TFloatingTypes2PackageQuery(FloatingTypes2Query q) or TFunctionTypesPackageQuery(FunctionTypesQuery q) or @@ -216,6 +218,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isEssentialTypesQueryMetadata(query, queryId, ruleId, category) or isEssentialTypes2QueryMetadata(query, queryId, ruleId, category) or isExpressionsQueryMetadata(query, queryId, ruleId, category) or + isExpressions2QueryMetadata(query, queryId, ruleId, category) or isFloatingTypesQueryMetadata(query, queryId, ruleId, category) or isFloatingTypes2QueryMetadata(query, queryId, ruleId, category) or isFunctionTypesQueryMetadata(query, queryId, ruleId, category) or diff --git a/cpp/common/src/codingstandards/cpp/types/Compatible.qll b/cpp/common/src/codingstandards/cpp/types/Compatible.qll index d6f65126e..56966b577 100644 --- a/cpp/common/src/codingstandards/cpp/types/Compatible.qll +++ b/cpp/common/src/codingstandards/cpp/types/Compatible.qll @@ -1,6 +1,7 @@ import cpp import codeql.util.Boolean import codingstandards.cpp.types.Graph +import codingstandards.cpp.types.FunctionType module TypeNamesMatchConfig implements TypeEquivalenceSig { predicate resolveTypedefs() { @@ -352,21 +353,3 @@ module FunctionDeclarationTypeEquivalence { ) } } - -/** - * Convenience class to reduce the awkwardness of how `RoutineType` and `FunctionPointerIshType` - * don't have a common ancestor. - */ -private class FunctionType extends Type { - FunctionType() { this instanceof RoutineType or this instanceof FunctionPointerIshType } - - Type getReturnType() { - result = this.(RoutineType).getReturnType() or - result = this.(FunctionPointerIshType).getReturnType() - } - - Type getParameterType(int i) { - result = this.(RoutineType).getParameterType(i) or - result = this.(FunctionPointerIshType).getParameterType(i) - } -} diff --git a/cpp/common/src/codingstandards/cpp/types/FunctionType.qll b/cpp/common/src/codingstandards/cpp/types/FunctionType.qll new file mode 100644 index 000000000..e43d1067f --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/types/FunctionType.qll @@ -0,0 +1,19 @@ +import cpp + +/** + * Convenience class to reduce the awkwardness of how `RoutineType` and `FunctionPointerIshType` + * don't have a common ancestor. + */ +class FunctionType extends Type { + FunctionType() { this instanceof RoutineType or this instanceof FunctionPointerIshType } + + Type getReturnType() { + result = this.(RoutineType).getReturnType() or + result = this.(FunctionPointerIshType).getReturnType() + } + + Type getParameterType(int i) { + result = this.(RoutineType).getParameterType(i) or + result = this.(FunctionPointerIshType).getParameterType(i) + } +} \ No newline at end of file diff --git a/rule_packages/c/Expressions2.json b/rule_packages/c/Expressions2.json new file mode 100644 index 000000000..88b5dff98 --- /dev/null +++ b/rule_packages/c/Expressions2.json @@ -0,0 +1,23 @@ +{ + "CERT-C": { + "EXP16-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Comparing function pointers to a constant value is not reliable and likely indicates a programmer error.", + "kind": "problem", + "name": "Do not compare function pointers to constant values", + "precision": "very-high", + "severity": "error", + "short_name": "DoNotCompareFunctionPointersToConstantValues", + "tags": [ + "correctness" + ] + } + ], + "title": "Do not compare function pointers to constant values" + } + } +} \ No newline at end of file From f6d62ecf55ec81dec636c3ce3d9fa2ead440aa3e Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Mon, 31 Mar 2025 13:09:44 -0700 Subject: [PATCH 08/10] Format --- cpp/common/src/codingstandards/cpp/types/FunctionType.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/types/FunctionType.qll b/cpp/common/src/codingstandards/cpp/types/FunctionType.qll index e43d1067f..20166322f 100644 --- a/cpp/common/src/codingstandards/cpp/types/FunctionType.qll +++ b/cpp/common/src/codingstandards/cpp/types/FunctionType.qll @@ -16,4 +16,4 @@ class FunctionType extends Type { result = this.(RoutineType).getParameterType(i) or result = this.(FunctionPointerIshType).getParameterType(i) } -} \ No newline at end of file +} From 067e1724a04df9eaaad348c4886a72e3276c9bd1 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Wed, 9 Apr 2025 20:39:32 -0700 Subject: [PATCH 09/10] Change obligation --- .../EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql | 2 +- .../src/codingstandards/cpp/exclusions/c/Expressions2.qll | 2 +- rule_packages/c/Expressions2.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql index 561374d66..88beae6bc 100644 --- a/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql +++ b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql @@ -8,7 +8,7 @@ * @problem.severity error * @tags external/cert/id/exp16-c * correctness - * external/cert/obligation/rule + * external/cert/obligation/recommendation */ import cpp diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll index cc22a5ce0..e7dffc30b 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Expressions2.qll @@ -13,7 +13,7 @@ predicate isExpressions2QueryMetadata(Query query, string queryId, string ruleId // `@id` for the `doNotCompareFunctionPointersToConstantValues` query "c/cert/do-not-compare-function-pointers-to-constant-values" and ruleId = "EXP16-C" and - category = "rule" + category = "recommendation" } module Expressions2Package { diff --git a/rule_packages/c/Expressions2.json b/rule_packages/c/Expressions2.json index 88b5dff98..7eaa2ff2e 100644 --- a/rule_packages/c/Expressions2.json +++ b/rule_packages/c/Expressions2.json @@ -2,7 +2,7 @@ "CERT-C": { "EXP16-C": { "properties": { - "obligation": "rule" + "obligation": "recommendation" }, "queries": [ { From 010b7c7d2b626dad88511d6292afc837eecd534d Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Wed, 9 Apr 2025 20:40:20 -0700 Subject: [PATCH 10/10] Remove commented out code --- ...NotCompareFunctionPointersToConstantValues.ql | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql index 88beae6bc..d9622b03b 100644 --- a/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql +++ b/c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql @@ -83,19 +83,3 @@ where function = funcExpr.getFunction() and funcName = funcExpr.getFuncName() select comparison, comparison.getExplanation(), function, funcName -//from -// EqualityOperation equality, FunctionExpr funcExpr, Element function, string funcName, -// Expr constantExpr -//where -// not isExcluded(equality, Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery()) and -// funcExpr = equality.getAnOperand() and -// constantExpr = equality.getAnOperand() and -// exists(constantExpr.getValue()) and -// function = funcExpr.getFunction() and -// funcName = funcExpr.getFuncName() and -// constantExpr.getFullyConverted().getUnderlyingType() = -// funcExpr.getFullyConverted().getUnderlyingType() -//select equality, -// "Pointer to function $@ compared to constant value." + -// constantExpr.getFullyConverted().getUnderlyingType().explain() + " / " + -// funcExpr.getFullyConverted().getUnderlyingType().explain(), function, funcName