Redmine

FTR, this vulnerability was introduced in r13213, which was supposed to fix another issue. This is not good :(
Rather than trying to parse the back_url and check if it's OK, another option would be (in addition or even instead of the current check) to sign the back_url param with a secret using ActiveSupport::MessageVerifier.
What do you think?

Jean-Philippe Lang wrote:

What do you think?

Hmm, interesting thought. Similar to how OpenID/OAuth are making sure that back_urls are valid. It would certainly make sure the URLs are all coming from "within" Redmine. I am pretty sure that Holger would have an opinion on this, but he's currently on holidays :) I'll ask him next week and post any feedback if you'd like one more opinion.

• no check is done in the current implementation (might be exploited)
• redirect to the referer would not be supported with the solution proposed in #19577-11

Jan from Planio www.plan.io wrote:

I'll ask Holger next week and post any feedback if you'd like one more opinion.

As promised, here's Holger's opinion on this for your consideration:

While it is certainly possible to sign the URL using ActiveSupport's
MessageVerifier class, doing so would significantly complicate matters
for external users of the interface (e.g. external services integrating
into Redmine who want to force a login, OAuth or single sign-on login
provider, ...). Also, it would hide the URL verification behind an
opaque layer of cryptographic "magic" which isn't guaranteed to solve
the issue but makes verification much harder, for devs as well as end users.

Instead, in order to provide an easier verification solution, I'd
propose to define the protocol for these URLs more strictly which would
make verification easier for us.

One way for this is to only allow full URLs with the correct protocol
and hostname as configured in the setting which could then easily be
verified using this Regex (with proper escaping added):

\A#{Setting.protocol}://#{Setting.host_name}(/|\z)

Such a full URL will be passed through all Rails layers unchanged on
redirects. Then, you just need to adapt all places where the back URL is
generated to generate a full URL instead, but this needs roughly the
same effort as when using signed URLs.

Thanks for sharing your thoughts.

While it is certainly possible to sign the URL using ActiveSupport's
MessageVerifier class, doing so would significantly complicate matters
for external users of the interface (e.g. external services integrating
into Redmine who want to force a login, OAuth or single sign-on login
provider, ...).

I'm not sure to get your point. Do you have a specific use case where this would be problematic while the current implementation is not?

Also, it would hide the URL verification behind an
opaque layer of cryptographic "magic" which isn't guaranteed to solve the issue

If we sign URLs generated by Redmine only, how could it not solve the issue? Do you have a particular example in mind?

but makes verification much harder, for devs as well as end users.

That's a concern, but we can leave the back_url param as is (clear text) and add a back_url_token param that would hold the HMAC for verification.

Instead, in order to provide an easier verification solution, I'd
propose to define the protocol for these URLs more strictly which would
make verification easier for us.

One way for this is to only allow full URLs with the correct protocol
and hostname as configured in the setting which could then easily be
verified using this Regex (with proper escaping added):

\A#{Setting.protocol}://#{Setting.host_name}(/|\z)

Such a full URL will be passed through all Rails layers unchanged on
redirects. Then, you just need to adapt all places where the back URL is
generated to generate a full URL instead, but this needs roughly the
same effort as when using signed URLs.

• Generating full URLs with protocol is a hassle, we've got a few tickets about the generated links in email notifications especially from people who run Redmine in a sub-uri
• Redirects to back urls would not work until an admin properly sets the protocol and host name
• This would not work when accessing a Redmine instance with different (eg. internal/external) host names, or different protocols.

And checking a param against a regexp is IMHO much open for attacks than signing generated values. This old Firefox vulnerability is a good example. CRLF injection in a back url param would not work in Redmine but still, see how the malicious URL would perfectly match the regexp above:
https://bugzilla.mozilla.org/show_bug.cgi?id=655389

Jean-Philippe Lang wrote:

Thanks for sharing your thoughts.

While it is certainly possible to sign the URL using ActiveSupport's
MessageVerifier class, doing so would significantly complicate matters
for external users of the interface (e.g. external services integrating
into Redmine who want to force a login, OAuth or single sign-on login
provider, ...).

I'm not sure to get your point. Do you have a specific use case where this would be problematic while the current implementation is not?

I guess it would be for cases where external software needs to redirect to content in Redmine but explicitly wants the user to sign in for it. Currently, this is possible by just composing the URL "from the outside". I agree though, that this case is rather exotic and probably not something that Redmine should worry too much about. If external apps need this, there will certainly also be a way to achieve it with a signed back_url param. It could also be solved by having a config param in configuration.yml that would disable this feature.

Also, it would hide the URL verification behind an
opaque layer of cryptographic "magic" which isn't guaranteed to solve the issue

If we sign URLs generated by Redmine only, how could it not solve the issue? Do you have a particular example in mind?

I guess that Holger's argument was that if someone somehow manages to get a malicious URL into Redmine to get it signed and then redirecred, and we would rely on signed back_url s only for verification, we might still be vulnerable to malicious redirects if there's no additional validity check based on regex for example. I agree that this risk is somehow theoretical, though.

but makes verification much harder, for devs as well as end users.

That's a concern, but we can leave the back_url param as is (clear text) and add a back_url_token param that would hold the HMAC for verification.

Yes, I think it would be great if the back_url would remain "visible" for end users.

Instead, in order to provide an easier verification solution, I'd
propose to define the protocol for these URLs more strictly which would
make verification easier for us.

One way for this is to only allow full URLs with the correct protocol
and hostname as configured in the setting which could then easily be
verified using this Regex (with proper escaping added):

[...]

Such a full URL will be passed through all Rails layers unchanged on
redirects. Then, you just need to adapt all places where the back URL is
generated to generate a full URL instead, but this needs roughly the
same effort as when using signed URLs.

I see a few problems with this solution:

• Generating full URLs with protocol is a hassle, we've got a few tickets about the generated links in email notifications especially from people who run Redmine in a sub-uri
• Redirects to back urls would not work until an admin properly sets the protocol and host name
• This would not work when accessing a Redmine instance with different (eg. internal/external) host names, or different protocols.

And checking a param against a regexp is IMHO much open for attacks than signing generated values. This old Firefox vulnerability is a good example. CRLF injection in a back url param would not work in Redmine but still, see how the malicious URL would perfectly match the regexp above:
https://bugzilla.mozilla.org/show_bug.cgi?id=655389

All valid points, I agree.

Jean-Philippe, do you know any other open source projects or sites, solving this issue using signed params? To be honest, I have never seen it "in the wild" which might also be the reason I'm still on the fence regarding this.

Jan from Planio www.plan.io wrote:

Jean-Philippe, do you know any other open source projects or sites, solving this issue using signed params? To be honest, I have never seen it "in the wild" which might also be the reason I'm still on the fence regarding this.

No. After a quick search, I found this guy from Google that suggests this solution:

Consider signing your redirects. If your website does have a genuine need to provide URL redirects, you can properly hash the destination URL and then include that cryptographic signature as another parameter when doing the redirect. That allows your own site to do URL redirection without opening your URL redirector to the general public.

http://googlewebmastercentral.blogspot.fr/2009/01/open-redirect-urls-is-your-site-being.html

Another option would be to use full URLs in all back_url params as proposed by Holger but use a simpler approach for verification, without using protocol and hostname settings, but #root_url instead. Rails generates the root url for us based on the current request:

back_url.starts_with?(root_url)

This has been fixed and released, therefore I'm opening up this issue.