classification
Title: asyncore's accept() is broken
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: Arfrever, BreamoreBoy, barry, dmalcolm, giampaolo.rodola, josiahcarlson, matejcik, pitrou, santa4nt
Priority: normal Keywords: patch

Created on 2009-08-14 23:03 by giampaolo.rodola, last changed 2010-12-07 15:22 by barry. This issue is now closed.

Files
File name Uploaded Description Edit
accept.patch giampaolo.rodola, 2010-09-24 20:15 review
accept.patch giampaolo.rodola, 2010-09-25 10:37 review
accept.patch giampaolo.rodola, 2010-10-02 13:18 adds handled_accepted() method review
Messages (16)
msg91579 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-08-14 23:03
An old bad design choice in asyncore is how it forces the user to 
override handle_accept() and then call self.accept() to obtain a socket 
pair.

    def handle_accept(self):
        conn, addr = self.accept()

The documentation itself shows the code above as an example of how an 
asyncore-based server should handle an incoming connection.
What the doc doesn't say is that the user calling self.accept() is 
exposed to different risks:

- self.accept() can return None instead of a socket pair in which case 
TypeError is raised (see pyftpdlib bug: 
http://code.google.com/p/pyftpdlib/issues/detail?id=91)

- ECONNABORTED can be raised. This is reproducible on Linux by hammering 
the server with nmap (see pyftpdlib bug: 
http://code.google.com/p/pyftpdlib/issues/detail?id=105)

- EAGAIN can be raised too. I never experienced this error condition 
myself in pyftpdlib but by looking at twisted/internet/tcp.py it seems 
reasonable to catch EAGAIN too.

- The resulting address can be None, which means that the connection 
didn't take place.
This is reproducible by hammering the server with nmap (see pyftpdlib 
bug http://code.google.com/p/pyftpdlib/issues/detail?id=104).


The right thing to do when one of such events occur is to "return" as no 
connection has really been established between client and server.

Now, altough handling these kind of conditions is quite easy, the API 
design remains fundamentally wrong, as it forces the user to call 
accept().
As asyncore.py is structured right now the only chance the user has to 
properly accepting a connection is manually handling all these cases in 
his subclass.


My patch in attachment tries to solve this problem by defining a new 
"handle_accept_event()" method which takes care of all the error 
conditions described above resulting in handle_accept() be called *only 
when a connection really took place*.
When the connection is established handle_accept_event() stores the 
socket pair as an attribute and the next call to accept() returns that 
pair.
This preserves the current implementation without breaking any code as 
the user will have to override handle_accept() and then call accept() in 
the same manner [1], with the difference that self.accept() will always 
return a valid socket pair.

[1] 
    def handle_accept(self):
        conn, addr = self.accept()
msg112315 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-08-01 11:47
@Giampaolo it looks as if you meant to attach a patch but didn't! :)  If you do attach one could you also supply a modified unit test file with it, thanks.
msg112699 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-08-03 21:51
Shame on me, it seems I totally forgot to attach the patch.
Unfortunately the patch went lost but I'm going to try to rewrite it.
As for tests, ECONN and EAGAIN error conditions are hardly reproducible unless you're using nmap.
I'm assigning this to me for now as a reminder and update the issue as long as I have a working patch.
msg117240 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2010-09-23 22:11
giampaolo: did you ever rewrite the patch?

For reference to other users:
http://code.google.com/p/pyftpdlib/source/browse/trunk/pyftpdlib/ftpserver.py

Note the complexity of the two handle_accept implementations in that file; both of them begin:
        try:
            sock, addr = self.accept()
        except TypeError:
            # sometimes accept() might return None (see issue 91)
            return
        except socket.error, err:
            # ECONNABORTED might be thrown on *BSD (see issue 105)
            if err[0] != errno.ECONNABORTED:
                logerror(traceback.format_exc())
            return
        else:
            # sometimes addr == None instead of (ip, port) (see issue 104)
            if addr == None:
                return
msg117329 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-09-24 20:15
Here's a rewriting attempt (not tested).
Now that I look at it I must say it's quite ugly, so I don't think we should follow this road.
An alternative I see is to return None in case of errors occurring on accept() and make this very clear in doc. 
Other than accept(), doc should explicitly show how to use handle_accept() in general, which would end up looking like this:

class SomeServer(asyncore.dispatcher):
    
    ...

    def handle_accept():
       conn = self.accept()
       if conn is None:
           return
       else:
           sock, addr = conn
       ...

    ...


A completely different approach could be to provide a new "TCPServer" class which deprecates direct accept() method usage. Something like this:


class TCPServer(asyncore.dispatcher):

    def __init__(self, ip, port, handler, interface='', map=None):
        asyncore.dispatcher.__init__(self, map=map)
        self.create_socket(family=socket.AF_INET, type=socket.SOCK_STREAM)
        self.bind((ip, port))
        self.listen(5)
        self.handler = handler

    def handle_accept(self):
        try:
            sock, addr = self.accept()
        except TypeError:
            return
        except socket.error, err:
            if err[0] != errno.ECONNABORTED:
                raise
            return
        else:
            if addr == None:
                return
        handler = self.handler(conn, self._map)

    def writable(self):
        return 0



...but for some reason I don't like it either. Point is asyncore API design is fundamentally wrong and there's nothing we can do about it unless we break backward compatibility or a brand new "asyncore2" module is written.
msg117356 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-09-25 10:37
Patch in attachment makes accept() return None in case no connection takes place and modifies the doc to make this very clear, also providing an example on how an asyncore-based server should be properly set-up .
msg117357 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-25 10:54
> EAGAIN can be raised too. I never experienced this error condition 
> myself in pyftpdlib

From the accept() man page:

EAGAIN or EWOULDBLOCK
              The  socket  is  marked  nonblocking and no connections are present to be accepted.
              POSIX.1-2001 allows either error to be returned for this case, and does not require
              these  constants to have the same value, so a portable application should check for
              both possibilities.

> The resulting address can be None, which means that the connection 
> didn't take place.

The only way this can happen is if the accept() system call returned 0 in addrlen, which sounds rather strange. I'm not convinced hiding operating system bugs is a good idea.
msg117358 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-09-25 11:43
> I'm not convinced hiding operating system bugs is a good idea.

Do you propose to let the error raise then?
The point of frameworks such as asyncore and twisted is to hide all system-specific errors as much as possible and provide a portable interface across all platforms.
AFAICT, the whole point of this issue is that there should be only one way for an asyncore-based server to accept an incoming connection, possibly avoiding the user to deal with low-level details such as catching EWOULDBLOCK/ECONNABORTED/... in his application, and looking for accept() returning None is one possibility.

As I said, in a better designed framework the user shouldn't be supposed to call accept() at all, but that's how asyncore is designed.
msg117359 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-25 11:56
> The point of frameworks such as asyncore and twisted is to hide all
> system-specific errors as much as possible and provide a portable
> interface across all platforms.

As long as these errors are reasonably explainable.
If a strange error only occurs when running nmap on an OS X server, then
it might be useful for the user to know that the OS X server isn't able
to service all connections properly due to a bug in the operating
system.

> AFAICT, the whole point of this issue is that there should be only one
> way for an asyncore-based server to accept an incoming connection,
> possibly avoiding the user to deal with low-level details such as
> catching EWOULDBLOCK/ECONNABORTED/...

I am talking specifically about the address being None (i.e., a (sock,
None) tuple is successfully returned).
EWOULDBLOCK and ECONNABORTED are documented error conditions for
accept(), but returning 0 in addrlen is not.

> As I said, in a better designed framework the user shouldn't be
> supposed to call accept() at all, but that's how asyncore is designed.

Perhaps it's time to add an alternative handle_accepted(sock, addr)
which relieves the user from calling accept() :))
Then, perhaps self.accept() shouldn't filter any errors at all, but the
default handle_accept() would silence them before calling
handle_accepted().
msg117360 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-09-25 12:34
> As long as these errors are reasonably explainable.
> If a strange error only occurs when running nmap on an OS X server, 
> then it might be useful for the user to know that the OS X server 
> isn't able to service all connections properly due to a bug in the 
> operating system.

It might be useful to find whether this is tracked somewhere.
For the record, here (comment #10) is a code which should reproduce this problem:
https://bugs.launchpad.net/zodb/+bug/135108
As for what asyncore should in this case I think it should theoretically be safe for accept() to return (sock, None). What usually happen after that is that the socket object is passed to another dispatcher subclass (the handler) and the connection gets automatically closed once recv() or send() get called on the next async-loop.

> Perhaps it's time to add an alternative handle_accepted(sock, addr)
> which relieves the user from calling accept() :))

