Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: Promote Unsafe certificate trust query from experimental #6171

Merged

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)

java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp Outdated Show resolved Hide resolved
@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