ci: add CodeQL query to enforce OpenSSL return code handling (#1867)

[Vishnu2707]

Closes #1867

What this PR does

Adds a custom CodeQL query that detects calls to OpenSSL EVP_*
functions whose return value is not guarded by the OQS_OPENSSL_GUARD
macro. Integrates the query into a new CodeQL workflow that runs on
every PR and push to main.

Files added

• .github/codeql/openssl-return-check.ql — custom CodeQL query
• .github/workflows/codeql.yml — CodeQL workflow

Files modified

• .github/workflows/pr.yml — adds codeql job to PR checks

Background

Trail of Bits identified unchecked EVP_* return values during their
audit of liboqs (issue #1862, fixed in #1869). This query makes that
class of bug permanently detectable in CI, preventing new instances
from being introduced.

The query was originally written by Trail of Bits and provided in
issue #1867.

Checklist

• Does this PR change the input/output behaviour of a cryptographic algorithm? No
• Does this PR change the list of algorithms available? No

[coveralls]

coverage: 82.346% (+0.01%) from 82.334% — Vishnu2707:codeql-openssl-return-check into open-quantum-safe:main

[Vishnu2707]

The query runs successfully, CodeQL scanned 200 C files with no errors. The remaining failure is CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled, which indicates the repo has CodeQL default setup active in settings. This conflicts with the custom workflow SARIF upload. @dstebila @baentsch, would it be possible to disable default setup in the repository's Code Security settings to allow the custom workflow to run, or should the workflow be restructured to work alongside default setup?

[baentsch]

Hi @Vishnu2707 Thanks very much for the PR and helping us reduce our backlog!

or should the workflow be restructured to work alongside default setup?

The latter: We want to keep the current checks. Copilot suggested this change (plus, removal of the new codeql-config.yml from the PR):

- name: Initialize CodeQL
  uses: github/codeql-action/init@4e828ff8d448a8a6e532957b1811f387a63867e8 # v3
  with:
    languages: cpp
    queries: security-and-quality,./.github/codeql/openssl-return-check.ql

Could you please try this / whether this gets the PR across the finish line?

[xuganyu96]

I took a look at the logs of a workflow run but had a hard time figuring out where I should look for positive/negative output, especially with regard to the pattern presented in this pull request.

Could you please provide some documentation on where the maintainers should look for "CodeQL catching bad examples matching this pattern?" This could be a section in CONTRIBUTING.md, or a "expect-fail" test case in pytest/GitHub Actions. Thank you.

[xuganyu96]

Could you please try this / whether this gets the PR across the finish line?

@baentsch it looks like your suggestion has been implemented. Could you please review this pull request? Thank you.

[baentsch]

Could you please try this / whether this gets the PR across the finish line?

@baentsch it looks like your suggestion has been implemented. Could you please review this pull request? Thank you.

Indeed, it has. Thanks for the heads up @xuganyu96 and to @Vishnu2707 for the improvement and closing a long-standing issue! So I'm approving, too, but would like to support this suggestion

Could you please provide some documentation on where the maintainers should look for "CodeQL catching bad examples matching this pattern?" This could be a section in CONTRIBUTING.md, or a "expect-fail" test case in pytest/GitHub Actions.

as a further improvement to the PR before merge. Would you be able to add something like that @Vishnu2707 ? Otherwise, we'll blindly trust the CI to flag errors as and when they occur/get re-introduced :)

[baentsch]

LGTM.

[baentsch]

Thanks for the negative test & documentation update @Vishnu2707 ! Ready to merge if you agree @xuganyu96 (and CI passes, of course),

[dstebila]

Could you please provide some documentation on where the maintainers should look for "CodeQL catching bad examples matching this pattern?" This could be a section in CONTRIBUTING.md, or a "expect-fail" test case in pytest/GitHub Actions.

as a further improvement to the PR before merge. Would you be able to add something like that @Vishnu2707 ? Otherwise, we'll blindly trust the CI to flag errors as and when they occur/get re-introduced :)

I'm still a little confused about where to look for errors resulting from this.

[Vishnu2707]

I'm still a little confused about where to look for errors resulting from this.

@dstebila - The bad example is at .github/codeql/test/openssl-return-check-bad.c. if CodeQL is running correctly, that file should produce an alert for the unchecked EVP_DigestInit_ex call. For day-to-day use, any new unchecked EVP_* call introduced in a PR will show up as a code scanning alert blocking merge.

[xuganyu96]

This doesn't look exactly right. With upload: false, the analyze step will not upload the scanning results to Code Scanning. I tried running it with upload set to true (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.yml workflow. 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:

• Upload was rejected because CodeQL default setup is enabled for code scanning

[Vishnu2707]

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.