[go: nahoru, domu]

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: CWE-273 Unsafe certificate trust #3550

Merged
merged 5 commits into from
Jun 29, 2020

Conversation

luchua-bc
Copy link
Contributor

Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.

Unsafe implementation of the interface X509TrustManager and HostnameVerifier ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks. This is a very common issue with web development.

The query checks whether trust manager is set to trust all certificates or the hostname verifier is turned off. I've tested it against some GitHub projects, and a test case has been created.

Please consider to merge the PR. Thanks.

@aibaars
Copy link
Contributor
aibaars commented May 25, 2020

Unsafe SSL configurations have been the cause of quite a few CVEs . I wanted to share a couple I ran into when investigating past CVEs. I don't expect this query to find all the cases listed below, but they might be a good starting point for more queries in this area.

CVE-2020-1929 apache/beam@a7dd23d

CVE-2018-17187 apache/qpid-proton-j@0cb8ca0

CVE-2018-8034 apache/tomcat@2835bb4

CVE-2018-10936 pgjdbc/pgjdbc@cdeeaca

CVE-2018-11087 spring-projects/spring-amqp@444b74e

CVE-2018-11775 apache/activemq@69fad2a

@luchua-bc
Copy link
Contributor Author

Thanks @aibaars for the CVEs. The apache/tomcat one can be detected with the existing query while three others can be detected with some new code. Two of them (apache/qpid-proton-j and apache/activemq) utilize the mechanism setEndpointIdentificationAlgorithm of SSLEngine and the spring-projects/spring-amqp one uses a third-party com.rabbitmq.client.ConnectionFactory library.

For pgjdbc/pgjdbc, the fix to the CVE is not to take an external hostname verifier that could be too lenient. As the code of external hostname verifier is not available in the repository, it cannot be detected with this query. And the first one apache/beam doesn't have a Java database (only JavaScript and Python) so probably there is something wrong with its file structure thus it cannot be analyzed.

In summary, the query is being updated to handle four of these CVEs and I'm in the middle of finalizing code changes and thorough testing. I will commit the code when it's done, probably in the next two days.

@luchua-bc

@aibaars
Copy link
Contributor
aibaars commented May 27, 2020

Really great! I had not expected that this query (with some additions) would be able to find all those CVEs .

I have two CodeQL databases for apache/beam. One before and one after the fix:

You can also create databases with the CodeQL CLI. Building old commits of a project can be tricky, they often require older versions of Java and maven. Also the maven central repository does not allow http downloads anymore, so you might need to search and replace http: -> https: in pom files.

@luchua-bc
Copy link
Contributor Author
luchua-bc commented May 28, 2020

@aibaars Thanks for generating the apache/beam databases. I've committed new code to detect all scenarios discussed above.

  • CVE-2020-1929 apache/beam@a7dd23d - The query can detect its insecure trustAllCerts implementation in both the before and after images. Since the CVE is to fix a configuration error that the implementation was invoked when it should not, the query still helps although it doesn't directly detect the configuration issue with the old version.

apache_beam

  • CVE-2018-17187 apache/qpid-proton-j@0cb8ca0 - The CVE is for possible misconfiguration with an externally created hostname verifier that doesn't validate hostname. The fix is to use the setEndpointIdentificationAlgorithm("HTTPS") method to enforce hostname verification when the application mode is SslDomain.VerifyMode.VERIFY_PEER_NAME. A new MethodAccess SSLEndpointIdentificationNotSet has been added to validate configuration of SSLEngine. Support to SSLSocket is also added since it's commonly used too.

apache_qpid-proton-j_0 29 0

apache_tomcat_9 0 8

  • CVE-2018-10936 pgjdbc/pgjdbc@cdeeaca - The fix is not to take an external hostname verifier that could be too lenient by default. As the code of external hostname verifier is not available in the repository, it cannot be detected with this query.

  • CVE-2018-11087 spring-projects/spring-amqp@444b74e - The fix is to invoke the enableHostnameVerification() method of com.rabbitmq.client.ConnectionFactory to enforce hostname verification. A new MethodAccess has been added to check this misconfiguration.

spring-amqp_v2 0 5

apache_activemq_5 15 5

Regards,
@luchua-bc

@p-
Copy link
Contributor
p- commented May 28, 2020

@luchua-bc Oh, wow, nice work!
I discovered several of those 2018 CVE's 😉
CVE-2020-11050 is one from this year, but I think you've already got it covered. (It was fixed with setEndpointIdentificationAlgorithm("HTTPS"))

@sj
Copy link
Collaborator
sj commented May 28, 2020

Amazing work, @luchua-bc!

@luchua-bc
Copy link
Contributor Author

Thanks for the nice comments. With those CVEs I can train and test my query, now I'm a little more fluent with CodeQL and I love it:-)

Copy link
Contributor
@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

I've added a number of inline comments to clean up the QL code a bit, but otherwise the QL code looks good.

@luchua-bc
Copy link
Contributor Author

Thanks @aschackmull a lot for all your help with this PR.

@luchua-bc

@aschackmull aschackmull merged commit b53b905 into github:master Jun 29, 2020
@luchua-bc luchua-bc deleted the java-unsafe-cert-trust branch June 29, 2020 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants