CPP: Add query for CWE-297: Improper Validation of Certificate with Host Mismatch

[ihsinme]

trafficstars

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.

[MathiasVP]

Hi @ihsinme,

Thanks for another contribution. We will review get to review it soon!

[MathiasVP]

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?

[MathiasVP]

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.

I'm not sure what you mean. Do you want to add the same file to several folders?

[ihsinme]

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?

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. if the program checks the validity of the certificate but does not check the name, it is possible to MITM by sending any valid certificate to the program.

[ihsinme]

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.

I'm not sure what you mean. Do you want to add the same file to several folders?

the problem is that i have to store the good and bad example in two different files. Perhaps you have rules for this case.

[MathiasVP]

the problem is that i have to store the good and bad example in two different files. Perhaps you have rules for this case.

You can have as many .c and .cpp test files as you want in the test directory as you want. codeql test run should pick them all up.

Does that solve your issue?

[ihsinme]

I have added another test file, could you please run the checks

[MathiasVP]

CI failures:

Executing tests in /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-297/semmle/tests.
2022-06-20T13:03:53.7182630Z --- expected
2022-06-20T13:03:53.7183311Z +++ actual
2022-06-20T13:03:53.7183720Z @@ -1,1 +1,1 @@
2022-06-20T13:03:53.7184595Z -| test1.cpp:11:6:11:13 | badTest1 | You may have missed checking the name of the certificate. |
2022-06-20T13:03:53.7184980Z +
2022-06-20T13:03:53.7190687Z ##[error][5386/5505 comp 6.1s eval 599ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-297/semmle/tests/ValidationCertificateHostMismatch.qlref

It looks like the testcase in test1.cpp isn't being flagged.

[MathiasVP]

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 Asio library. It would make sense for this query to also cover this API. If you want to improve your final score on the bug bounty program, I suggest the following extensions to your query:

• Cover the verify callback from Boost's Asio library (like in https://github.com/github/securitylab/tree/main/CodeQL_Queries/cpp/OpenSSL-hostname-validation).
• Cover incorrect certificate verification in GnuTLS.

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.

[ihsinme]

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. I need some time for this.

Thank you.

fixing the work with the test file, I'll deal with it after the request has been improved.

[ihsinme]

@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. if the query logic is looking for something that is missing somewhere in the code, then building the test files is very difficult. the presence of one correct example excludes detection ( Is there any trick for this?

[MathiasVP]

@MathiasVP, i made additions.

Fantastic!

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. if the query logic is looking for something that is missing somewhere in the code, then building the test files is very difficult. the presence of one correct example excludes detection ( Is there any trick for this?

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:

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.

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).

[ihsinme]

I have established a call chain connection, please have a look.

I would like to make a few points:

1. in the boost, I still couldn't make the correct calls, because I don't know how to define a callback call in the test file. (if you have any thoughts please push me)
2. in file test.cpp in function goodTest3 on line 113 if you change the call to X509_NAME_get_index_by_NID (X509_get_subject_name (cert), nid, -1);. then there will be no error, but this call will not be considered X509_NAME_get_index_by_NID either. if you could tell me why this is happening, that would be great.
3. when I wrote about the complexity of working with several test files, I thought that there is a mechanism that allows each test file to be considered as a separate project.

[MathiasVP]

I have established a call chain connection, please have a look.

Great! Thanks for fixing this :)

in the boost, I still couldn't make the correct calls, because I don't know how to define a callback call in the test file. (if you have any thoughts please push me)

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?

in file test.cpp in function goodTest3 on line 113 if you change the call to X509_NAME_get_index_by_NID (X509_get_subject_name (cert), nid, -1);. then there will be no error, but this call will not be considered X509_NAME_get_index_by_NID either. if you could tell me why this is happening, that would be great.

I don't completely follow what you're saying here. In particular:

• What do you mean by "this call will not be considered X509_NAME_get_index_by_NID either"?
• You're saying that if you change the line:

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).

when I wrote about the complexity of working with several test files, I thought that there is a mechanism that allows each test file to be considered as a separate project.

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.

[ihsinme]

You should be able to do something similar to what they do in this example:

thanks, added two goodTest functions.

then there will not be an alert? There shouldn't be an alert in this function anyway, right? (since it's a good test).

I was surprised that I didn't declare a function with three parameters in this test. and expected an error with such a replacement.

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.

P.S. could you show me the result of working on three file tests?

[ihsinme]

good afternoon @MathiasVP. any news on this PR?

[MathiasVP]

good afternoon @MathiasVP. any news on this PR?

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.

[MathiasVP]

CI failure:

--- expected
2022-08-03T09:16:17.3406473Z +++ actual
2022-08-03T09:16:17.3407144Z @@ -1,1 +1,5 @@
2022-08-03T09:16:17.3408765Z -| test1.cpp:11:6:11:13 | badTest1 | You may have missed checking the name of the certificate. |
2022-08-03T09:16:17.3410313Z +| test1.cpp:47:5:47:12 | badTest1 | You may have missed checking the name of the certificate. |
2022-08-03T09:16:17.3411826Z +| test1.cpp:53:5:53:12 | badTest2 | You may have missed checking the name of the certificate. |
2022-08-03T09:16:17.3413306Z +| test1.cpp:59:5:59:12 | badTest3 | You may have missed checking the name of the certificate. |
2022-08-03T09:16:17.3414764Z +| test2.cpp:41:6:41:13 | badTest1 | You may have missed checking the name of the certificate. |
2022-08-03T09:16:17.3416365Z +| test.cpp:138:6:138:13 | badTest1 | You may have missed checking the name of the certificate. |
2022-08-03T09:16:17.3425768Z ##[error][6893/7565 comp 7.5s eval 925ms] FAILED(RESULT) ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-297/semmle/tests/ValidationCertificateHostMismatch.qlref

Looks like the .expected files need to be updated.

[ihsinme]

@MathiasVP please show me what is the error.

[ihsinme]

good afternoon @MathiasVP. maybe I didn’t do something for this PR?

[MathiasVP]

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 main.

[ihsinme]

Hi @MathiasVP. I have to do something for this PR?

[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 main will resolve it 🤞.

[github-actions[bot]]

QHelp previews:

cpp/ql/src/experimental/Security/CWE/CWE-297/ValidationCertificateHostMismatch.qhelp

Validation certificate host mismatch.

Lack of certificate name validation allows any valid certificate to be used. And that can lead to disclosure.

Example

The 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

• CERT Coding Standard: DRD19. Properly verify server certificate on SSL/TLS - Android - Confluence.
• Common Weakness Enumeration: CWE-297.

[ihsinme]

Hi @MathiasVP The check didn't start.