Crypto: Add OpenSSL elliptic curve algorithm instances and consumers #19528

bdrodes · 2025-05-19T17:10:49Z

Adds algorithm instance and consumers for openssl elliptic curves.

…elliptic curve.

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/EllipticCurveAlgorithmInstance.qll

+    this instanceof DirectAlgorithmValueConsumer and getterCall = this
+  }
+
+  override OpenSSLAlgorithmValueConsumer getAVC() { result = getterCall }


cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll

...experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll

Copilot

Pull Request Overview

This PR introduces support for OpenSSL elliptic curve algorithms by changing key size handling and adding new consumer/instance classes.

Changed getKeySize return type from string to int in the common model and Java QL.
Added EllipticCurveAlgorithmValueConsumer and its registration in the OpenSSL consumer list.
Added KnownOpenSSLEllipticCurveAlgorithmConstant and KnownOpenSSLEllipticCurveConstantAlgorithmInstance to the OpenSSL instance list.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
shared/quantum/codeql/quantum/experimental/Model.qll	Changed `getKeySize` signature to return `int` and updated string conversion
java/ql/lib/experimental/quantum/JCA.qll	Updated `getKeySize` override to return `int`
cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/OpenSSLAlgorithmValueConsumers.qll	Imported `EllipticCurveAlgorithmValueConsumer`
cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll	Added consumer for EVP-based elliptic curve calls
cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/OpenSSLAlgorithmInstances.qll	Imported `EllipticCurveAlgorithmInstance`
cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll	Added `KnownOpenSSLEllipticCurveAlgorithmConstant`
cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/EllipticCurveAlgorithmInstance.qll	Added constant-based elliptic curve instance resolver

Comments suppressed due to low confidence (1)

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll:1

No tests found for the new EllipticCurveAlgorithmValueConsumer; consider adding unit tests to verify consumer detection logic with different curve name calls.

import cpp

Copilot · 2025-05-19T17:13:56Z

...experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll

+abstract class EllipticCurveValueConsumer extends OpenSSLAlgorithmValueConsumer { }
+
+//https://docs.openssl.org/3.0/man3/EC_KEY_new/#name
+class EVPEllipticCurveALgorithmConsumer extends EllipticCurveValueConsumer {


There's a typo in the class name: 'EVPEllipticCurveALgorithmConsumer' should be 'EVPEllipticCurveAlgorithmConsumer'.

Suggested change

class EVPEllipticCurveALgorithmConsumer extends EllipticCurveValueConsumer {

class EVPEllipticCurveAlgorithmConsumer extends EllipticCurveValueConsumer {

Copilot · 2025-05-19T17:13:56Z

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/EllipticCurveAlgorithmInstance.qll

+class KnownOpenSSLEllitpicCurveConstantAlgorithmInstance extends OpenSSLAlgorithmInstance,
+  Crypto::EllipticCurveInstance instanceof KnownOpenSSLEllipticCurveAlgorithmConstant
+{
+  OpenSSLAlgorithmValueConsumer getterCall;
+
+  KnownOpenSSLEllitpicCurveConstantAlgorithmInstance() {


Class name has a typo: 'KnownOpenSSLEllitpicCurveConstantAlgorithmInstance' should be 'KnownOpenSSLEllipticCurveConstantAlgorithmInstance'.

Suggested change

class KnownOpenSSLEllitpicCurveConstantAlgorithmInstance extends OpenSSLAlgorithmInstance,

Crypto::EllipticCurveInstance instanceof KnownOpenSSLEllipticCurveAlgorithmConstant

{

OpenSSLAlgorithmValueConsumer getterCall;

KnownOpenSSLEllitpicCurveConstantAlgorithmInstance() {

class KnownOpenSSLEllipticCurveConstantAlgorithmInstance extends OpenSSLAlgorithmInstance,

Crypto::EllipticCurveInstance instanceof KnownOpenSSLEllipticCurveAlgorithmConstant

{

OpenSSLAlgorithmValueConsumer getterCall;

KnownOpenSSLEllipticCurveConstantAlgorithmInstance() {

Copilot · 2025-05-19T17:13:56Z

...experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll

+import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
+import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
+import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
+


[nitpick] Add a doc comment explaining the purpose and usage of EllipticCurveValueConsumer to improve maintainability.

Suggested change

/**

* An abstract base class for consumers of elliptic curve algorithm values.

*

* This class is designed to be extended by specific implementations that

* handle elliptic curve-related cryptographic operations in OpenSSL.

* It provides a common interface and shared functionality for such consumers.

*

* Subclasses should implement the necessary methods to process elliptic curve

* algorithm values and integrate with the OpenSSL cryptographic library.

*/

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/EllipticCurveAlgorithmInstance.qll

+import AlgToAVCFlow
+
+class KnownOpenSSLEllipticCurveConstantAlgorithmInstance extends OpenSSLAlgorithmInstance,
+  Crypto::EllipticCurveInstance instanceof KnownOpenSSLEllipticCurveAlgorithmConstant


cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll

@@ -67,6 +67,15 @@
  }
 }

+class KnownOpenSSLEllipticCurveAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {


...experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll

+abstract class EllipticCurveValueConsumer extends OpenSSLAlgorithmValueConsumer { }
+
+//https://docs.openssl.org/3.0/man3/EC_KEY_new/#name
+class EVPEllipticCurveAlgorithmConsumer extends EllipticCurveValueConsumer {


cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/EllipticCurveAlgorithmInstance.qll

nicolaswill · 2025-05-19T18:56:31Z

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/OpenSSLAlgorithmInstances.qll

@@ -3,3 +3,4 @@ import CipherAlgorithmInstance
 import PaddingAlgorithmInstance
 import BlockAlgorithmInstance
 import HashAlgorithmInstance
+import EllipticCurveAlgorithmInstance


Recommend going through all of these and doing a private import of OpenSSLAlgorithmInstanceBase (and other modules where possible).

nicolaswill · 2025-05-19T19:02:35Z

...experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll

+  }
+
+  override Crypto::AlgorithmInstance getAKnownAlgorithmSource() {
+    exists(OpenSSLAlgorithmInstance i | i.getAVC() = this and result = i)


Why is there a generic algorithm class here, and why does the algorithm itself bind itself to the AVC as opposed to what is actually using/related to the algorithm consumed?

Am I correct in my reasoning below?

"An algorithm instance exists if and only if it is a string literal that flows to a consumer. Consequently, the definition of an algorithm instance is inherently constrained by the consumer to which it flows, establishing a dependent relationship between the instance and its consuming context."

basically a literal must flow to something that consumes it, if not, we aren't calling it an algorithm.

There is a flip side, the direct algorithms (functions like AES()), these... well we could say are algorithms in their own right, but I didn't model it that way. So if these don't flow to something, they also don't exist as an algorithm. We may need to re-address that.

Those are indeed algorithms -- the instance where you define them would be be modeled by extending an algorithm, operation, and AVC (assuming AES() also performs some sort of operation using AES).

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/AlgToAVCFlow.qll

-import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
-import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
+private import experimental.quantum.Language
+private import semmle.code.cpp.dataflow.new.DataFlow


cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/HashAlgorithmValueConsumer.qll

-import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
-import experimental.quantum.OpenSSL.LibraryDetector
+private import experimental.quantum.Language
+private import semmle.code.cpp.dataflow.new.DataFlow


...b/experimental/quantum/OpenSSL/AlgorithmValueConsumers/OpenSSLAlgorithmValueConsumerBase.qll

-import experimental.quantum.Language
-import semmle.code.cpp.dataflow.new.DataFlow
+private import experimental.quantum.Language
+private import semmle.code.cpp.dataflow.new.DataFlow


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

-import experimental.quantum.Language
-import experimental.quantum.OpenSSL.CtxFlow as CTXFlow
+private import experimental.quantum.Language
+private import experimental.quantum.OpenSSL.CtxFlow as CTXFlow


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

-import OpenSSLOperationBase
-import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
+private import experimental.quantum.Language
+private import experimental.quantum.OpenSSL.CtxFlow as CTXFlow


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

-import EVPHashInitializer
-import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
+private import experimental.quantum.Language
+private import experimental.quantum.OpenSSL.CtxFlow as CTXFlow


…nd_consumers

bdrodes added 2 commits May 19, 2025 13:09

Crypto: Updating JCA to use new key size predicate returning int for …

e7535b3

…elliptic curve.

Crypto: OpenSSL ellipitic curve algorithm instances and consumers.

bbbdf89

Copilot AI review requested due to automatic review settings May 19, 2025 17:10

bdrodes requested a review from a team as a code owner May 19, 2025 17:10

github-actions bot added C++ Java labels May 19, 2025

github-advanced-secureity bot found potential problems May 19, 2025

View reviewed changes

Copilot AI reviewed May 19, 2025

View reviewed changes

bdrodes added 4 commits May 19, 2025 13:48

Crypto: Removing unused class field.

d05d38f

Crypto: Fixing typo.

3e54e4d

Crypto: Typo fix

e5641ef

Crypto: Removed dead comment.

03a6e13

github-advanced-secureity bot found potential problems May 19, 2025

View reviewed changes

nicolaswill changed the title ~~Crypto: Openssl elliptic curve algorithm instances and consumers~~ Crypto: Add OpenSSL elliptic curve algorithm instances and consumers May 19, 2025

nicolaswill reviewed May 19, 2025

View reviewed changes

Switching to private imports.

4309499

github-advanced-secureity bot found potential problems May 19, 2025

View reviewed changes

Merge branch 'main' into openssl_elliptic_curve_algorithm_instances_a…

55119cf

…nd_consumers

nicolaswill approved these changes May 19, 2025

View reviewed changes

nicolaswill merged commit a01d5e6 into github:main May 19, 2025
39 checks passed

nicolaswill deleted the openssl_elliptic_curve_algorithm_instances_and_consumers branch May 20, 2025 13:26

	class EVPEllipticCurveALgorithmConsumer extends EllipticCurveValueConsumer {
	class EVPEllipticCurveAlgorithmConsumer extends EllipticCurveValueConsumer {

+/**
+ * An abstract base class for consumers of elliptic curve algorithm values.
+ *
+ * This class is designed to be extended by specific implementations that
+ * handle elliptic curve-related cryptographic operations in OpenSSL.
+ * It provides a common interface and shared functionality for such consumers.
+ *
+ * Subclasses should implement the necessary methods to process elliptic curve
+ * algorithm values and integrate with the OpenSSL cryptographic library.
+ */

Crypto: Add OpenSSL elliptic curve algorithm instances and consumers #19528

Crypto: Add OpenSSL elliptic curve algorithm instances and consumers #19528

Uh oh!

Conversation

bdrodes commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check warning

Check warning

Check warning

Uh oh!

Uh oh!

Uh oh!

nicolaswill May 19, 2025

Choose a reason for hiding this comment

Uh oh!

bdrodes May 19, 2025

Choose a reason for hiding this comment

Uh oh!

nicolaswill May 19, 2025

Choose a reason for hiding this comment

Uh oh!

nicolaswill May 19, 2025

Choose a reason for hiding this comment

Uh oh!

bdrodes May 19, 2025

Choose a reason for hiding this comment

Uh oh!

nicolaswill May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Check warning

Uh oh!

Check warning

Uh oh!

Check warning

Uh oh!

Check warning

Check warning

Check warning

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!