asynchat does not check if terminator is negative integer

[socketpair]

BPO	11259
Nosy	@vstinner, @giampaolo, @socketpair
Files	z.patch qwe.py: long example how this bug may cause protocol errors shorttest.py: Short test if bug exists in python installation z31.patch: the same pathch, but for python 3.1 asynchat_tip.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:

assignee = 'https://github.com/giampaolo'
closed_at = <Date 2014-07-07.22:38:19.205>
created_at = <Date 2011-02-20.16:40:01.224>
labels = ['type-bug', 'library']
title = 'asynchat does not check if terminator is negative integer'
updated_at = <Date 2014-07-07.22:38:19.204>
user = 'https://github.com/socketpair'

bugs.python.org fields:

activity = <Date 2014-07-07.22:38:19.204>
actor = 'vstinner'
assignee = 'giampaolo.rodola'
closed = True
closed_date = <Date 2014-07-07.22:38:19.205>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2011-02-20.16:40:01.224>
creator = 'socketpair'
dependencies = []
files = ['20915', '20916', '20917', '20987', '34310']
hgrepos = []
issue_num = 11259
keywords = ['patch']
message_count = 16.0
messages = ['128914', '128921', '128938', '129378', '129546', '129557', '129559', '129973', '129995', '129999', '130102', '182804', '212960', '222377', '222529', '222530']
nosy_count = 7.0
nosy_names = ['vstinner', 'giampaolo.rodola', 'Arfrever', 'BreamoreBoy', 'devin', 'socketpair', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11259'
versions = ['Python 3.4', 'Python 3.5']

[socketpair]

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.

[giampaolo]

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).

[socketpair]

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:].

[giampaolo]

Can you provide a patch including a test case?

[socketpair]

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

[socketpair]

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

[socketpair]

===== 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#

[giampaolo]

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.

[socketpair]

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

[giampaolo]

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/

[socketpair]

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.