codeql icon indicating copy to clipboard operation
codeql copied to clipboard

CPP: Disabled SSL certificate verification

Open porcupineyhairs opened this issue 1 year ago • 1 comments

Disable SSL certificate verification can expose the communication to MITM attacks.

This PR adds a query to detect the same. This also include the tests and qhelp for the same.

porcupineyhairs avatar Jun 23 '24 09:06 porcupineyhairs

Hello porcupineyhairs 👋 You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

  • the submission models widely-used frameworks/libraries
  • the vulnerability modeled in the submission is impactful
  • the submission finds new true positive vulnerabilities
  • the submission finds very few false positives
  • code in the submission is easy to read and will be easy to maintain
  • documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
  • the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

ghsecuritylab avatar Jun 24 '24 00:06 ghsecuritylab

@geoffw0 I have included the changes from the review. However, for some reason, the tests don't work anymore. I am unable to diagnose what's wrong here. Can you please take a look?

ghost avatar Sep 15 '24 23:09 ghost

@geoffw0 Changes done!

ghost avatar Sep 18 '24 22:09 ghost

QHelp previews:

cpp/ql/src/experimental/Security/CWE/CWE-295/CurlSSL.qhelp

Disabled certifcate verification

Disabling verification of the SSL certificate allows man-in-the-middle attacks. A SSL connection is vulnerable to man-in-the-middle attacks if the certification is not checked properly. If the peer or the host's certificate verification is not verified, the underlying SSL communication is insecure.

Recommendation

It is recommended that all communications be done post verification of the host as well as the peer.

Example

The following snippet disables certification verification by setting the value of CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYHOST to 0:

string host = "codeql.com"
void bad(void) {
	std::unique_ptr<CURL, void(*)(CURL*)> curl =
		std::unique_ptr<CURL, void(*)(CURL*)>(curl_easy_init(), curl_easy_cleanup);
	curl_easy_setopt(curl.get(), CURLOPT_SSL_VERIFYPEER, 0);
	curl_easy_setopt(curl.get(), CURLOPT_SSL_VERIFYHOST, 0); 
  	curl_easy_setopt(curl.get(), CURLOPT_URL, host.c_str());
  	curl_easy_perform(curl.get());
}

This is bad as the certificates are not verified any more. This can be easily fixed by setting the values of the options to 2.

string host = "codeql.com"
void good(void) {
	std::unique_ptr<CURL, void(*)(CURL*)> curl =
		std::unique_ptr<CURL, void(*)(CURL*)>(curl_easy_init(), curl_easy_cleanup);
	curl_easy_setopt(curl.get(), CURLOPT_SSL_VERIFYPEER, 2);
	curl_easy_setopt(curl.get(), CURLOPT_SSL_VERIFYHOST, 2); 
  	curl_easy_setopt(curl.get(), CURLOPT_URL, host.c_str());
  	curl_easy_perform(curl.get());
}

References

github-actions[bot] avatar Sep 19 '24 17:09 github-actions[bot]

Looks like you need to:

  • autoformat CurlSSL.ql
  • fix the qhelp; I think you're just missing <p> / </p> tags around your text blocks.

Let me know if you need any help with these things.

geoffw0 avatar Sep 19 '24 17:09 geoffw0

@geoffw0 Changes done. PTAL.

ghost avatar Sep 19 '24 17:09 ghost