[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Changes by Марк Коренберг :


--
title: asynchat -> asynchat does not check if terminator is negative integer

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
assignee:  -> giampaolo.rodola
nosy: +giampaolo.rodola

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis :


--
nosy: +Arfrever

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Марк Коренберг  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

Can you provide a patch including a test case?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Марк Коренберг  added the comment:

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

--
keywords: +patch
Added file: http://bugs.python.org/file20912/z.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Changes by Марк Коренберг :


Removed file: http://bugs.python.org/file20912/z.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Марк Коренберг  added the comment:

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

--
Added file: http://bugs.python.org/file20915/z.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Марк Коренберг  added the comment:

= ORIGINAL ===
$ ./qwe.py 10
read length: 10
read data: "xx"
should read "test". read: "test"

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

= PATCHED ===
$ ./qwe.py 10
read length: 10
read data: "xx"
should read "test". read: "test"

$ ./qwe.py -10
read length: -10
error: uncaptured python exception, closing channel 
<__main__.http_request_handler connected '' at 0x7fe69b9bf878> (: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#

--
Added file: http://bugs.python.org/file20916/qwe.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Changes by Марк Коренберг :


Added file: http://bugs.python.org/file20917/shorttest.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Changes by Марк Коренберг :


Added file: http://bugs.python.org/file20987/z31.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Марк Коренберг  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

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/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Марк Коренберг]

Марк Коренберг  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11259] asynchat does not check if terminator is negative integer

[Devin Cook]

Devin Cook added the comment:

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.

--
nosy: +devin
type: security -> behavior
Added file: http://bugs.python.org/file29202/asynchat_tip.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com