Open Redirection issue

[garudlaksha1]

Hello,

In IDP initiated login for Mellon, ReturnTo parameter could be really anything and that gets added as relayState. Once assertion is consumed, mellon redirects to relayState arbitrarily. This leads to Open Redirect security issue. Ideally, redirection code should check if its in same domain. Can this be tracked?

For example,
https://host/admin/auth/login?ReturnTo=https://www.google.com&IdP=

Will redirect to Google after successful assertion. This would also lead to Phishing kind of attack.

[RMastop]

Hi @garudlaksha1,

did you find a way to solve, or work around this issue?

[gurtzoo]

/logout?ReturnTo= is also open.

[olavmrk]

@gurtzoo: Logout should have been addressed at the same time as login, in commit 9d28908. It is handled by this piece of code: https://github.com/Uninett/mod_auth_mellon/blob/v0.14.1/auth_mellon_handler.c#L891-L897

Are you still seeing this issue when you set the MellonRedirectDomains option?

[gurtzoo]

Hi,
we are using httpd24-mod_auth_mellon v0.13.1, x86_64

Configure in the Location /mellon the
MellonRedirectDomains [self] domain1 domain2 domain3

trying
https://application.com/mellon/logout?ReturnTo=http:%2f%5cwww.malicious.com

result, we go to the http://www.malicious.com site!

[olavmrk]

@gurtzoo: Thank you for providing a more details! Using your directions I was able to reproduce the issue locally, and have a fix ready.

We would like to credit you for discovering this vulnerability -- what name would you prefer we use? You GitHub profile is a bit sparse 🙂

[gurtzoo]

Hi, i have updated my profile name!
Is this fix already available?
thank you.

[olavmrk]

@gurtzoo: Thanks! The fix is not available quite yet. I am wrapping up the details for publishing a security update of mod_auth_mellon; I hope to get everything in place now, but it may not be ready until tomorrow morning.

[olavmrk]

@gurtzoo: I have now released version 0.14.2 which includes a fix for this vulnerability.

The commit that fixes the vulnerability is 6204142.

[Dmitry-Davidovsky]

Hi @olavmrk ,

trying
https://application.com/mellon/login?ReturnTo=http:www.malicious.com

result, we go to the http://www.malicious.com site!

Discovered by https://github.com/awakenine

[olavmrk]

Hi @Dmitry-Davidovsky,

thank you for letting us know about this issue!

This is the third time that we have seen an issue due to a mismatch between how apr_uri_parse() parses an URL and how browsers parse an URL.

For the previous fixes, we could work around it by blacklisting characters in the URL, but I don't see how to do something similar to solve this issue. This means that a more complicated fix will be necessary to solve this problem, which requires more work.

Unfortunately other tasks here at Uninett needs to take priority, so I don't have time to look at this issue until mid-August at the earliest.

[nluedtke]

This was assigned CVE-2019-13038.

[awakenine]

Hi @Dmitry-Davidovsky,

thank you for letting us know about this issue!

This is the third time that we have seen an issue due to a mismatch between how apr_uri_parse() parses an URL and how browsers parse an URL.

For the previous fixes, we could work around it by blacklisting characters in the URL, but I don't see how to do something similar to solve this issue. This means that a more complicated fix will be necessary to solve this problem, which requires more work.

Unfortunately other tasks here at Uninett needs to take priority, so I don't have time to look at this issue until mid-August at the earliest.

I've investigated a bit. Absense of "//" for apr_uri_parse means that URI is in format [scheme]:[path]

e.g.

uri: 'http:www.example.com:8080/page.html' (ret=0)
	scheme: 'http'
	hostinfo: '(null)'
	user: '(null)'
	password: '(null)'
	hostname: '(null)'
	port_str: '(null)'
	path: 'www.example.com:8080/page.html'
	query: '(null)'
	fragment: '(null)'
	port: '0'
	is_initialized: '1'
	dns_looked_up: '0'
	dns_resolved: '0'

You can add one more condition under the line

mod_auth_mellon/auth_mellon_util.c

Line 118 in 5c29028

if (uri.scheme) {

    /* http and https schemes without hostname are invalid. */
    if (!uri.hostname) {
        return HTTP_BAD_REQUEST; 
    }

I created pull request with this fix. After that you can also remove previous backslash check as it is no longer required.
6204142

[jhrozek]

I can confirm that @awakenine 's fix works. I also can't think of a reason to accept a ReturnTo argument that contains a scheme but no hostname in the URI, but maybe others could. If there is no use-case for such argument, I would vote for merging the patch.

OTOH, I don't completely agree with removal of the backslash check. Maybe my knowledge of the codebase is still not so deep, but it appears that the backslash check is sort of orthogonal to the parsing and moreover am_check_url is used in fewer locations than am_validate_redirect_url and the functions are not codependent.

At the very least, the commit message of the patch that touches am_validate_redirect_url should be a bit better.

[awakenine]

I can confirm that @awakenine 's fix works. I also can't think of a reason to accept a ReturnTo argument that contains a scheme but no hostname in the URI, but maybe others could. If there is no use-case for such argument, I would vote for merging the patch.

OTOH, I don't completely agree with removal of the backslash check. Maybe my knowledge of the codebase is still not so deep, but it appears that the backslash check is sort of orthogonal to the parsing and moreover am_check_url is used in fewer locations than am_validate_redirect_url and the functions are not codependent.

At the very least, the commit message of the patch that touches am_validate_redirect_url should be a bit better.

@jhrozek, thank you for your comment.

As I understand from this conversation backslash check was introduced as a fix to previous bypass. Now, it should be covered with hostname check because apr_uri_parse() will parse any http and https URL without :// as URI [scheme]:[path], and it will cause HTTP_BAD_REQUEST.

uri: 'http:/\example.com/page.html' (ret=0)
	scheme: 'http'
	hostinfo: '(null)'
	user: '(null)'
	password: '(null)'
	hostname: '(null)'
	port_str: '(null)'
	path: '/\example.com/page.html'
	query: '(null)'
	fragment: '(null)'
	port: '0'
	is_initialized: '1'
	dns_looked_up: '0'
	dns_resolved: '0'
psznewuri: 'http:/\example.com/page.html'

Please let me know if you can bypass that.

[olavmrk]

Closing this issue as part of archiving this project. See the announcement for details:

https://github.com/Uninett/mod_auth_mellon/blob/info/README.md