Skip to content

CERT: Add query tags for "Risk Assessment" properties #896

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
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

lcartey
Copy link
Collaborator

@lcartey lcartey commented May 2, 2025

Description

This PR adds query tags for the five Risk Assessment properties which are specified on CERT rules:

  • Severity - "How serious are the consequences of the rule being ignored?"
  • Likelihood - "How likely is it that a flaw introduced by violating the rule can lead to an exploitable vulnerability?"
  • Remediation Cost - "How expensive is it to comply with the rule?"
  • Priorities - each of the three categories above are multiplied together to produce a "Priority" score from in range 1, 2, 3, 4, 6, 8, 9, 12, 18, and 27, where
  • Levels - priority scores are grouped into levels, with Level 1 being the highest priority and Level 3 being the lowest.

We add these properties to help users be able to select which queries to run based on the risk assessment properties.

In addition to the properties, we also add new query suites for L1, L2 and L3 rules. I think these are the most likely properties users will want to use, as they combine the three individual categories to create a blended priority score.

From an implementation perspective, this PR:

  • Adds a script for scraping the tags from the .md help files for the Risk Assessment tables, and adds them as tags to the rule package json files.
  • Adds extra validation checks to ensure these properties exist on all CERT queries, and do not exist on non-CERT queries.
  • Updates some markdown help files where the Risk Assessment section was missing header markings ##.
  • Updates all the queries to include the new tags.
  • Adds query suites cert-<lang>-<level>.qls. Note: these include the rules and not the recommendations, based on users likely wanting a more focused set of queries, not additional queries.

Change request type

  • Release or process automation (GitHub workflows, internal scripts)
  • Internal documentation
  • External documentation
  • Query files (.ql, .qll, .qls or unit tests)
  • External scripts (analysis report or other code shipped as part of a release)

Rules with added or modified queries

  • No rules added
  • Queries have been added for the following rules:
    • rule number here
  • Queries have been modified for the following rules:
    • All CERT rules

Release change checklist

A change note (development_handbook.md#change-notes) is required for any pull request which modifies:

  • The structure or layout of the release artifacts.
  • The evaluation performance (memory, execution time) of an existing query.
  • The results of an existing query in any circumstance.

If you are only adding new rule queries, a change note is not required.

Author: Is a change note required?

  • Yes
  • No

🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.

  • Confirmed

Reviewer: Confirm that either a change note is not required or the change note is required and has been added.

  • Confirmed

Query development review checklist

For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:

Author

  • Have all the relevant rule package description files been checked in?
  • Have you verified that the metadata properties of each new query is set appropriately?
  • Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
  • Are the alert messages properly formatted and consistent with the style guide?
  • Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
    As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
  • Does the query have an appropriate level of in-query comments/documentation?
  • Have you considered/identified possible edge cases?
  • Does the query not reinvent features in the standard library?
  • Can the query be simplified further (not golfed!)

Reviewer

  • Have all the relevant rule package description files been checked in?
  • Have you verified that the metadata properties of each new query is set appropriately?
  • Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
  • Are the alert messages properly formatted and consistent with the style guide?
  • Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
    As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
  • Does the query have an appropriate level of in-query comments/documentation?
  • Have you considered/identified possible edge cases?
  • Does the query not reinvent features in the standard library?
  • Can the query be simplified further (not golfed!)

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enriches all CERT C queries with detailed risk assessment tags (severity, likelihood, remediation-cost, priority, level) and introduces three new query suites for Level 1–3 rules.

  • Added risk assessment metadata tags to each CERT rule query.
  • Created cert-c-l1.qls, cert-c-l2.qls, and cert-c-l3.qls suites filtering by level.
  • Enhanced validation to ensure risk tags on CERT queries only and updated missing markdown headers.

Reviewed Changes

Copilot reviewed 317 out of 317 changed files in this pull request and generated 22 comments.

Show a summary per file
File Description
c/cert/src/rules/CON37-C/DoNotCallSignalInMultithreadedProgram.ql Added CERT risk assessment tags
c/cert/src/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql Added CERT risk assessment tags
c/cert/src/rules/CON35-C/DeadlockByLockingInPredefinedOrder.ql Added CERT risk assessment tags
c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql Added CERT risk assessment & recommendation tags
c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql Added CERT risk assessment & recommendation tags
c/cert/src/rules/CON33-C/RaceConditionsWhenUsingLibraryFunctions.ql Added CERT risk assessment tags
c/cert/src/rules/CON32-C/PreventDataRacesWithMultipleThreads.ql Added CERT risk assessment tags
c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.ql Added CERT risk assessment tags
c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql Added CERT risk assessment tags
c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql Added CERT risk assessment tags
c/cert/src/rules/ARR39-C/DoNotAddOrSubtractAScaledIntegerToAPointer.ql Added CERT risk assessment tags
c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.ql Added CERT risk assessment tags
c/cert/src/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.ql Added CERT risk assessment tags
c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql Added CERT risk assessment tags
c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql Added CERT risk assessment tags
c/cert/src/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.ql Added CERT risk assessment tags
c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.ql Added CERT risk assessment tags
c/cert/src/codeql-suites/cert-c-l3.qls New Level 3 suite (priority 1–4)
c/cert/src/codeql-suites/cert-c-l2.qls New Level 2 suite (priority 6–9)
c/cert/src/codeql-suites/cert-c-l1.qls New Level 1 suite (priority 12–27)
Comments suppressed due to low confidence (1)

c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql:12

  • This recommendation query already has the recommendation tag; ensure only the rule query is updated elsewhere.
*       external/cert/recommendation/con34-c

In some of the CERT help files they use "Recommendation" rather
than "Rule" as a header in the Risk Assessment table, creating
spurious query tags.
Copy link

@felickz felickz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

I had a similar PR with same tags - love the idea of additional suites to run rules as needed!

lcartey added 2 commits May 5, 2025 22:16
Deprecation warning lists location in query file, which has
changed due to addition of new tags.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants