tgext.routes changes

[Søren Løvborg]

So, sorry for the delay here, had to go and fix CVE-2016-3691, caused
by 1) Routes having a feature to turn GET requests into POST requests,
and 2) Routes enabling this "feature" by default, causing it to be
enabled in Kallithea, apparently without any Kallithea developers
realizing it. (AFAIK, in normal operation, Kallithea only overrides
POST requests.) Not sure I can possibly add anything else to that
discussion, but a good thing we had it, otherwise I'd never have
noticed this security issue.

To get back to the original topic: When overriding the method (of a
POST request...), it makes perfect sense to replace the REQUEST_METHOD
in the environment, too. Except that if you change the method to
DELETE, WebOb will no longer read out POST data (since the concept of
DELETE data is not entirely well-defined), which then means we can't
read the CSRF token, and you get an error 403.

Now, for a real DELETE request (with no overrides involved), you'd
typically specify CSRF token (or API key or similar auth info) in an
HTTP *header*, not the request body. But since we're sending these
requests via an HTML form, that is not an option. It's also possible
to send the CSRF token in the URL, but that's a no-go due to the
greatly increased risk of leaking the token.

Considering that method overrides are designed specifically to
accommodate HTML forms, we could pull the CSRF token out of the POST
request body and stuff it into a header as part of the override
process. But at that point, it just feels like we're digging ourselves
in even deeper. A saner approach would be to phase out method
overrides altogether, and just let POST requests be POST requests.
(Add an "action" argument or similar as needed, but leave that to the
controller, and keep it out of routing and security checks.)

Best,
Søren


On Thu, Apr 14, 2016 at 10:00 PM, Alessandro Molina
<alessandro.molina at gmail.com> wrote:
> On Thu, Apr 14, 2016 at 3:17 PM, Søren Løvborg <sorenl at unity3d.com> wrote:
>
>>
>
>> Sorry, this is going to get long. :-)
>>
>> Thomas De Schampheleire wrote:
>> > So this means updating Kallithea. Do you happen to be interested and
>> > available for such change?
>>
>> Yes. I am currently looking into the Kallithea code to see how this
>> would work. There is definitely room for improvement. I'll get back to
>> you (and the list) when I have something more concrete.
>>
>> Next, I wrote:
>> >>> (Aside: I did not look at the tgext.routes code, but I assume the
>> >>> override support is opt-in? Enabling it automatically for all
>> >>> applications
>> >>> could cause security issues for applications that don't have CSRF
>> >>> protection.)
>>
>> Alessandro Molina replied:
>> > Nope, there is no opt-in.
>> > There isn't in routes itself too:
>> >
>> > https://github.com/bbangert/routes/blob/master/routes/middleware.py#L61-L70
>> >
>> > Also even though you would opt-out you can still perform CSRF in any
>> > case by
>> > using an XMLHTTPRequest or a form.
>>
>> Well, in Routes, it's an opt-out, but the option is there (the
>> use_method_override argument). I think it's a mistake to enable by
>> default.
>>
>> Messing around with the HTTP request like this is definitely not
>> something you should do in a library, unless the application
>> explicitly asks for it, and even then only under certain limited
>> circumstances. This is why:
>>
>
> I'm sorry if my reply made you fervent about the topic, I quickly discarded
> the discussion about opt-in/out just because I found it pretty useless in
> this context. As it doesn't guarantee you are safe from cross site attacks
> and kallithea needed that feature on in any case (it was actually added for
> kallithea itself).
>
> I was more interested on the concern of updating the environ key or not,
> which for consinstency I would do, but it's open to interpretation.
>
> I know that by RFC you should theoretically stick to some behaviours, but in
> practice they are not enforced and the standard itself states it might be
> considered a feature being able to override them. I mean while it's wrong
> and will hit you back for many reasons caching included... the world is full
> of apps that change things on GET requests...
>
> I'll gladly add an option to opt in in tgext.routes if that makes you feel
> more comfortable it won't change much for me as there are no other apps
> apart from Kallithea that replace the whole routing stack in tg with
> tgext.routes and it was made for kallitha so no one will complain ;)