Quantum: Add initial qltests for OpenSSL modeling #19564
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| class KnownOpenSSLSignatureAlgorithmConstant extends KnownOpenSSLAlgorithmConstant { |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase. Warning
| //Heuristics for distinguishing int literals from other literals | ||
| exists(this.getValue().toInt()) and | ||
| not this instanceof CharLiteral and | ||
| not this instanceof StringLiteral |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase. Warning
| * Note: this predicate should only consider restrictions with respect to integers only. | ||
| * General restrictions are in the OpenSSLGenericSourceCandidateLiteral class. | ||
| */ | ||
| private predicate isOpenSSLIntLiteralGenericSourceCandidate(IntLiteral l) { |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase. Warning
| * Literals are filtered, for example, if they are used in a way no indicative of an algorithm use | ||
| * such as in an array index, bitwise operation, or logical operation. | ||
| * Note a case like this: | ||
| * if(algVal == "AES") |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase. Warning
|
Just a quick drive-by comment: the tests should preferably go into the |
@jketema Would |
nicolaswill
changed the title
|
@nicolaswill How about changing the pattern in the code owners file to |
bdrodes
force-pushed
the
initial_openssl_tests
branch
2 times, most recently
from
3fc48eb to
5ab05cd
Compare
…ound through tests, and updating CODEOWNERS for quantum tests
bdrodes
force-pushed
the
initial_openssl_tests
branch
from
5ab05cd to
41f008d
Compare
|
@jketema Ready for your review. |
| 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") |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If this is in fact only needed for your test to work, then I would strongly suggest to rewrite the test.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@jketema We stub out other libraries with the Apache license. An example is Spring. |
|
@nicolaswill Thanks for pointing our the prior art in the Java QL tests. I think we want a similar kind of set up where the stubs are isolated in their own directory, we have a top-level readme like https://github.com/github/codeql/blob/main/java/ql/test/stubs/README.md, and have a license file in the directory with the OpenSSL stubs. |
…l apache license and a readme for future stub creation. Modify existing test case to reference stubs location.
|
I added a stubs location and moved the openssl stubs accordingly, and added the openssl apache license. |
|
Do we need to update codeowners for the stubs? |
No description provided.