8000 Quantum: Add base classes for OpenSSL EVP methods by GrosQuildu · Pull Request #19607 · github/codeql · GitHub
[go: up one dir, main page]
Skip to content

Quantum: Add base classes for OpenSSL EVP methods #19607

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jun 3, 2025
Merged

Quantum: Add base classes for OpenSSL EVP methods #19607

merged 7 commits into from
Jun 3, 2025

Conversation

GrosQuildu
Copy link
Contributor
@GrosQuildu GrosQuildu commented May 28, 2025 •
edited
Loading

Changes OpenSSLOperation and adds base classes for OpenSSL's EVP API. Mostly a refactor to make existing code nicer.
The PR fixes some tests too.
Made on top of #19564

@Copilot Copilot AI review requested due to automatic review settings May 28, 2025 12:37
@GrosQuildu GrosQuildu requested review from a team as code owners May 28, 2025 12:37
@github-actions github-actions bot added the C++ label May 28, 2025
@GrosQuildu GrosQuildu changed the title Openssl base classes Quantum: Add base classes for OpenSSL EVP methods May 28, 2025
Copilot
Copilot AI reviewed May 28, 2025
View reviewed changes
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 adds foundational QL modeling for OpenSSL operations under the experimental quantum API and introduces corresponding library-tests for both hash and cipher operations.

  • Introduces base and specialized QL classes for EVP crypto operations (hash, cipher, keygen, etc.)
  • Adds new .ql queries under library-tests/quantum/openssl and their expected result files
  • Updates CODEOWNERS to include deeper experimental paths and imports EVPSignatureOperation

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cpp/ql/test/experimental/library-tests/quantum/openssl/*.ql New test queries for hash, cipher, key, and nonce sources
cpp/ql/test/experimental/library-tests/quantum/openssl/*.expected Expected output for the new test queries
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperations.qll Imported EVPSignatureOperation into main operations
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll Added context flow, algorithm consumer, and base methods
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVP*.qll Refactored individual EVP operation classes
cpp/ql/lib/experimental/quantum/OpenSSL/CtxFlow.qll Extended context type matching for CTX flow
CODEOWNERS Broadened experimental/quantum path pattern
Comments suppressed due to low confidence (3)

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperations.qll:5

  • You’ve added EVPSignatureOperation to the main operations import but there are no corresponding tests for signature flows. Consider adding library-tests for signature init, update, and final calls to ensure full coverage.
import EVPSignatureOperation

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll:111

  • [nitpick] The newly added abstract method getOutputArg lacks a doc comment. Adding a brief description will help maintainers understand its contract and intended use.
abstract Expr getOutputArg();

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/ECKeyGenOperation.qll:15

  • In getOutputKeyArtifact you’re using result.asExpr() whereas most other output artifacts use asDefiningArgument(). Verify that this correctly models the artifact’s definition point in the dataflow.
result.asExpr() = this.(Call).getArgument(0)

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPHashOperation.qll

override Crypto::ConsumerInputDataFlowNode getInputConsumer() { result = this.getInputNode() }
override Crypto::ArtifactOutputDataFlowNode getOutputArtifact() {
Copy link
Preview
Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

[nitpick] There’s repeated boilerplate in many EVP* classes for overriding getOutputArtifact and getInputConsumer just to call the super implementation. Consider moving those common overrides into a shared intermediate base to reduce duplication.

Copilot uses AI. Check for mistakes.

CODEOWNERS Outdated
@@ -16,7 +16,7 @@
/java/ql/test-kotlin2/ @github/codeql-kotlin

# Experimental CodeQL cryptography
**/experimental/quantum/ @github/ps-codeql
**/experimental/**/quantum/ @github/ps-codeql
Copy link
Preview
Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The new glob **/experimental/**/quantum/ may match more paths than intended. If you only want the QL modules under a specific directory, consider narrowing the pattern to avoid unintended CODEOWNERS assignments.

