BUG: Malicious paginate URL blows up (TG1.05)
dazza
Over at http://www.newmetalarmy.com I'm getting an interesting URL coming through that's causing ALL records to be returned from my news table which exhausts my swap memory and forces FreeBSD to kill apache. I've traced through the code and found a potential problem. I thought I'd ask for advice.
The url is formed thus:
http://www.newmetalarmy.com?news_tgp_no=3777&news_tgp_limit=30&news_tgp_ordering=%7B%27created%27%3A%20%5B0%2C%20False%5D%7D
which in english is
?news_tgp_no=3777&news_tgp_limit=30&news_tgp_ordering={'created'%3A [0%2C False]}
Its the ordering parameter here that is troublesome. It gets passed through to turbogears.paginate.sort_data in the ordering parameter. This in turn calls sqlalchemy_get_column("{'created': [0") which returns None (as expected).
The problem is then the code adds the column to a list called key_cols (and here is where I get lost), the code falls through and executes data=list(data) which gets all 300,000 news items and boom.
I've resolved the issue by commenting out some code (http://paste.turbogears.org/paste/3288 lines 32-37).
Does anyone know what's really going on here?
tg-admin info => http://paste.turbogears.org/paste/3289
SQLAchemy version is 0.4.6
FreeBSD version 7.0
Apache version 2.2.9
mod_wsgi version 1.4 (tested with version 2.1 as well)
tia
--
blog: http://www.luckydonkey.com
work: http://www.newmetalarmy.com
Christoph Zwerschke
> The url is formed thus:
> http://www.newmetalarmy.com?news_tgp_no=3777&news_tgp_limit=30&news_tgp_ordering=%7B%27created%27%3A%20%5B0%2C%20False%5D%7D
TG 1.0.5 should not produce such complicated URLs any more. Did you
create this manually?
-- Christoph
dazza
Christoph Zwerschke
> Looking at the apache logs a little more this morning these are infact
> legacy URL's requests coming from gogglebot and slurp. So while not
> malicious they certainly could be crafted and used against any 1.05
> application.
That's true. Just to make clearer what is happening: Sometimes paginate
cannot order a query using an SQL "order by" clause. This can happen
when the underlying data is not created by an SQL query or is using an
attribute that is e.g. a property of the ORM class which does not exist
in the corresponding database table. And, as in your case, this can also
happen when a malicous or invalid old query parameter is used. In this
case, when ordering via SQL is not possible, paginate tries to sort in
memory. Actually this is a very convenient thing, but where you are
dealing with datasets of hundred thousands of items, as in your case,
this is indeed counterproductive and could be used for DOS attacks.
To fix this, I think we need to introduce another paginate parameter,
e.g. "sort_limit" that would give an upper limit for rows that is
allowed to be fetched and sorted in memory. The default value could be
something like 100*limit (i.e. 100 pages of data).
There is a simialr problem with the "_tgp_limit" query parameter. If
"allow_limit_override" is set, then you can also query the whole
dataset. Though "allow_limit_override" is false by default, you often
want to allow users to choose a limit (e.g. 10, 20 or 50 datasets).
So I suggest adding a parameter "max_limit" for the maximum number of
rows a user can request per page. If set to 0, False or None, this could
be interpreted as "don't allow changing the number of rows per page."
The "allow_limit_override" could then be deprecated.
-- Christoph
dazza
I do worry that paginate is starting to suffer from parameter creep though. I understand it's a necessary evil to maintain backward compatibility.
Christoph Zwerschke
> This sounds like a reasonable solution, shall I open a ticket and link
> to this conversation?
>
> I do worry that paginate is starting to suffer from parameter creep
> though. I understand it's a necessary evil to maintain backward
> compatibility.
Yes, but now the milk is already spilt and we need to fix these things.
Adding these safety mechanisms should be considered a bugfix.
If you file a ticket, I'll take care of it and fix it as suggested if
there are no better ideas.
-- Christoph
dazza
Yes, but now the milk is already spilt and we need to fix these things.
Adding these safety mechanisms should be considered a bugfix.
If you file a ticket, I'll take care of it and fix it as suggested if
there are no better ideas.
-- Christoph