This sounds like a very good idea for 3.2.
As for 3.1 and 2.7 (2.6?) I think it can be useful to suppress EWOULDBLOCK/ECONNABORTED and return None instead.
Considering accept() can *already* return None it wouldn't break backward compatibility.
msg117873 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-10-02 13:18
Patch in attachment adds a handled_accepted() method to dispatcher class as recommended by Antoine.
msg117925 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-03 19:28
I'm not an asyncore expert, but I can't see anything wrong with the patch.
msg117981 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-10-04 21:09
Python 3.2 changes committed in r85220.
Still have to commit EWOULDBLOCK/ECONNABORTED changes for 3.1 and 2.7.
msg120003 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2010-10-30 16:44
CVE-2010-3492 references this issue.
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2010-3492
msg120132 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-11-01 15:19
Fixed in r86084 (2.7) and r86085 (3.1).
msg123556 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2010-12-07 15:22
I do not see this as a security bug so no patch for 2.6 please.  (Comment requested from IRC).
History
Date User Action Args
2010-12-07 15:22:09barrysetnosy: + barry
messages: + msg123556
2010-11-01 15:19:36giampaolo.rodolasetstatus: open -> closed
versions: + Python 3.1, Python 2.7
type: behavior -> security
messages: + msg120132

resolution: fixed
stage: patch review -> resolved
2010-10-30 16:44:26Arfreversetmessages: + msg120003
2010-10-30 16:40:31Arfreversetnosy: + Arfrever
2010-10-04 21:09:57giampaolo.rodolasetmessages: + msg117981
2010-10-03 19:28:59pitrousetstage: needs patch -> patch review
messages: + msg117925
versions: - Python 2.6, Python 3.1, Python 2.7
2010-10-02 13:18:23giampaolo.rodolasetfiles: + accept.patch

messages: + msg117873
2010-09-25 12:34:54giampaolo.rodolasetmessages: + msg117360
2010-09-25 11:56:47pitrousetmessages: + msg117359
2010-09-25 11:43:40giampaolo.rodolasetmessages: + msg117358
2010-09-25 10:54:43pitrousetmessages: + msg117357
2010-09-25 10:37:46giampaolo.rodolasetfiles: + accept.patch

messages: + msg117356
versions: + Python 2.6
2010-09-24 20:15:53giampaolo.rodolasetfiles: + accept.patch

nosy: + pitrou
messages: + msg117329

keywords: + patch
2010-09-23 22:11:47dmalcolmsetnosy: + dmalcolm
messages: + msg117240
2010-09-13 19:32:25santa4ntsetnosy: + santa4nt
2010-09-13 14:05:13matejciksetnosy: + matejcik
2010-08-03 21:51:24giampaolo.rodolasetassignee: giampaolo.rodola
messages: + msg112699
2010-08-01 11:47:47BreamoreBoysetversions: + Python 3.1, Python 3.2
nosy: + BreamoreBoy

messages: + msg112315

type: behavior
stage: needs patch
2009-08-14 23:03:35giampaolo.rodolacreate