Quantum: Add OpenSSL signature models #19705

bdrodes · 2025-06-09T19:09:04Z

No description provided.

…calls that return a known algorithm and calls that operate on a known algorithm. update KnownAlgorithmConstants to correct algType for signature algorithms. Update all instances and prior uses of the old mechanic to KnownAlgorithmConstants.

… that is an argument or a return.

…dd initial signature tests (no expected files yet). Add new openssl .h stubs. Clean up of OperationBase and associated uses. Update test case stubs to be closer to the actual stubs. Fix unncessary instanceof check in signatures.

…tep to handle paramgen. Remove redundant test. Overhaul of EVP update/initializer/final mechanics. Misc. updates for new API and refactoring EVPKeyGenOperation. Clean up of keygen_operaitons.ql.

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

… initializer subclasses.

…del update. Remove setting RSA bits as an RSA algorithm. Fix bug in hash algorithm. Add missing PKey encryption to cipher ops. Consolidate ctx initializers. Add unit tests, and alter unit test directory structure to allow for application to other APIs. Update expected files for unit tests (not all updated yet, a work in progress).

…properties that configure downstream operations. Add key size tests

…to disallow null key and IV on initializers (typically do not represent an actual key or IV).

…. Update model to account for MAC algorithms.

…n and correct handling of MACs.

… for now and noting the issue for a future PR.

nicolaswill

Did a brief review -- nothing semantic other than verifying that results look reasonable. There are other instances of the same code pattern issues I left some example comments for and open QL-for-QL alerts.

nicolaswill · 2025-06-15T13:50:24Z

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

@@ -29,7 +29,7 @@ predicate knownOpenSSLConstantToHashFamilyType(
      or
      name.matches(["SHA", "SHA1"]) and type instanceof Crypto::SHA1
      or
-      name.matches("SHA+%") and not name.matches(["SHA1", "SHA3-"]) and type instanceof Crypto::SHA2
+      name.matches("SHA_%") and not name.matches(["SHA1", "SHA3-"]) and type instanceof Crypto::SHA2


This is redundant. SHA_ cannot match SHA1 or SHA3.

The _ in matches means match one character, not any character (+ vs *), so it isn't redundant. The issue was I accidentally used the '+' syntax previously.

Ah! Four years of writing CodeQL, and I just learned that now...

Holds when the receiver matches the pattern. Patterns are matched by case sensitive string matching, and there are two wildcards: _ matches a single character, and % matches any sequence of characters.

Anyway, isn't _% redundant then? Just % should do the same thing.

I basically didn't want to match just "sha" and call it sha2. I wrote this awhile back so there is probably a better way, but basically it would not be correct to just use % because of that.

nicolaswill · 2025-06-15T13:51:54Z

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

-    this instanceof DirectAlgorithmValueConsumer and getterCall = this
+    this instanceof OpenSSLAlgorithmCall and
+    this instanceof DirectAlgorithmValueConsumer and
+    getterCall = this


Will this ever hold? This class does not extend OpenSSLAlgorithmValueConsumer.

Copy Pasta? I'll fix this.

nicolaswill · 2025-06-15T13:54:10Z

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

+    //ASSUMPTION: these cases will have operands for the call
+    not exists(this.(Call).getAnArgument()) and
+    exists(string name, string parsedTargetName |
+      parsedTargetName =


Seeing name normalization done outside of the model raises red flags for me. Does OpenSSL have the same algorithm specified in various places with either underscores or dashes? If so, that makes sense I guess.

However, why not change the matching logic for the OpenSSL-modelling-to-model-type mapping to have a wildcard on underscores or dashes rather than normalize it at the instance-modelling stage?

From prior investigations, it appeared openssl direct algorithm fetches use underscores, whereas the constants OpenSSL uses for algorithms uses dashes. The constants (the strings) are mapped to NID with dashes, so this normalization attempts to take function names and normalize them to the other kinds of constant strings. I'm open to other suggestions here but it is probably best suited for a separate PR.

nicolaswill · 2025-06-15T13:58:43Z

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

+    //TODO: this set will have to be exhaustive, and for each operation
+    //further modeling will be necessary for each case to map the APIs operands
+    //ASSUMPTION: these cases must have operands for the call
+    exists(this.(Call).getAnArgument()) and


Why does it need to have an argument? What would those arguments be? Quick evaluating doesn't really answer my question at first glance either, so could you please clarify the documentation?

Let me preface this by saying how we handle direct operation calls is something that will need some work.

But, what I've done so far is to distinguish direct fetching calls (e.g., something like AES_256(), which would return an algorithm container of some kind), from operation calls (e.g., AES_256_encrypt(....)). In the former, we never see arguments passed in because you are just getting an algorithm directly. For the latter, it is an assumed operation, meaning you have to pass a parameter somehow to operate on data.

Does that answer your question?

nicolaswill · 2025-06-15T13:59:02Z

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

+    exists(this.(Call).getAnArgument()) and
+    //TODO: Each case would be enumerated here. Will likely need an exhaustive mapping much like
+    // for known constants.
+    knownOpenSSLAlgorithmOperationCall(this, normalizedName, algType)


If there is no normalized name or algorithm type identified for this, the instance will simply not exist. We want to report known unknowns as well.

Per my comment above. How we handle direct operations needs a general revision. What we currently have is a POC for some of the work done with signatures, and even then it was partial. In the prior work (the PR we started from) example signatures were occurring using some direct operations. I want to open an issue to entirely refactor this, but for now it is a breadcrumb.

nicolaswill · 2025-06-15T14:08:57Z

cpp/ql/lib/experimental/quantum/OpenSSL/CtxFlow.qll

+private class CtxPointerReturn extends CtxPointerExpr {
+  CtxPointerReturn() { exists(Call c | c = this) }
+
+  Call getCall() { result = this.(Call) }


Suggested change

Call getCall() { result = this.(Call) }

Call getCall() { result = this }

nicolaswill · 2025-06-15T14:10:35Z

cpp/ql/lib/experimental/quantum/OpenSSL/KeyFlow.qll

+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(Call call | call.(Call).getAnArgument() = sink.asExpr())


Suggested change

exists(Call call | call.(Call).getAnArgument() = sink.asExpr())

exists(Call call | call.getAnArgument() = sink.asExpr())

nicolaswill · 2025-06-15T14:16:48Z

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

+  }
+}
+
+class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer {


Suggested change

class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer {

class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer instanceof Call {

If you follow the inheritance back up, it is already an instanceof call.

nicolaswill · 2025-06-15T14:16:56Z

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

+}
+
+class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer {
+  EVP_CipherInit_SKEY_Call() { this.(Call).getTarget().getName() in ["EVP_CipherInit_SKEY"] }


Suggested change

EVP_CipherInit_SKEY_Call() { this.(Call).getTarget().getName() in ["EVP_CipherInit_SKEY"] }

EVP_CipherInit_SKEY_Call() { super.getTarget().hasName("EVP_CipherInit_SKEY") }

I could do this with the extends call you mentioned above I suppose but it already does extend a call so it's all very odd I have to restate it. If you prefer that though I can do it, but I just wanted to bring to your attention it is already explicitly instanceof call.

nicolaswill · 2025-06-15T14:17:15Z

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

+class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer {
+  EVP_CipherInit_SKEY_Call() { this.(Call).getTarget().getName() in ["EVP_CipherInit_SKEY"] }
+
+  override Expr getKeyOperationSubtypeArg() { result = this.(Call).getArgument(5) }


Suggested change

override Expr getKeyOperationSubtypeArg() { result = this.(Call).getArgument(5) }

override Expr getKeyOperationSubtypeArg() { result = super.getArgument(5) }

See comments above.

…des/codeql into pawel_signatures_conversion

… Evp.

…nversion

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

bdrodes added 6 commits June 4, 2025 15:41

Crypto: Added Signature algorithm instance and consumer

952bc26

Crypto: Collapse initializer qll's into operations.

33e239d

Crypto: Update CtxFlow to flow from any "source ctx" which is any ctx…

f952f90

… that is an argument or a return.

Crypto: CtxFlow now uses an interface for additional steps. Add CTX s…

4f2045b

…tep to handle paramgen. Remove redundant test. Overhaul of EVP update/initializer/final mechanics. Misc. updates for new API and refactoring EVPKeyGenOperation. Clean up of keygen_operaitons.ql.

github-actions bot added the C++ label Jun 9, 2025

github-advanced-security bot found potential problems Jun 9, 2025

View reviewed changes

Crypto: Separate out CTX parameter initialization, and add additional…

729467c

… initializer subclasses.

nicolaswill changed the title ~~Pawel signatures conversion~~ Quantum: Add OpenSSL signature models (Pawel Platek) Jun 10, 2025

bdrodes added 8 commits June 10, 2025 13:37

Crypto: Add support to trace keys, add support to find prior key gen …

d3cff2d

…properties that configure downstream operations. Add key size tests

Crypto: Consolidate tests to use node, edges, and properties.

8f25380

Crypto: Overhaul/refactor of EVPInitialzers. Update cipher operation …

20e2c7c

…to disallow null key and IV on initializers (typically do not represent an actual key or IV).

Crypto: Further simplify test caes to only use edges/nodes/properties…

eb20955

…. Update model to account for MAC algorithms.

Crypto: Initial model of signatures. Still incomplete for verificatio…

cf2f0f1

…n and correct handling of MACs.

Crypto: Update expected files. There are failures, but accepting them…

fb495bf

… for now and noting the issue for a future PR.

Crypto: EVP Signature Operation cleanup.

1882db7

bdrodes marked this pull request as ready for review June 13, 2025 16:13

bdrodes requested review from a team as code owners June 13, 2025 16:13

bdrodes changed the title ~~Quantum: Add OpenSSL signature models (Pawel Platek)~~ Quantum: Add OpenSSL signature models Jun 13, 2025

Merge branch 'main' into pawel_signatures_conversion

db0bc47

nicolaswill requested changes Jun 15, 2025

View reviewed changes

nicolaswill and others added 6 commits June 16, 2025 14:57

Merge branch 'main' into pawel_signatures_conversion

f975428

Crypto: Code review cleanup.

45fa2c9

Merge branch 'pawel_signatures_conversion' of https://github.com/bdro…

90e480b

…des/codeql into pawel_signatures_conversion

Crypto: Acronym change from OpenSSL to OpenSsl, AVC to Avc and EVP to…

790a607

… Evp.

Crypto: Further ql-for-ql alert alert fixes.

7c18686

Crypto: Remove dead comments

6c9c969

bdrodes added 2 commits June 16, 2025 12:53

Crypto: Update JCA model to account for Model.qll changes.

2b6a832

Merge branch 'openssl_acronym_normalization' into pawel_signatures_co…

ac35634

…nversion

github-actions bot added the Java label Jun 17, 2025

github-advanced-security bot found potential problems Jun 17, 2025

View reviewed changes

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll Fixed Show fixed Hide fixed

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll Fixed Show fixed Hide fixed

bdrodes added 3 commits June 17, 2025 13:18

Crypto: QL docs fix.

f2f97c9

Crypto: Fix redundant override issue.

a81fffc

Merge branch 'main' into pawel_signatures_conversion

b2765a2

nicolaswill self-requested a review June 18, 2025 13:31

nicolaswill approved these changes Jun 18, 2025

View reviewed changes

nicolaswill merged commit 16c6411 into github:main Jun 18, 2025
39 checks passed

	Call getCall() { result = this.(Call) }
	Call getCall() { result = this }

	exists(Call call \| call.(Call).getAnArgument() = sink.asExpr())
	exists(Call call \| call.getAnArgument() = sink.asExpr())

	class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer {
	class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer instanceof Call {

	EVP_CipherInit_SKEY_Call() { this.(Call).getTarget().getName() in ["EVP_CipherInit_SKEY"] }
	EVP_CipherInit_SKEY_Call() { super.getTarget().hasName("EVP_CipherInit_SKEY") }

	override Expr getKeyOperationSubtypeArg() { result = this.(Call).getArgument(5) }
	override Expr getKeyOperationSubtypeArg() { result = super.getArgument(5) }

Quantum: Add OpenSSL signature models #19705

Quantum: Add OpenSSL signature models #19705

Uh oh!

Conversation

bdrodes commented Jun 9, 2025

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

nicolaswill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!