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.

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?

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.

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?

One additional thought on this. Maybe rename examine_url to
examine_or_make_absolute_url with the semantics:

  if not absolute (https?://), prepend baseurl and
     validate the result

  if absolute url, validate as normal

(the validation may effectively be a no-op for now, but
future use could validate references below the baseurl).

Marking this as an RFE as it's not a bug. The absolute url is 
intentional. How to handle this use case while preventing mischief
is an open question.

Consider:

  
https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html

* Where possible, have the user provide short name, ID or token which 
is mapped server-side to a full target URL.

  *    This provides the highest degree of protection against the 
attack tampering with the URL.
  *    Be careful that this doesn't introduce an enumeration 
vulnerability where a user could cycle through IDs to find all 
possible redirect targets

by prepending the site url, I am attempting to use a short name in the 
redirect. This also recommends:

Validating URLs

When attempting to validate and sanitise user-input to determine 
whether the URL is safe, wherever possible you should use a built-in 
library or function to parse the URLs, such as parse_url() in PHP, 
rather than rolling your own parser using regex. Additionally, make 
sure that you take the following into account:

    Input starting with a / to redirect to local pages is not safe. 
//example.org is a valid URL.
    Input starting with the desired domain name is not safe. 
https://example.org.attacker.com is valid.
    Only allow HTTP(S) protocols. All other protocols, including 
JavaScript URIs such as javascript:alert(1) should be blocked
    Data URIs such as data:text/html,<script>alert(document.domain)
</script> should be blocked

I think my technique is guarding against all of these, but it needs to 
be tested.

Actually I misstated. I am requiring absolute url, so I am not using 
their preferred mechanisms.

Hmm.