Warn the user when redirecting to an external site

[mwahlroos]

CKAN doesn't currently do any checks when redirecting to a URL through /util/redirect. For example http://demo.ckan.org/util/redirect?url=http://www.google.com will send you directly to the target URL with no warning that what you're going to access is not hosted at http://demo.ckan.org.

It might be desirable to warn the user (and give a clickable link with the actual target URL) to avoid situations where e.g. http://reputable-site.com/util/redirect?url=http://undesirable-site.com/ could send oblivious users for example to malware sites.

If you think this is a good idea, I might be able to contribute.

[davidread]

Ouch, yes open redirect is very bad https://www.owasp.org/index.php/Open_redirect

[amercader]

Not sure why we need the /util/redirect endpoint at all. If it is not used I suggest getting rid of it and handle redirects properly from the controllers code

@mwahlroos do you want to do a quick search on the code to see where it is used? (if it is at all)

[mwahlroos]

@amercader it seems to be used by the UI language selector, but a quick search didn't reveal it being used anywhere else.

[mwahlroos]

So, do you think the endpoint should be removed and the language selector modified to use something else instead, or the endpoint fixed?

We'll need to fix this for an extension we're developing anyway, so we might as well do it in a way acceptable for CKAN core rather than diverge with a solution that differs from what you guys are going to do.

I might tend towards removing the endpoint unless there's some other need for it beside the current use in the language selector. At the moment it's pretty much just a thin wrapper around other functionality anyway.

[amercader]

@mwahlroos Sorry for the late reply. I'm happy to remove the endpoint, but we still need to do a redirect on the language selector. Perhaps we can add a quick check on the controller action to only support internal redirects. There is a small function on the user controller that does it:

ckan/ckan/controllers/user.py

Line 692 in 625b51c

def _sane_came_from(self, url):

[mwahlroos]

@amercader I haven't figured out an easy way to modify the language selector so that it wouldn't require some kind of URL redirection accessible through a controller. I guess there could be other options such as handling it in JavaScript somehow rather than doing a server-side redirect.

Rather than modifying the language selector, I've tried adding a check to the redirect action using the method you linked to. However, since the method is currently in the user controller, and accessing the instance methods (or even functions) of a controller from another controller seems icky, I had to move the locality checking method out of the user controller.

One option would be to put it in helpers, but that's already getting quite big. However, I couldn't immediately find a more appropriate home for the function, so I though about putting it in helpers anyway (and renaming the function it since in a more general context the old name is no longer very descriptive).

Does this sound ok to you, or do you see problems with this approach?

[ThrawnCA]

Since CKAN doesn't have any CSRF protection built in, the only practical CSRF defence (without rewriting half the codebase) is to check the referrer header. And having an open redirect - including one that only allows local redirects - nullifies this.

[davidread]

We're now being more strict about discussing security matters privately - perhaps we could start a discussion on this point by you emailing security@lists.ckan.org ?