Skip to content

Conversation

atorralba
Copy link
Contributor

@atorralba atorralba commented Jun 28, 2021

PR to promote the Unsafe certificate trust query created in #3550

Changes

  • Existing files were moved out of experimental
  • The UnsafeCertTrust.qll file was created to contain most of the classes and predicates used by the query.
  • Some classes that will probably be reusable by other queries were added to the Networking.qll and Encryption.qll libraries.
  • The piece of logic that was handling flawed hostname verifications because of a missing (or wrong) SSL endpoint identification algorithm was refactored - now, a taint tracking configuration is used to properly detect established connections without a safe SSL configuration (see SslEndpointIdentificationFlowConfig).
  • Changed tests to use InlineExpectationsTest and added some RabbitMQ stubs.

Evaluation

➕ The following CVEs are now detected by the query:

✅ The following CVEs are still detected by the query:

➡️ The following CVEs are now detected by the Insecure TrustManager query (#4879):

⚠️ The following CVEs are still not detected by the query:

❌ The following CVEs are no longer detected by the query:

  • CVE-2018-8034 apache/tomcat@2835bb4. This is due to taint tracking now being used, and the flow being lost because of it crossing through a class field used in a nested class.

To Consider

  • The sources of SafeSslParametersFlowConfig are PostUpdateNodes because what characterizes them is the update done by setEndpointIdentificationAlgorithm.
  • The predicate isSslSocket is an heuristic and could (probably) be improved.
  • SslConnectionWithSafeSslParameters uses localFlow to add subsequent uses of the sanitizer. This is to correctly identify sanitizers that are added with the following pattern:
SSLSocket socket = (SSLSocket) socketFactory.createSocket();
if (safe) {
    SSLParameters sslParameters = socket.getSSLParameters();
    sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
    socket.setSSLParameters(sslParameters);
}
socket.getOutputStream();

where safe is a boolean that is (almost) always true (e.g. a configuration value that defaults to true, or something like socket instanceof SSLSocket). This is needed because the fix applied to several CVEs follows this pattern, e.g. a configuration value is added in order to give the user the option to revert to the old behavior (no forced hostname verification).

@atorralba atorralba requested a review from a team as a code owner June 28, 2021 13:10
@intrigus-lgtm
Copy link
Contributor

This kind-of collides with #4879, which removes everything related to TrustManagers from the UnsafeCertTrust query:
https://github.com/github/codeql/pull/6171/files#diff-d5c7a86618d97f18cf413d2e72f87196fbba850816852aa6d58ba7896d721c3bR9-R57

@atorralba atorralba force-pushed the atorralba/promote-unsafe-certificate-trust branch from 9974117 to 3f94ab9 Compare June 28, 2021 13:29
@atorralba
Copy link
Contributor Author

This kind-of collides with #4879, which removes everything related to TrustManagers from the UnsafeCertTrust query:
https://github.com/github/codeql/pull/6171/files#diff-d5c7a86618d97f18cf413d2e72f87196fbba850816852aa6d58ba7896d721c3bR9-R57

Yes, just noticed a bunch of conflicts :-P

Rebased to resolve them - let me know if you spot any lingering code that shouldn't be in this query.

Thanks for pointing this out @intrigus-lgtm

@atorralba atorralba dismissed a stale review via ba42105 July 21, 2021 09:30
@atorralba atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jul 21, 2021
@atorralba
Copy link
Contributor Author

@github/docs-content-codeql please review the qhelp file. Even though changes aren't introduced in this PR, it wasn't reviewed when this query was merged to experimental.

@docs-bot
Copy link
Contributor

:octocat:📚 Thanks for the docs ping! 🛎️ This was added to our docs first-responder project board. A team member will be along shortly to respond. To request changes to the docs you can also open a CodeQL docs issue.

Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

@atorralba - this LGTM ✨
A few minor nits (see inline comments for more info)

@atorralba
Copy link
Contributor Author

Thanks @mchammer01, your reviews are really helpful! All comments applied.

@atorralba atorralba force-pushed the atorralba/promote-unsafe-certificate-trust branch 3 times, most recently from a2c3a59 to 2312fe5 Compare November 12, 2021 10:12
@atorralba atorralba force-pushed the atorralba/promote-unsafe-certificate-trust branch from f72e7e0 to 695e77a Compare January 19, 2022 16:01
@atorralba atorralba merged commit c09b669 into github:main Jan 20, 2022
@atorralba atorralba deleted the atorralba/promote-unsafe-certificate-trust branch January 20, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants