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

SecureTransport does not close unverified connections correctly #1976

Closed
hodbn opened this issue Sep 21, 2020 · 0 comments · Fixed by #1977
Closed

SecureTransport does not close unverified connections correctly #1976

hodbn opened this issue Sep 21, 2020 · 0 comments · Fixed by #1977
Assignees
Labels

Comments

@hodbn
Copy link
Member

hodbn commented Sep 21, 2020

SecureTransport does not handle SSL custom-verification failure correctly. Instead of sending an alert and terminating the socket, the client socket leaks thus hanging the server socket.

Expected behavior

When custom-verification is enabled, SecureTransport should terminate the connection.

Actual behavior

While SecureTransport fails custom validation, an SSLError is raised but the socket itself is not terminated:

if trust_result.value not in successes:
raise ssl.SSLError(
"certificate verify failed, error code: %d" % trust_result.value
)

this leads to a conn.close():
if not clean_exit:
# We hit some kind of exception, handled or otherwise. We need
# to throw the connection away unless explicitly told not to.
# Close the connection, set the variable to None, and make sure
# we put the None back in the pool to avoid leaking it.
conn = conn and conn.close()
release_this_conn = True

but conn.sock is still None, it will only be assigned the socket after (and only if) the socket is wrapped successfully:
self.sock = ssl_wrap_socket(

Reproduction

This snippet hangs the server eventually instead of failing NUM_CONNECTIONS times cleanly.

import logging
import socket
import ssl
import threading

import trustme
import urllib3
from urllib3.contrib.securetransport import inject_into_urllib3, extract_from_urllib3

CERT_PATH = "/tmp/ca.pem"
SSL_CTX2 = ssl.SSLContext()
NUM_CONNECTIONS = 3

def server(s):
    for i in range(NUM_CONNECTIONS):
        c, _ = s.accept()
        try:
            logging.critical("Server iter")
            c = SSL_CTX2.wrap_socket(c, server_side=True)
            c.recv(1024)
            c.sendall(b"HTTP/1.1 404 Not Found\r\nConnection: close\r\n\r\n")
            c.close()
        except Exception as e:
            logging.critical("Server ex: %r" % (e,))

def connect_and_fail(port):
    with urllib3.HTTPSConnectionPool("localhost", port, cert_reqs="REQUIRED", ca_certs=CERT_PATH) as p:
        for i in range(NUM_CONNECTIONS):
            logging.critical("Client iter")
            try:
                p.request("GET", "/", retries=False)
            except urllib3.exceptions.SSLError as e:
                logging.critical("Client ex: %r" % (e,))

def main():
    logging.basicConfig(level=logging.INFO)
    # client's chain - ca
    ca = trustme.CA()
    ca.cert_pem.write_to_path(CERT_PATH)
    # server's chain - ca2
    ca2 = trustme.CA()
    server2_cert = ca2.issue_cert(u"localhost")
    server2_cert.configure_cert(SSL_CTX2)

    # start an SSL server with ca chain
    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    s.bind(('', 0))
    port = s.getsockname()[1]
    s.listen(10)
    th = threading.Thread(target=server, args=(s, ))
    th.start()
    
    # inject SecureTransport and try to connect with ca2 chain
    inject_into_urllib3()
    connect_and_fail(port)
    extract_from_urllib3()
    th.join()
@hodbn hodbn self-assigned this Sep 21, 2020
@hodbn hodbn changed the title SecureTransport does not close unverified connections currectly SecureTransport does not close unverified connections correctly Sep 21, 2020
@hodbn hodbn added the TLS label Sep 22, 2020
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 a pull request may close this issue.

1 participant