CPP: Add query for CWE-297: Improper Validation of Certificate with Host Mismatch #9086
Conversation
|
Hi @ihsinme, Thanks for another contribution. We will review get to review it soon! |
|
Hi @ihsinme, Can you add a comment to this query explaining how it's different from https://github.com/github/codeql/blob/main/cpp/ql/src/Security/CWE/CWE-295/SSLResultNotChecked.ql? |
I'm not sure what you mean. Do you want to add the same file to several folders? |
As I understand it, this request is looking for a situation where the certificate was not checked at all, for correctness. my query is looking for a situation where the domain name of the certificate is not verified. |
the problem is that i have to store the good and bad example in two different files. |
You can have as many Does that solve your issue? |
| import cpp | ||
| import semmle.code.cpp.valuenumbering.GlobalValueNumbering | ||
|
|
||
| predicate checkHostnameOlder1() { |
There was a problem hiding this comment.
Reading this predicate out loud, this is what I'm reading:
This predicate holds if there exists a function call anywhere in the program that calls X509_get_ext_d2i, and either:
- There exists a use of the macro
sk_GENERAL_NAME_numor a use of the macrosk_GENERAL_NAME_value, or - There exists a call to the function
sk_GENERAL_NAME_valueor a call to the functionsk_GENERAL_NAME_num.
That doesn't make a lot of sense to me. Can sk_GENERAL_NAME_num and sk_GENERAL_NAME_value really both be macros and function calls (maybe depending on versions?). Also: Shouldn't this predicate have any parameters? Right now, this is just a property of the entire codebase that either holds of doesn't hold (and I can make it hold by adding a random call to X509_get_ext_d2i, sk_GENERAL_NAME_num and sk_GENERAL_NAME_value anywhere in the program.
What are you really trying to model with this predicate?
There was a problem hiding this comment.
sorry for the delay in replies.
in this predicate I have omitted call connectivity control due to the presence of macros, in the rest of the predicates I use GVN for call connectivity control.
| ) | ||
| } | ||
|
|
||
| predicate checkHostnameOlder2() { |
There was a problem hiding this comment.
Same question here: This predicate is a property of the entire codebase that either holds or doesn't hold. And I can make it hold by adding a couple of calls to some functions anywhere in the codebase. Please describe what are you trying to do here, and I'm sure we can fix it up.
There was a problem hiding this comment.
I have implemented a search for all possible ways to validate the name of the certificate specified in https://wiki.openssl.org/index.php/Hostname_validation.
I'm looking for code where the developer doesn't use any of these name validation methods at all, but uses certificate validation. In this case, it's definitely a threat.
I skip when there is an error in the implementation of one of the methods, or the method is not implemented for all connections.
| not checkHostnameOlder2() and | ||
| not checkHostnameUp110() and | ||
| not checkHostnameUp102() and | ||
| fc.getTarget().hasName(["SSL_set_verify", "SSL_get_peer_certificate", "SSL_get_verify_result"]) |
There was a problem hiding this comment.
Right now you're raising an alert on all uses of the functions whose name is SSL_set_verify, SSL_get_peer_certificate or SSL_get_verify_result. Is that really what you intend?
There was a problem hiding this comment.
at the moment I want to find a place to work with connection verification, when in the code I did not find a way to check the name of the certificate. every place is a mistake.
|
I have added another test file, could you please run the checks |
|
CI failures: It looks like the testcase in |
|
Hi @ihsinme, I've discussed the status of this query with the GitHub Security Lab team, and they've pointed me to this query in their repo: https://github.com/github/securitylab/tree/main/CodeQL_Queries/cpp/OpenSSL-hostname-validation This query also covers a verify callback from Boost's
To be clear: You don't have to do this, but adding the above extensions will likely improve the final score on your bug bounty submission. |
Hi @MathiasVP. of course I will try to improve this query. Thank you. fixing the work with the test file, I'll deal with it after the request has been improved. |
|
@MathiasVP, i made additions. but my local environment is not able to work with multiple test files, can you show the results of working with test files? Pay attention to the test files, I talked about this at the beginning of the request. |
Fantastic!
There really isn't any tricks to this. The tests highlights the behavior of the query as you wrote it. I described the problem here:
I think the only way to fix the tests is to change the query logic so that the above isn't the case anymore (i.e., so that a single call in some function anywhere in the code doesn't fix all the alerts in the codebase). |
|
I have established a call chain connection, please have a look. I would like to make a few points:
|
Great! Thanks for fixing this :)
You should be able to do something similar to what they do in this example: class verify_context { /* ... */ };
bool my_verify_callback(
bool preverified,
verify_context& ctx
) {
/* ... */
}
void test(boost::asio::ssl::stream sock){
// ...
sock.set_verify_callback(my_verify_callback);
// ...
}or using a lambda: class verify_context { /* ... */ };
void test(boost::asio::ssl::stream sock){
// ...
sock.set_verify_callback([&](bool preverified, verify_context& ctx) {
// ...
});
// ...
}Does that make sense?
I don't completely follow what you're saying here. In particular:
len = X509_NAME_get_text_by_NID (X509_get_subject_name (cert), nid, NULL, 0);to len = X509_NAME_get_index_by_NID (X509_get_subject_name (cert), nid, -1);then there will not be an alert? There shouldn't be an alert in this function anyway, right? (since it's a good test).
Different .cpp files for a qltest are compiled as different compilation units (like you'd normally do for a C/C++ project), but they're all linked together before running the test. |
thanks, added two goodTest functions.
I was surprised that I didn't declare a function with three parameters in this test. and expected an error with such a replacement.
thanks. P.S. could you show me the result of working on three file tests? |
|
good afternoon @MathiasVP. |
Hi! I've just come back from vacation now and I'm catching up with all of the open PRs of yours that I'm assigned to. I promise to have an update on all of those today. |
|
CI failure: Looks like the .expected files need to be updated. |
|
@MathiasVP please show me what is the error. |
|
good afternoon @MathiasVP. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
| } | ||
|
|
||
| predicate checkHostnameGnuTLS(FunctionCall fc) { |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase.
There was a problem hiding this comment.
Do I understand correctly that I need to correct the name to CheckHostnameGnuTLS ?
There was a problem hiding this comment.
No. The Code Scanning alert is saying that QL names must preferably be in PascalCase (for classes and modules) or camelCase (for predicates and variables). So checkHostnameGnuTLS should ideally be checkHostnameGnuTls to match our style guide. However, one could argue that, since TLS is a common way to write Transport Layer Security, this name is actually fine.
So I don't think you need to change your name to make the Code Scanning alert disappear.
| ) | ||
| } | ||
|
|
||
| predicate checkHostnameBOOST1(FunctionCall fc) { |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase.
| ) | ||
| } | ||
|
|
||
| predicate checkHostnameBOOST2(FunctionCall fc) { |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase.
|
Hi @ihsinme, Apologies for not getting back to this one. There was a technical issue that caused CI to complain. I hope we've fixed it by merging in the latest |
|
Hi @MathiasVP. |
|
Hi @ihsinme, I'm trying to resolve the CI failure. Not sure why you keep being a victim of this particular issue. Hopefully merging in |
|
QHelp previews: cpp/ql/src/experimental/Security/CWE/CWE-297/ValidationCertificateHostMismatch.qhelpValidation certificate host mismatch.Lack of certificate name validation allows any valid certificate to be used. And that can lead to disclosure. ExampleThe following example shows working with and without certificate name verification. ...
SSL_set_hostflags(ssl, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
if (!SSL_set1_host(ssl, host)) return false;
SSL_set_verify(ssl, SSL_VERIFY_PEER, NULL); // GOOD
...
result = SSL_get_verify_result(ssl);
if (result == X509_V_OK)
{
cert = SSL_get_peer_certificate(ssl);
if(cert) return true; // BAD
}
...
References
|
|
Hi @MathiasVP |
this query finds certificate situations without name validation.
I tried to take into account all cases of name processing, so there are old methods.
CVE-2010-1155
CVE-2013-7449
CVE-2016-10937
I had some confusion with the test file, the current one contains only good situations. I don't know how I can proceed in this case, I do a separate simple test without validation, but I'm not sure if it's common practice, two file tests in different folders. tell me how to be.