Suggested change
**/experimental/**/quantum/ @github/ps-codeql
/experimental/cryptography/quantum/ @github/ps-codeql

Copilot uses AI. Check for mistakes.

github-advanced-security[bot]
github-advanced-security bot found potential problems May 28, 2025
View reviewed changes
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll
@@ -1,21 +1,157 @@
private import experimental.quantum.Language
private import experimental.quantum.OpenSSL.CtxFlow as CTXFlow

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in CTXFlow should be PascalCase/camelCase.
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll
/**
* Final calls of EVP API.
*/
abstract class EVPFinal extends EVPOperation {

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in EVPFinal should be PascalCase/camelCase.
bdrodes
bdrodes reviewed May 28, 2025
View reviewed changes
10000
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 2, 2025
View reviewed changes
@GrosQuildu GrosQuildu force-pushed the openssl-base-classes branch from d3e1faa to 58a0a61 Compare June 3, 2025 13:11
nicolaswill
nicolaswill requested changes Jun 3, 2025
View reviewed changes
Copy link
Contributor
@nicolaswill nicolaswill left a comment

Choose a reason for hiding this comment

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

There are some QL for QL alerts to be resolved before merging. I would also recommend using super. rather than this.(Call). since the parent type is instanceof Call (if that works... I may be mistaken).

@nicolaswill
Copy link
Contributor

There are still several open QL for QL alerts other than the camelCase / PascalCase ones.

GrosQuildu and others added 7 commits June 3, 2025 16:27
@GrosQuildu
refactor EVP common classes
6d1b1d1 
add initial work for openssl signatures

add basic C test files for ciphers and signatures

more signature classes, comments for evp base classes

more signature tests

fix super calls for input consumers

fix getOutputArtifact for tests

formatting

delete redundant test files

move algorithm methods to OpenSSLOperation

refactor ECKeyGenOperation for new EVP classes

formatting

fix getOutputArtifact

fix cipher and digest operation test results

mv openssl signature to another PR
@GrosQuildu
fix openssl outputs
af8702d
@GrosQuildu @bdrodes
Update cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOper…
f103e8b 
…ationBase.qll

Co-authored-by: Ben Rodes <benjaminrodes@gmail.com>
@GrosQuildu @bdrodes
Apply docs suggestions
328cf79 
Co-authored-by: Ben Rodes <benjaminrodes@gmail.com>
@GrosQuildu
rm one-shot class
f04fa58
@GrosQuildu
remove redundant if/none
eff6eb3
@GrosQuildu
update docs
60d9b6e
@GrosQuildu GrosQuildu force-pushed the openssl-base-classes branch from 9491cee to 60d9b6e Compare June 3, 2025 14:27
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 3, 2025
View reviewed changes
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll
* These are not operations in the sense of Crypto::OperationInstance,
* but they are used to initialize the context for the operation.
*/
abstract class EVPInitialize extends Call {

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in EVPInitialize should be PascalCase/camelCase.
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll
* These are not operations in the sense of Crypto::OperationInstance,
* but they are used to update the context for the operation.
*/
abstract class EVPUpdate extends Call {

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in EVPUpdate should be PascalCase/camelCase.
cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll
* This captures one-shot APIs (with and without an initilizer call) and final calls.
* Provides some default methods for Crypto::KeyOperationInstance class
*/
abstract class EVPOperation extends OpenSSLOperation {

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in EVPOperation should be PascalCase/camelCase.
@nicolaswill nicolaswill self-requested a review June 3, 2025 17:23
@nicolaswill nicolaswill merged commit 0ef17ba into github:main Jun 3, 2025
13 checks passed
@GrosQuildu GrosQuildu deleted the openssl-base-classes branch June 4, 2025 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdrodes bdrodes bdrodes left review comments

Copilot code review Copilot Copilot left review comments

@nicolaswill nicolaswill nicolaswill approved these changes

Assignees
No one assigned
Labels
C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GrosQuildu @nicolaswill @bdrodes
0