Skip to content

Quantum: Add initial qltests for OpenSSL modeling #19564

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Quantum: Add initial qltests for OpenSSL modeling #19564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41f008d
Crypto: Adding initial openssl tests, fixing a bug in hash modeling f…
bdrodes May 22, 2025
b1ce44e
Crypto: Move openssl stubs to a shared stubs location. Include openss…
bdrodes May 28, 2025
69e3a20
Crypto: Update crypto stubs location under 'crypto' and associate cod…
bdrodes May 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
/java/ql/test-kotlin2/ @github/codeql-kotlin

# Experimental CodeQL cryptography
**/experimental/quantum/ @github/ps-codeql
**/experimental/**/quantum/ @github/ps-codeql
/shared/quantum/ @github/ps-codeql
**/test/stubs/crypto/ @github/ps-codeql

# CodeQL tools and associated docs
/docs/codeql/codeql-cli/ @github/codeql-cli-reviewers
Expand Down
11 changes: 3 additions & 8 deletions ...l/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/HashAlgorithmValueConsumer.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@
private import semmle.code.cpp.dataflow.new.DataFlow
private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
private import experimental.quantum.OpenSSL.LibraryDetector

abstract class HashAlgorithmValueConsumer extends OpenSSLAlgorithmValueConsumer { }

/**
* EVP_Q_Digest directly consumes algorithm constant values
*/
class EVP_Q_Digest_Algorithm_Consumer extends OpenSSLAlgorithmValueConsumer {
EVP_Q_Digest_Algorithm_Consumer() {
isPossibleOpenSSLFunction(this.(Call).getTarget()) and
this.(Call).getTarget().getName() = "EVP_Q_digest"
}
class EVP_Q_Digest_Algorithm_Consumer extends HashAlgorithmValueConsumer {
EVP_Q_Digest_Algorithm_Consumer() { this.(Call).getTarget().getName() = "EVP_Q_digest" }

override Crypto::ConsumerInputDataFlowNode getInputNode() {
result.asExpr() = this.(Call).getArgument(1)
Expand All @@ -35,13 +31,12 @@
* The EVP digest algorithm getters
* https://docs.openssl.org/3.0/man3/EVP_DigestInit/#synopsis
*/
class EVPDigestAlgorithmValueConsumer extends OpenSSLAlgorithmValueConsumer {
class EVPDigestAlgorithmValueConsumer extends HashAlgorithmValueConsumer {

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in EVPDigestAlgorithmValueConsumer should be PascalCase/camelCase.
DataFlow::Node valueArgNode;
DataFlow::Node resultNode;

EVPDigestAlgorithmValueConsumer() {
resultNode.asExpr() = this and
isPossibleOpenSSLFunction(this.(Call).getTarget()) and
(
this.(Call).getTarget().getName() in [
"EVP_get_digestbyname", "EVP_get_digestbynid", "EVP_get_digestbyobj"
Expand Down
14 changes: 13 additions & 1 deletion cpp/ql/lib/experimental/quantum/OpenSSL/CtxFlow.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,19 @@ import semmle.code.cpp.dataflow.new.DataFlow
* - EVP_PKEY_CTX
*/
private class CtxType extends Type {
CtxType() { this.getUnspecifiedType().stripType().getName().matches("evp_%ctx_%st") }
CtxType() {
// It is possible for users to use the underlying type of the CTX variables
// these have a name matching 'evp_%ctx_%st
this.getUnspecifiedType().stripType().getName().matches("evp_%ctx_%st")
Copy link
Contributor

@nicolaswill nicolaswill May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For a C team reviewer: I am curious about necessity of the logic in this charpred (see comment about "build mode none issues" on the lines below).

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you just being defensive here, or have you seen these issues on real world databases? To me it looks very unlikely that the case below can actually occur, because everything here is coming from the OpenSSL headers, and if it could not find one of them, it probably would not have been able to find any of them.

In general we have only been introducing workarounds like the below when we saw many issues arising from a missing definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is in fact only needed for your test to work, then I would strongly suggest to rewrite the test.

Copy link
Contributor Author

@bdrodes bdrodes May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is a defensive move. The build mode none work has me very nervous now with respect to tying anything to resolution of library headers (perhaps I'm wrong). The combination of the two approaches in this CTX type class should handle all cases (e.g., the user uses the underlying _st type, the user uses the typedef, the header is found, the header isn't). The entire modeling approach is very brittle if the CTX underlying type isn't detected (the entire inventory for openSSL will fail if CTX isn't located correctly), so I'm inclined to be very very robust here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion in the comment. This is not a hypothetical case but a real-world one observed on Microsoft DBs, and the test-case reflects that pattern. With a cryptography inventory, one small failure in determining the existence of an asset will damage the credibility of the inventory we produce, so we want to be exhaustive in addressing known shortcomings proactively.

or
// In principal the above check should be sufficient, but in case of build mode none issues
// i.e., if a typedef cannot be resolved,
// or issues with properly stubbing test cases, we also explicitly check for the wrapping type defs
// i.e., patterns matching 'EVP_%_CTX'
exists(Type base | base = this or base = this.(DerivedType).getBaseType() |
base.getName().matches("EVP_%_CTX")
)
}
}

/**
Expand Down
4 changes: 3 additions & 1 deletion cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPCipherOperation.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ private module AlgGetterToAlgConsumerConfig implements DataFlow::ConfigSig {
}

predicate isSink(DataFlow::Node sink) {
exists(EVP_Cipher_Operation c | c.getInitCall().getAlgorithmArg() = sink.asExpr())
exists(EVP_Cipher_Operation c | c.getAlgorithmArg() = sink.asExpr())
}
}

Expand All @@ -32,6 +32,8 @@ private module AlgGetterToAlgConsumerFlow = DataFlow::Global<AlgGetterToAlgConsu
abstract class EVP_Cipher_Operation extends OpenSSLOperation, Crypto::KeyOperationInstance {
Expr getContextArg() { result = this.(Call).getArgument(0) }

Expr getAlgorithmArg() { this.getInitCall().getAlgorithmArg() = result }

override Expr getOutputArg() { result = this.(Call).getArgument(1) }

override Crypto::KeyOperationSubtype getKeyOperationSubtype() {
Expand Down
15 changes: 8 additions & 7 deletions cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPHashOperation.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgor
abstract class EVP_Hash_Operation extends OpenSSLOperation, Crypto::HashOperationInstance {
Expr getContextArg() { result = this.(Call).getArgument(0) }

Expr getAlgorithmArg() { result = this.getInitCall().getAlgorithmArg() }

EVP_Hash_Initializer getInitCall() {
CTXFlow::ctxArgFlowsToCtxArg(result.getContextArg(), this.getContextArg())
}
Expand All @@ -23,7 +25,7 @@ abstract class EVP_Hash_Operation extends OpenSSLOperation, Crypto::HashOperatio
*/
override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
AlgGetterToAlgConsumerFlow::flow(result.(OpenSSLAlgorithmValueConsumer).getResultNode(),
DataFlow::exprNode(this.getInitCall().getAlgorithmArg()))
DataFlow::exprNode(this.getAlgorithmArg()))
}
}

Expand All @@ -33,7 +35,7 @@ private module AlgGetterToAlgConsumerConfig implements DataFlow::ConfigSig {
}

predicate isSink(DataFlow::Node sink) {
exists(EVP_Hash_Operation c | c.getInitCall().getAlgorithmArg() = sink.asExpr())
exists(EVP_Hash_Operation c | c.getAlgorithmArg() = sink.asExpr())
}
}

Expand Down Expand Up @@ -64,6 +66,8 @@ class EVP_Q_Digest_Operation extends EVP_Hash_Operation {
// simply return 'this', see modeled hash algorithm consuers for EVP_Q_Digest
this = result
}

override Expr getAlgorithmArg() { result = this.(Call).getArgument(1) }
}

class EVP_Digest_Operation extends EVP_Hash_Operation {
Expand All @@ -72,17 +76,14 @@ class EVP_Digest_Operation extends EVP_Hash_Operation {
// There is no context argument for this function
override Expr getContextArg() { none() }

override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
AlgGetterToAlgConsumerFlow::flow(result.(OpenSSLAlgorithmValueConsumer).getResultNode(),
DataFlow::exprNode(this.(Call).getArgument(4)))
}

override EVP_Hash_Initializer getInitCall() {
// This variant of digest does not use an init
// and even if it were used, the init would be ignored/undefined
none()
}

override Expr getAlgorithmArg() { result = this.(Call).getArgument(4) }

override Expr getOutputArg() { result = this.(Call).getArgument(2) }

override Expr getInputArg() { result = this.(Call).getArgument(0) }
Expand Down
2 changes: 2 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_key_sources.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
| openssl_basic.c:40:13:40:31 | EncryptOperation | openssl_basic.c:31:49:31:51 | Key | openssl_basic.c:179:43:179:76 | Constant |
| openssl_basic.c:90:11:90:29 | DecryptOperation | openssl_basic.c:77:45:77:47 | Key | openssl_basic.c:179:43:179:76 | Constant |
6 changes: 6 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_key_sources.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import cpp
import experimental.quantum.Language

from Crypto::CipherOperationNode op, Crypto::KeyArtifactNode k
where op.getAKey() = k
select op, k, k.getSourceNode()
2 changes: 2 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_nonce_sources.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
| openssl_basic.c:40:13:40:31 | EncryptOperation | openssl_basic.c:31:54:31:55 | Nonce | openssl_basic.c:180:42:180:59 | Constant |
| openssl_basic.c:90:11:90:29 | DecryptOperation | openssl_basic.c:77:50:77:51 | Nonce | openssl_basic.c:180:42:180:59 | Constant |
6 changes: 6 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_nonce_sources.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import cpp
import experimental.quantum.Language

from Crypto::CipherOperationNode op, Crypto::NonceArtifactNode n
where op.getANonce() = n
select op, n, n.getSourceNode()
8 changes: 8 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_operations.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
| openssl_basic.c:40:13:40:31 | EncryptOperation | openssl_basic.c:35:54:35:62 | Message | openssl_basic.c:40:13:40:31 | KeyOperationOutput | openssl_basic.c:23:62:23:65 | Key | openssl_basic.c:23:68:23:71 | Nonce | openssl_basic.c:23:37:23:51 | KeyOperationAlgorithm | Encrypt |
| openssl_basic.c:40:13:40:31 | EncryptOperation | openssl_basic.c:35:54:35:62 | Message | openssl_basic.c:40:13:40:31 | KeyOperationOutput | openssl_basic.c:23:62:23:65 | Key | openssl_basic.c:31:54:31:55 | Nonce | openssl_basic.c:23:37:23:51 | KeyOperationAlgorithm | Encrypt |
| openssl_basic.c:40:13:40:31 | EncryptOperation | openssl_basic.c:35:54:35:62 | Message | openssl_basic.c:40:13:40:31 | KeyOperationOutput | openssl_basic.c:31:49:31:51 | Key | openssl_basic.c:23:68:23:71 | Nonce | openssl_basic.c:23:37:23:51 | KeyOperationAlgorithm | Encrypt |
| openssl_basic.c:40:13:40:31 | EncryptOperation | openssl_basic.c:35:54:35:62 | Message | openssl_basic.c:40:13:40:31 | KeyOperationOutput | openssl_basic.c:31:49:31:51 | Key | openssl_basic.c:31:54:31:55 | Nonce | openssl_basic.c:23:37:23:51 | KeyOperationAlgorithm | Encrypt |
| openssl_basic.c:90:11:90:29 | DecryptOperation | openssl_basic.c:81:49:81:58 | Message | openssl_basic.c:90:11:90:29 | KeyOperationOutput | openssl_basic.c:69:58:69:61 | Key | openssl_basic.c:69:64:69:67 | Nonce | openssl_basic.c:69:33:69:47 | KeyOperationAlgorithm | Decrypt |
| openssl_basic.c:90:11:90:29 | DecryptOperation | openssl_basic.c:81:49:81:58 | Message | openssl_basic.c:90:11:90:29 | KeyOperationOutput | openssl_basic.c:69:58:69:61 | Key | openssl_basic.c:77:50:77:51 | Nonce | openssl_basic.c:69:33:69:47 | KeyOperationAlgorithm | Decrypt |
| openssl_basic.c:90:11:90:29 | DecryptOperation | openssl_basic.c:81:49:81:58 | Message | openssl_basic.c:90:11:90:29 | KeyOperationOutput | openssl_basic.c:77:45:77:47 | Key | openssl_basic.c:69:64:69:67 | Nonce | openssl_basic.c:69:33:69:47 | KeyOperationAlgorithm | Decrypt |
| openssl_basic.c:90:11:90:29 | DecryptOperation | openssl_basic.c:81:49:81:58 | Message | openssl_basic.c:90:11:90:29 | KeyOperationOutput | openssl_basic.c:77:45:77:47 | Key | openssl_basic.c:77:50:77:51 | Nonce | openssl_basic.c:69:33:69:47 | KeyOperationAlgorithm | Decrypt |
6 changes: 6 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_operations.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import cpp
import experimental.quantum.Language

from Crypto::CipherOperationNode n
select n, n.getAnInputArtifact(), n.getAnOutputArtifact(), n.getAKey(), n.getANonce(),
n.getAnAlgorithmOrGenericSource(), n.getKeyOperationSubtype()
1 change: 1 addition & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_plaintext_sources.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
| openssl_basic.c:40:13:40:31 | EncryptOperation | openssl_basic.c:35:54:35:62 | Message | openssl_basic.c:181:49:181:87 | Constant |
6 changes: 6 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/cipher_plaintext_sources.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import cpp
import experimental.quantum.Language

from Crypto::CipherOperationNode n, Crypto::MessageArtifactNode m
where n.getAnInputArtifact() = m
select n, m, m.getSourceNode()
2 changes: 2 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/hash_input_sources.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
| openssl_basic.c:124:13:124:30 | HashOperation | openssl_basic.c:120:37:120:43 | Message | openssl_basic.c:181:49:181:87 | Constant |
| openssl_basic.c:144:13:144:22 | HashOperation | openssl_basic.c:144:24:144:30 | Message | openssl_basic.c:181:49:181:87 | Constant |
6 changes: 6 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/hash_input_sources.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import cpp
import experimental.quantum.Language

from Crypto::HashOperationNode n, Crypto::MessageArtifactNode m
where n.getInputArtifact() = m
select n, m, m.getSourceNode()
2 changes: 2 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/hash_operations.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
| openssl_basic.c:124:13:124:30 | HashOperation | openssl_basic.c:124:13:124:30 | Digest | openssl_basic.c:116:38:116:47 | HashAlgorithm | openssl_basic.c:120:37:120:43 | Message |
| openssl_basic.c:144:13:144:22 | HashOperation | openssl_basic.c:144:13:144:22 | Digest | openssl_basic.c:144:67:144:73 | HashAlgorithm | openssl_basic.c:144:24:144:30 | Message |
5 changes: 5 additions & 0 deletions cpp/ql/test/experimental/library-tests/quantum/openssl/hash_operations.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import cpp
import experimental.quantum.Language

from Crypto::HashOperationNode n
select n, n.getDigest(), n.getAnAlgorithmOrGenericSource(), n.getInputArtifact()
Loading