asynchat does not check if terminator is negative integer. so constructions like self.ac_in_buffer[:n] will lead to misbehaviour.

When that integer goes from net, attack can be crafted. For example, on Content-Length field.

What do you mean by "constructions like self.ac_in_buffer[:n] will lead to misbehaviour."?
Please try to be more precise (e.g. by providing a piece of code which demonstrates the issue).

asynchat.py: class async_chat: handle_read():
-----------------------
elif isinstance(terminator, int) or isinstance(terminator, long):
# numeric terminator
n = terminator
if lb < n:
self.collect_incoming_data (self.ac_in_buffer)
self.ac_in_buffer = ''
self.terminator = self.terminator - lb
else:
self.collect_incoming_data (self.ac_in_buffer[:n])
self.ac_in_buffer = self.ac_in_buffer[n:]
self.terminator = 0
self.found_terminator()
------------------------------
suppose, terminator is -10. "if lb < n" never match. So, "else" branch executed.
next, it will call "self.collect_incoming_data (self.ac_in_buffer[:n])", to push data to user. It should push some data from beginning of the buffer, intead of this, total buffer except last 10 characters pushed.

Moreover, "self.ac_in_buffer = self.ac_in_buffer[n:]" shoudl give tail of the buffer, ut instead of this, "self.ac_in_buffer" will contain part of the tail.

Such behaviour may break protocol parsing. In my case, malicious user pass 'Content-Length: -100' and totally break protocol parsing. Crafted values may gain memory leak.

In any way, author of this code does not thought about negative n in constructions [:n] or [n:].

Can you provide a patch including a test case?

Real patch is the first hunk of attached file. Other 2 hunks are optimizations..

only first hunk is really the patch. 2 next hunks are optimizations.

===== ORIGINAL ===========

$ ./qwe.py 10
read length: 10
read data: "xxxxxxxxxx"
should read "test". read: "test"

$ ./qwe.py -10
read length: -10
read data: "xxxxxx"
should read "test". read: "xxxxtest"

===== PATCHED ===========

$ ./qwe.py 10
read length: 10
read data: "xxxxxxxxxx"
should read "test". read: "test"

$ ./qwe.py -10
read length: -10
error: uncaptured python exception, closing channel <__main__.http_request_handler connected '' at 0x7fe69b9bf878> (<type 'exceptions.ValueError'>:Negative terminator value is not allowed [/usr/lib/python2.6/asyncore.py|read|78] [/usr/lib/python2.6/asyncore.py|handle_read_event|428] [/tmp/qwe/asynchat.py|handle_read|160] [./qwe.py|found_terminator|19] [/tmp/qwe/asynchat.py|set_terminator|98])
root@fad:/tmp/qwe#

Can you write an actual patch which includes tests?
Also, I think the z.patch in attachment is targeted for python 2.x as it does not apply cleanly against the current trunk.

actual patch which includes tests
I do not understand you. Probably I can not write that patch. Do not know how to. Sorry :(

By "writing a test" I mean adding a unittest-based test case in Lib/test/test_asynchat.py which fails before fixing Lib/asynchat.py and succeeds afterwards.

Now, I'm not even sure I properly understood your bug exactly.
I've never used negative terminators in asynchat and I'm not even sure why one would want to.

In this case, taking a look at a test would help me (and others) to understand what you are complaining about exactly and figure out whether this is actually a bug and if it is worth fixing it.

As for how to properly write a patch see:
http://www.python.org/dev/faq/
All new patches should be applied to python 3.3 first so every time you submit a new one you should work on the 3.3 branch (current trunk) which is:
http://svn.python.org/projects/python/branches/py3k/

I've never used negative terminators in asynchat and I'm not even sure why one would want to.
no one wants :), but terminator numeric value may be achieved from the net, and hackers sometimes use such technique.

attached shorttest.py do the test. If negative terminator is passed, ValueError exception will raise on patched version, and will not raise in unpatched.

Currently, I studying test system for python, but I think you will add such simple test faster and cleaner.

I agree that this is probably a bug, but can't think of any instances where this in itself would cause a security issue. By sending something like a negative Content-Length, you do indeed get data returned that doesn't really match the data sent on the wire. If you're able to manipulate the Content-Length, though, instead of sending a negative value num, you could instead send len(data) + num.

Here's a simple example I was able to come up with:

Server reads data and runs "echo -n > {data}" (or any write the file specified in "data").
Client is supposed to send Content-Length, then that many bytes, expected to be a file that should be written to.
Client instead sends "-4\n/etc/passwd.bak".
Server runs "echo -n > /etc/passwd".

So that's certainly unexpected bahavior. However, this is a fairly low-level module, and doesn't actually do anything with the data it collects. That's left to the subclass, and subclasses should be responsible for validating any data read off the wire before using it.

Attached is a patch to tip, including a new test case.

updating the patch to the current tip

I've no objection to people trying to take this forward but they should be aware that asyncio is recommended for new code.

New changeset f67df13dd512 by Victor Stinner in branch '3.4':
Issue bpo-11259: asynchat.async_chat().set_terminator() now raises a ValueError if
http://hg.python.org/cpython/rev/f67df13dd512

New changeset d164fda9063a by Victor Stinner in branch 'default':
(Merge 3.4) Issue bpo-11259: asynchat.async_chat().set_terminator() now raises a
http://hg.python.org/cpython/rev/d164fda9063a

This issue is now fixed, thanks for the report. Sorry for the delay :-(

Asy Mark wrote, asynchat is now deprecated: it's time to switch to the new shiny asyncio module!