classification
Title: asynchat does not check if terminator is negative integer
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: Arfrever, BreamoreBoy, devin, giampaolo.rodola, haypo, mmarkk, python-dev
Priority: normal Keywords: patch

Created on 2011-02-20 16:40 by mmarkk, last changed 2014-07-07 22:38 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
z.patch mmarkk, 2011-02-26 14:39
qwe.py mmarkk, 2011-02-26 15:14 long example how this bug may cause protocol errors
shorttest.py mmarkk, 2011-02-26 15:14 Short test if bug exists in python installation
z31.patch mmarkk, 2011-03-03 18:39 the same pathch, but for python 3.1
asynchat_tip.patch devin, 2014-03-09 12:49 review
Messages (16)
msg128914 - (view) Author: Марк Коренберг (mmarkk) Date: 2011-02-20 16:40
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.
msg128921 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-02-20 21:01
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).
msg128938 - (view) Author: Марк Коренберг (mmarkk) Date: 2011-02-21 04:56
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:].
msg129378 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-02-25 15:11
Can you provide a patch including a test case?
msg129546 - (view) Author: Марк Коренберг (mmarkk) Date: 2011-02-26 13:59
Real patch is the first hunk of attached file. Other 2 hunks are optimizations..
msg129557 - (view) Author: Марк Коренберг (mmarkk) Date: 2011-02-26 14:39
only first hunk is really the patch. 2 next hunks are optimizations.
msg129559 - (view) Author: Марк Коренберг (mmarkk) Date: 2011-02-26 15:14
===== 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#
msg129973 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-03-03 14:59
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.
msg129995 - (view) Author: Марк Коренберг (mmarkk) Date: 2011-03-03 18:40
> actual patch which includes tests
I do not understand you. Probably I can not write that patch. Do not know how to. Sorry :(
msg129999 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-03-03 19:08
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/
msg130102 - (view) Author: Марк Коренберг (mmarkk) Date: 2011-03-05 05:00
> 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.
msg182804 - (view) Author: Devin Cook (devin) Date: 2013-02-23 19:52
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.
msg212960 - (view) Author: Devin Cook (devin) Date: 2014-03-09 12:49
updating the patch to the current tip
msg222377 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-05 21:59
I've no objection to people trying to take this forward but they should be aware that asyncio is recommended for new code.
msg222529 - (view) Author: Roundup Robot (python-dev) Date: 2014-07-07 22:35
New changeset f67df13dd512 by Victor Stinner in branch '3.4':
Issue #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 #11259: asynchat.async_chat().set_terminator() now raises a
http://hg.python.org/cpython/rev/d164fda9063a
msg222530 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-07-07 22:38
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!
History
Date User Action Args
2014-07-07 22:38:19hayposetstatus: open -> closed

nosy: + haypo
messages: + msg222530

resolution: fixed
2014-07-07 22:35:10python-devsetnosy: + python-dev
messages: + msg222529
2014-07-05 21:59:59BreamoreBoysetnosy: + BreamoreBoy

messages: + msg222377
versions: + Python 3.4, Python 3.5
2014-03-09 12:49:59devinsetfiles: + asynchat_tip.patch

messages: + msg212960
2014-03-09 12:48:23devinsetfiles: - asynchat_tip.patch
2013-02-23 19:52:12devinsetfiles: + asynchat_tip.patch

nosy: + devin
messages: + msg182804

type: security -> behavior
2011-03-05 05:00:45mmarkksetnosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg130102
2011-03-03 19:08:14giampaolo.rodolasetnosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg129999
2011-03-03 18:40:34mmarkksetnosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg129995
2011-03-03 18:39:01mmarkksetfiles: + z31.patch
nosy: giampaolo.rodola, Arfrever, mmarkk
2011-03-03 14:59:33giampaolo.rodolasetnosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg129973
2011-02-26 15:14:54mmarkksetfiles: + shorttest.py
nosy: giampaolo.rodola, Arfrever, mmarkk
2011-02-26 15:14:20mmarkksetfiles: + qwe.py
nosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg129559
2011-02-26 14:39:13mmarkksetfiles: + z.patch
nosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg129557
2011-02-26 14:00:19mmarkksetfiles: - z.patch
nosy: giampaolo.rodola, Arfrever, mmarkk
2011-02-26 13:59:12mmarkksetfiles: + z.patch

messages: + msg129546
keywords: + patch
nosy: giampaolo.rodola, Arfrever, mmarkk
2011-02-25 15:11:05giampaolo.rodolasetnosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg129378
2011-02-21 04:56:59mmarkksetnosy: giampaolo.rodola, Arfrever, mmarkk
messages: + msg128938
2011-02-20 22:13:47Arfreversetnosy: + Arfrever
2011-02-20 21:01:36giampaolo.rodolasetmessages: + msg128921
2011-02-20 20:31:51pitrousetassignee: giampaolo.rodola

nosy: + giampaolo.rodola
2011-02-20 16:40:47mmarkksettitle: asynchat -> asynchat does not check if terminator is negative integer
2011-02-20 16:40:01mmarkkcreate