Roundup Tracker - Issues

Issue 2550951

classification
Title: __redirect_to is restricted to absolute url only (Attn Bern, Ralf)
Type: Severity: normal
Components: Versions:
process
Status: new Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: antmail, rouilj
Priority: Keywords: patch

Created on 2017-09-11 12:42 by antmail, last changed 2017-09-12 13:45 by rouilj.

Files
File name Uploaded Description Edit Remove
relative_examine_url.diff antmail, 2017-09-11 12:42
Messages
msg6011 Author: [hidden] (antmail) Date: 2017-09-11 12:42
Hello,

I found that I can't set value of "__redirect_to" to the relative url.

Recently introduced examine_url() function will raise an error in such
case because it expect only an absolute url.

Patch attached.
msg6012 Author: [hidden] (rouilj) Date: 2017-09-11 23:50
Hi Anthony:

In message <668020702.20170911154217@inbox.ru>,
Anthony writes:
>I found that I can't set value of "__redirect_to" to the relative url.

That's correct and intentional. The __redirect_to (and _came_from)
parameter is accessible to the user or to any nasty malware in the
browser.

>Recently introduced examine_url() function will raise an error in such
>case because it expect only an absolute url.

From the comment right above that block:

       To try to prevent XSS attacks, validate that the url that is
       passed in is under self.base for the tracker. This is used to
       clean up "__came_from" and "__redirect_to" form variables used
       by the LoginAction and NewItemAction actions.

I assume the URL is generated by accessing the base url in the tracker
config and using that to generate an absolute url. For example:

    tal:attributes="value string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}"

What is your use case where this must be a relative url?
msg6013 Author: [hidden] (antmail) Date: 2017-09-12 09:19
Hello, John.

> What is your use case where this must be a relative url?

Just something like
<input type="hidden" name="__redirect_to" value="issue">

 I   think   this  variant  has  a much better readability. Also I see
no reason to constraint URL to be absolute inside tracker's html.
Am I wrong?

> That's correct and intentional. The __redirect_to (and _came_from)
> parameter is accessible to the user or to any nasty malware in the
> browser.
>>From the comment right above that block:
>        To try to prevent XSS attacks, validate that the url that is
>        passed in is under self.base for the tracker. This is used to
>        clean up "__came_from" and "__redirect_to" form variables used
>        by the LoginAction and NewItemAction actions.

I  think  the  relative  url  that  is passed in is under self.base by
definition. Is it true?

As you see in patch I pass URL base checking only in case when it is a
relative URL.
msg6014 Author: [hidden] (rouilj) Date: 2017-09-12 13:45
Hello Anthony:

In message <225309719.20170912121932@inbox.ru>,
Anthony writes:
>Anthony added the comment:
>> What is your use case where this must be a relative url?
>
>Just something like
><input type="hidden" name="__redirect_to" value="issue">
>
> I think this  variant  has  a much better readability.

I agree with that.

>Also I see no reason to constraint URL to be absolute inside
>tracker's html. Am I wrong?

If there is an xss attack the __redirect_to can be sent to anywhere. E.G.

  https:/mybadsite.com/tracker/I/will/steal/your/credentials

I.E. the __redirect_to is totally under the control of the attacker
and can be used to redirect the victim to any site. Given the number
of ways to encode/mangle the url (puny code, url encoding...) passed
in __redirect_to, I require an exact match to the base url for the
tracker with no url encoding or other representations. However I am
not sure the code as written is enough.

>> That's correct and intentional. The __redirect_to (and _came_from)
>> parameter is accessible to the user or to any nasty malware in the
>> browser.
>>>From the comment right above that block:
>>        To try to prevent XSS attacks, validate that the url that is
>>        passed in is under self.base for the tracker. This is used to
>>        clean up "__came_from" and "__redirect_to" form variables used
>>        by the LoginAction and NewItemAction actions.
>
>I  think  the  relative  url  that  is passed in is under self.base by
>definition. Is it true?

AFAIK the relative url is "appended" to whatever the page declares as
a base url when the relative url is used *in the page*.

In this case there is no page around the relative url. It is used as
part of a 302/301 redirect. So I am not quite sure what happens in
this case.  Hence I required absolute fully qualified url's.

As a result I think your code to handle relative urls needs to
generate a fully specified url. At the very least the scheme:
http/https should be specified to prevent malicious code in the
browser from downgrading https to http.

Making sure that the url is sanitized and valid is tricky and there
have been multiple issues with that in other code bases.

I see where you are decoding the url to make sure that %2F doesn't
sneak through. That's good. I am just not sure that is sufficient.

>As you see in patch I pass URL base checking only in case when it is a
>relative URL.

Bern, Ralf comments on this? I don't want to introduce a new security
hole like I did when I originally coded the feature that required this
sanitizing patch.

I have a bad feeling that my code isn't a good enough sanitation as
it stands and could be exploited. Adding a new use case (relative
url's) which could also be exploited is not the direction I want to go.

However to address one of Anthony's concerns maybe a
python:make_absolute_url('url'[, 'query']) helper function that is the
equivalent of:

    tal:attributes= "value
       string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}"

might make things more readable. I agree the whole string/tal above is
mouthful and not exactly obvious.

Also if we do handle relative url's in __redirect_to and __came_from,
I think we must always return absolute URL's in the redirect by
prepending the request base and any path info when we raise the
redirect exception.

Comments?
History
Date User Action Args
2017-09-12 13:45:52rouiljsetmessages: + msg6014
title: __redirect_to is restricted to absolute url only -> __redirect_to is restricted to absolute url only (Attn Bern, Ralf)
2017-09-12 09:19:42antmailsetmessages: + msg6013
2017-09-11 23:50:24rouiljsetnosy: + rouilj
messages: + msg6012
2017-09-11 12:42:23antmailcreate