Potential Cryptography Issues in file libcloud/compute/drivers/vsphere.py

[fazledyn-or]

Summary

This bug report is created by manually analyzing the source codes based on two fixes generated by Intelligent Code Repair tool (iCR).

Detailed Information

• Python: 3.8.10
• OS: Ubuntu 20.04

Suggested Fix 1

In your project file libcloud/compute/drivers/vsphere.py on Line 111, there’s a code segment that goes-

context = ssl.create_default_context(cafile=ca_cert)
self.connection = connect.SmartConnect(
    host=host,
    port=port,
    user=username,
    pwd=password,
    sslContext=context,
)

While triaging your repository, we noticed that the connect.SmartConnect method from pyVim library uses a method called Connect that calls a method called __Login which creates a SoapStubAdapter class object. A comment on that class on Line 1380 - 1384 goes-

# @param sslContext SSL Context describing the various SSL options. It is
#                   only supported in Python 2.7.9 or higher.
#            if sslContext is used, load cert & key to the context with API
#            sslContext = ssl.create_default_context(cafile=ca_cert_file)
#            sslContext.load_cert_chain(key_file, cert_file)

However, in your source file the Certificate Chain isn’t loaded into sslContext object. We suggest that you load the certificate chain into the sslContext object as mentioned in the comments.

Suggested Fix 2

In the same file on Line 131 - 135, it goes-

if "certificate verify failed" in error_message:
                # bypass self signed certificates
                try:
                    context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
                    context.verify_mode = ssl.CERT_NONE

Now, it says here that the following code is to bypass the self-signed certificates. In this case, the official documentation for ssl says-

ssl.PROTOCOL_SSLv23
Alias for PROTOCOL_TLS.
Deprecated since version 3.6: Use PROTOCOL_TLS instead.

To clear the confusion, it’s suggested that you use PROTOCOL_TLS while instantiating the context object. However, if the code is used for some other reason that bypassing self-signed certificates, please let us have a discussion.

CLA Requirements:

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

All contributed commits are already automatically signed off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).

• Git Commit Sign Off documentation

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed - to improve global software supply chain security.

The bug is found by running the iCR tool by OpenRefactory, Inc. and then manually triaging the results.

[Kami]

Thanks for reporting this.

That code is pretty old and I'm wondering how much of it still relevant. It appears a lot of it was needed in the past due to compatibility with old Python version.

I would personally feel much more comfortable if that code could be simplified to reduce all the possible permutations and edge cases. This should reduce the surface area and make security and other issues less likely.

And in general I think that code is not following best practices. Doing something like this here (

libcloud/libcloud/container/types.py

Line 19 in abba8c1

class Type:

) and simply accepting any server certificate when that error is received seems like a bad and insecure practice to me. So if anything, that could should be removed.

If we want to leave such option in the code, it should be an explicit opt-in by the end user - they need to understand and accept the consequences if they chose to skip ca cert validation.

[fazledyn-or]

Hi @Kami, thanks for replying. I think we can apply the following fixes-

Case 1

We can rewrite the vsphere.py file into something like this- we put option for keyfile and certfile. Then it's completely depends on the user. They may not use the certificate chain but the support is there in the library.

class VSphereNodeDriver

...
context = ssl.create_default_context(cafile=ca_cert)
if certfile and keyfile:
    context.load_cert_chain(certfile=certfile, keyfile=keyfile)
self.connection = connect.SmartConnect(
    host=host,
    port=port,
    user=username,
    pwd=password,
    sslContext=context,
)

class VSphere_REST_NodeDriver

Since VSphereNodeDriver is used by this class too, we add support for certificate chain here too. The updated code would look something like this-

def __init__(self, key, secret=None, secure=True, host=None, port=443, ca_cert=None, certfile=None, keyfile=None):
    ...
    if ca_cert:
        self.connection.connection.ca_cert = ca_cert
    else:
        self.connection.connection.ca_cert = False
    if certfile and keyfile:
        self.connection.connection.certfile = certfile
        self.connection.connection.keyfile = keyfile
    else:
        self.connection.connection.certfile = False
        self.connection.connection.keyfile = False

And then use it as below-

self.driver_soap = VSphereNodeDriver(
    ...
    ca_cert=self.connection.connection.ca_cert,
    certfile=self.connection.connection.certfile,
    keyfile=self.connection.connection.keyfile,
)

Case 2

Since I don't have the entire context about the whole project, I think simply replacing PROTOCOL_SSLv23 with PROTOCOL_TLS will suffice. The fixed code would look something like this-

if "certificate verify failed" in error_message:
                # bypass self signed certificates
                try:
                    context = ssl.SSLContext(ssl.PROTOCOL_TLS)
                    context.verify_mode = ssl.CERT_NONE

Please let me know what you think.