Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#7252 closed defect (fixed)

Gajim fails to handle invalid certificates

Reported by: Gu1 Owned by: zimio
Priority: normal Milestone: 0.15.3
Component: None Version: 0.15
Severity: major Keywords: ssl, certificate, security
Cc: jefry.reyes@…, mschmidt@… Blocked By:
Blocking: OS: All

Description

Bug description

Gajim does not seem to properly handle invalid/broken/expired certificates. The _ssl_verify_callback function in tls_nb.py is called by OpenSSL for every certificate in the certificate chain (CA first, server certificate last) but always return True whether an error was encountered or not.

This forces OpenSSL to verify each certificate until none is left, at which points it will call _ssl_verify_callback one last time with an error number of 0.

(This behavior is documented here: man 3 SSL_CTX_set_verify
"If verify_callback returns 1, the verification process is continued. If verify_callback always returns 1, the TLS/SSL handshake will not be terminated with respect to verification failures and the connection will be established."
And can be observed in function crypto/x509/x509_vfy.c:internal_verify() in OpenSSL source code.)

_ssh_verify_callback only stores the last error code, which always is 0 unless an error was encountered in the deepest level of the chain (the CA), so gajim will not warn as long as the CA is recognized.

Steps to reproduce

To confirm this behavior, add at the begining of _ssl_verify_callback:

        self._dumpX509(cert)
        print >>self.stderr, "Args:", errnum, depth, ok, '\n-------'

And try to connect to a server with an invalid server certificate (for example, an expired one). You will observe the following:

Digest (SHA-1): {CA cert fingerprint}
(...)
Expired: No
Subject:
X509Name: {The CA Name}
(...)
Args: 0 1 1 
-----------------------------
Digest (SHA-1): {Server cert fingerprint}
(...)
Expired: Yes
Subject:
X509Name: {The Server's CN}
(...)
Args: 10 0 0 
-----------------------------
Digest (SHA-1): {Server cert fingerprint again}
(...)
Expired: Yes
Subject:
X509Name: {The Server's CN again}
(...)
Args: 0 0 1
-----------------------------

You can see that _ssl_verify_callback is called with an errnum of 10 (meaning expired certificate), but since it returns True and OpenSSL has reached the last certificate in the chain, OpenSSL calls it one last time with an error code of 0 and the "ok" argument set to 1.

This problem goes beyond expired certificates. It is also possible to edit any existing and valid server certificate by changing the CN manually. The certificate's signature will be become invalid and OpenSSL will detect it and return errnum 7 ("Certificate signature failure") but gajim will not warn and will proceed with the connection anyway...

Attachments (3)

ssl.patch (2.0 KB) - added by deknos 3 years ago.
patch for the faulty certificate handling
gajim_ssl_error.patch (3.5 KB) - added by zimio 3 years ago.
nbxmpp_error_ssl.patch (604 bytes) - added by zimio 3 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by zimio

  • Cc jefry.reyes@… added

comment:2 Changed 3 years ago by michich

  • Cc mschmidt@… added

comment:3 Changed 3 years ago by zimio

  • Owner set to zimio

I honestly don't know what _ssl_verify_callback does. It doesn't seem to verify for expired certificates. It is a short function so I'm going to paste it here.

    def _ssl_verify_callback(self, sslconn, cert, errnum, depth, ok):
        # Exceptions can't propagate up through this callback, so print them here.
        try:
            self._owner.ssl_fingerprint_sha1 = cert.digest('sha1')
            self._owner.ssl_certificate = cert
            self._owner.ssl_errnum = errnum
            self._owner.ssl_cert_pem = OpenSSL.crypto.dump_certificate(
                    OpenSSL.crypto.FILETYPE_PEM, cert)
            return True
        except:
            log.error("Exception caught in _ssl_info_callback:", exc_info=True)
            # Make sure something is printed, even if log is disabled.
            traceback.print_exc()

From what I see, it saves the fingerprint, the certificate, error number, and a dump of the certificate. Then returns true, if that fails for some reason (the try block catches everything, so I don't know what exception it is expecting), then it returns None.

Now, I agree that it should return false instead of None when a exception is caught. But I don't understand much how that fixes the bug.

Could you send a patch? Or perhaps explain to me what this function is really doing, cause I'm lost.

comment:4 Changed 3 years ago by asterix

It should not return True, else connection is closed. Those saved error number and certificate are used in Connection to know if we should warn user that there is a certificate problem. What we should do here is maybe to save the different errors and certificates in a list, and correctly check that list in Connection object.

Changed 3 years ago by deknos

patch for the faulty certificate handling

comment:5 Changed 3 years ago by deknos

hi, while processing release critical bugs for debian wheezy i encountered this issue and wrote a patch which SHOULD fix this issue described here and on http://www.openwall.com/lists/oss-security/2012/11/11/6

But atm i don't have tested it, i will at the weekend, but you're welcome to test it yourself, whether it has now the correct behaviour.

I'll write later more.

comment:6 Changed 3 years ago by asterix

Hi deknos,

I'm not sure this patch will work. We use non-blocking sockets, so at the place you test if correct_verification = total_verification, handshake is not done yet.

Moreover, you seem to close the connection is certificate chain is not fully correct. I don't think it's a good user experience. What we currently do (for other errors) is that we accept the connection, but before continuing sending data we ask user if he wants to continue. This check is done in connection.py / connection_accepted().

It's why I think we should just get a list of errors in tls_nb.py, and check all errors in connection.py

comment:7 Changed 3 years ago by zimio

I'm having problems testing. I can't find a server with an expired certificate.

Changed 3 years ago by zimio

Changed 3 years ago by zimio

comment:8 Changed 3 years ago by zimio

I attached two patch (one for gajim and the other for nbxmpp). As I said, I'm having problems testing. Maybe asterix can take a look at it.

comment:9 Changed 3 years ago by Yann Leboulanger <asterix@…>

  • Milestone set to 0.16
  • Resolution set to fixed
  • Status changed from new to closed

(In [1965d691ba63]) correctly handle SSL errors. Fixes #7252

comment:10 Changed 3 years ago by Yann Leboulanger <asterix@…>

(In [d343c934b40a]) correctly handle SSL errors. Fixes #7252

comment:11 Changed 3 years ago by Yann Leboulanger <asterix@…>

  • Milestone changed from 0.16 to 0.15.3

(In [1d8caae49a31]) correctly handle SSL errors. Fixes #7252

Note: See TracTickets for help on using tickets.