Simone Pelosi has proposed merging ~pelpsi/launchpad:avoid-open-redirect-attack-on-logout into launchpad:master.
Commit message: Restricted user control on next_to redirect A penetration test found that lougot redirect is vulnerable to open redirect attack. "next_to" url is now validated: if it belongs to our domains, the user is redirected to that url, otherwise the user is redirected to a default url (homepage). Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/439730 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:avoid-open-redirect-attack-on-logout into launchpad:master.
diff --git a/lib/launchpad_loggerhead/app.py b/lib/launchpad_loggerhead/app.py index 8326a12..abd30e4 100644 --- a/lib/launchpad_loggerhead/app.py +++ b/lib/launchpad_loggerhead/app.py @@ -5,7 +5,7 @@ import logging import os import threading import xmlrpc.client -from urllib.parse import urlencode, urljoin +from urllib.parse import urlencode, urljoin, urlparse import oops_wsgi from breezy import errors, lru_cache, urlutils @@ -163,7 +163,8 @@ class RootApp: environ[self.session_var].clear() query = dict(parse_querystring(environ)) next_url = query.get("next_to") - if next_url is None: + next_url_domain = urlparse(next_url).netloc + if next_url is None or next_url_domain not in allvhosts.hostnames: next_url = allvhosts.configs["mainsite"].rooturl raise HTTPMovedPermanently(next_url) diff --git a/lib/launchpad_loggerhead/tests.py b/lib/launchpad_loggerhead/tests.py index 560042c..b39420f 100644 --- a/lib/launchpad_loggerhead/tests.py +++ b/lib/launchpad_loggerhead/tests.py @@ -106,7 +106,7 @@ class TestLogout(TestCase): # TestLoginAndLogout.test_CookieLogoutPage). # Here, we will have a more useless example of the basic machinery. - dummy_root = "http://dummy.test/" + dummy_root = "http://launchpad.test/" self.browser.open( config.codehosting.secure_codebrowse_root + "+logout?" @@ -122,6 +122,26 @@ class TestLogout(TestCase): self.browser.contents, b"This is a dummy destination.\n" ) + def testLoggerheadLogoutRedirectFailure(self): + # When we visit +logout with a 'next_to' value in the query string, + # the logout page will redirect to the given URI only if + # the url belongs to well known domains + + # Here, we will have an example of open redirect attack + dummy_root = "http://launchpad.phishing.test/" + self.browser.open( + config.codehosting.secure_codebrowse_root + + "+logout?" + + urlencode(dict(next_to=dummy_root + "+logout")) + ) + + # We are logged out, as before. + self.assertEqual(self.session, {}) + + # We are redirected to the default homepage because + # the next_to is unknown + self.assertEqual(self.browser.url, "http://launchpad.test/") + class TestWSGI(TestCaseWithFactory): """Smoke tests for Launchpad's loggerhead WSGI server."""
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp