-
Notifications
You must be signed in to change notification settings - Fork 759
ci: add CodeQL query to enforce OpenSSL return code handling (#1867) #2415
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
Vishnu2707
wants to merge
9
commits into
open-quantum-safe:main
Choose a base branch
from
Vishnu2707:codeql-openssl-return-check
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.
+84
−0
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
5804e50
ci: add CodeQL query to enforce OpenSSL return code handling
Vishnu2707 feb12ad
ci: move security-events permission to job level in CodeQL workflow
Vishnu2707 4236b26
ci: fix CodeQL query path using config file
Vishnu2707 fbb718c
ci: disable CodeQL TRAP caching to fix pre-finalize crash
Vishnu2707 43313b3
ci: use inline queries field per maintainer suggestion
Vishnu2707 75bee09
ci: re-add TRAP caching workaround after queries field change
Vishnu2707 1d1048b
ci: disable SARIF upload to avoid default setup conflict
Vishnu2707 7846e61
ci: add CodeQL query test example and CONTRIBUTING docs
Vishnu2707 81255c4
ci: add SPDX header to CodeQL test file
Vishnu2707 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
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 @@ | ||
| /** | ||
| * @name Unchecked OpenSSL EVP return value | ||
| * @description Calls to OpenSSL EVP functions whose return value is not | ||
| * checked by the OQS_OPENSSL_GUARD macro may silently ignore | ||
| * errors, leading to undefined behaviour. | ||
| * @kind problem | ||
| * @problem.severity warning | ||
| * @precision medium | ||
| * @id cpp/openssl-unchecked-return | ||
| * @tags security | ||
| * correctness | ||
| */ | ||
|
|
||
| import cpp | ||
|
|
||
| from FunctionCall call, Function f | ||
| where | ||
| f = call.getTarget() and | ||
| f.getName().matches("EVP%") and | ||
| not f.getType() instanceof PointerType and | ||
| not f.getType() instanceof VoidType and | ||
| not exists(MacroAccess m | | ||
| m.getLocation().subsumes(call.getLocation()) and | ||
| m.getMacroName() = "OQS_OPENSSL_GUARD" | ||
| ) | ||
| select call, "Return value of " + f.getName() + "() is not checked by OQS_OPENSSL_GUARD." |
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,6 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| /* expect-fail: EVP return value not checked via OQS_OPENSSL_GUARD */ | ||
| #include <openssl/evp.h> | ||
| void bad_example(EVP_MD_CTX *ctx, const EVP_MD *md) { | ||
| EVP_DigestInit_ex(ctx, md, NULL); | ||
| } |
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,36 @@ | ||
| name: CodeQL analysis | ||
| permissions: | ||
| contents: read | ||
| on: | ||
| workflow_call: | ||
| workflow_dispatch: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| branches: [main] | ||
| jobs: | ||
| codeql: | ||
| permissions: | ||
| contents: read | ||
| security-events: write | ||
| env: | ||
| CODEQL_EXTRACTOR_CPP_TRAP_CACHING: false | ||
| name: CodeQL | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 | ||
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@4e828ff8d448a8a6e532957b1811f387a63867e8 # v3 | ||
| with: | ||
| languages: cpp | ||
| queries: security-and-quality,./.github/codeql/openssl-return-check.ql | ||
| - name: Build liboqs | ||
| run: | | ||
| cmake -S . -B build -DOQS_MINIMAL_BUILD="KEM_ml_kem_768;SIG_ml_dsa_65" | ||
| cmake --build build --parallel $(nproc) | ||
| - name: Perform CodeQL analysis | ||
| uses: github/codeql-action/analyze@4e828ff8d448a8a6e532957b1811f387a63867e8 # v3 | ||
| with: | ||
| category: "/language:cpp" | ||
| upload: false | ||
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
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
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.
This doesn't look exactly right. With
upload: false, theanalyzestep will not upload the scanning results to Code Scanning. I tried running it withuploadset totrue(and with default CodeQL setup disabled), the workflow reported successful upload, but I still could not find the corresponding entries in Code Scanning.The liboqs repository currently enables the default CodeQL setup, which blocks code scanning results uploads from REST API (i.e. GitHub Actions) and CodeQL CLI. If we want to proceed with adding custom CodeQL queries, the default setup must be disabled, and to preserve existing code scanning we will need to manually add a
codeql.ymlworkflow. Given how this touches privileged settings of the liboqs repository, I'm afraid that configuring custom CodeQL queries is beyond the authorization of external contributors.cc: @dstebila
References:
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.
Thanks @xuganyu96 for checking this out. Disabling the default CodeQL setup needs admin access so that's on your end. @dstebila @baentsch let me know how you want to proceed, happy either way.