python / cpython

The Python programming language
https://www.python.org
Other
62.59k stars 30.04k forks source link

ssl.SSLSocket shutdown doesn't behave like socket.shutdown #63080

Open 614783e3-0c6b-47f0-af06-b3212152b317 opened 11 years ago

614783e3-0c6b-47f0-af06-b3212152b317 commented 11 years ago
BPO 18880
Nosy @pitrou, @tiran, @alex, @dstufft, @matrixise
Files
  • ssl-shutdown-fail.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['3.7', 'expert-SSL', 'type-bug', 'library'] title = "ssl.SSLSocket shutdown doesn't behave like socket.shutdown" updated_at = user = 'https://bugs.python.org/zielmicha' ``` bugs.python.org fields: ```python activity = actor = 'christian.heimes' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)', 'SSL'] creation = creator = 'zielmicha' dependencies = [] files = ['31514'] hgrepos = [] issue_num = 18880 keywords = ['patch'] message_count = 6.0 messages = ['196494', '272351', '277423', '301389', '301390', '301392'] nosy_count = 7.0 nosy_names = ['janssen', 'pitrou', 'christian.heimes', 'alex', 'dstufft', 'matrixise', 'zielmicha'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue18880' versions = ['Python 2.7', 'Python 3.6', 'Python 3.7'] ```

    Linked PRs

    614783e3-0c6b-47f0-af06-b3212152b317 commented 11 years ago

    SSLSocket documentation mentions shutdown as analogue to socket.shutdown. However, instead of forbidding communication, it removes SSL wrapper from socket. For example, the following script doesn't work and returns garbage:

        import socket
        import ssl
    
        s = socket.socket()
        s.connect(('google.com', 443))
        client = ssl.wrap_socket(s)
        client.sendall(b'GET / HTTP/1.0\nConnection: close\n\n')
        client.shutdown(socket.SHUT_WR)
    
        print(repr(client.recv(40)))

    Attached patch makes shutdown raise exception if how != SHUT_RDWR, as closing one side of socket over SSL doesn't make sense (unless I'm missing something).

    matrixise commented 8 years ago

    Christian,

    What do you think about this issue ?

    1. Fix for 3.5 and 3.6
    2. Maybe for 2.7 ?
    tiran commented 8 years ago

    Sounds fine, but it's not a security issue. I'm re-targeting the bug for 3.7.

    tiran commented 7 years ago

    Sounds like a good idea.

    pitrou commented 7 years ago

    This will needlessly break code which until now accepts both kinds of sockets.

    By the way, socket.shutdown() doesn't specify that *only one direction is shut down when using SHUT_RD or SHUT_WR; what is guaranteed is that *at least the given direction will shut down. But there may be socket types where unidirectional shutdown is not supported and both directions will be shut down. This is (approximately) what SSLSocket does -- though the SSL unwrapping part is a bit unintuitive as well.

    tiran commented 7 years ago

    I agree with Antoine. I tried to test your patch and found out that is not compatible with socketserver. The socketserver module shuts down the connection with SHUT_WR.

    We could either ignore the problem or ignore the how and use SHUT_RDWR in all cases.