Vulnerable to CSRF when Referer header omitted

Bug #529348 reported by William Grant on 2010-02-28
262
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
High
Leonard Richardson

Bug Description

canonical.launchpad.webapp.publication.LaunchpadBrowserPublication.maybeBlockOffsiteFormPost permits requests without a Referer. This is a problem: it is simple for an attacker to get browsers to POST without one (from an ftp:// URL for some browsers, although that doesn't work on Firefox, but data: does). Some users may also use browsers, proxies or plugins that strip Referer if the domains differ. Free CSRF vulnerabilities for everybody!

There may be concerns about causing problems for users who do not send Referer at all, but as of a couple of days ago login.ubuntu.com rejects such requests, and I believe Launchpad will use it for authentication soon.

Related branches

Gary Poster (gary) on 2010-03-01
Changed in launchpad-foundations:
status: New → Triaged
importance: Undecided → High
milestone: none → 10.03
assignee: nobody → Gary Poster (gary)
Gary Poster (gary) wrote :

I agree that closing this security hole is more important than supporting users who strip Referer.

Handling the user complaints may be painful. Maybe we'll already encounter the pain with login.ubuntu.com, as you say.

Looks like all we'd have to do is drop the two lines ``if not referrer: return``.

Thanks

Gary

William Grant (wgrant) wrote :

They won't complain about Launchpad, because they won't get an opportunity to make a POST before they authenticate, which they won't be able to do. ISD will take all of the flaming!

Kees Cook (kees) wrote :

Could we get this fixed sooner that the end of the month?

Gary Poster (gary) wrote :

This will be cherry-picked, yes.

Gary Poster (gary) wrote :

The simple change results in a huge swath of test failures. I'll be addressing this week.

Changed in launchpad-foundations:
status: Triaged → In Progress
William Grant (wgrant) wrote :

So, this was cherrypicked to production a few days back. Since it hasn't landed on any public branch yet, I hunted around for a recent leaked copy of production-devel to have a look at the fix. In it I found this, which skips the checks on some conditions:

        if (IWebServiceClientRequest.providedBy(request) or
            not IBrowserRequest.providedBy(request) or
            'oauth_consumer_key' in request.form or
            'oauth_token' in request.form):
            # We only want to check for the referrer header if we are in
            # the middle of a browser request. If it is a webservice
            # request (which extends a normal browser request) or an
            # XMLRPC request (which doesn't), we can just return.
            # Checking for an oauth request is messy, because it is
            # still a browser request. Even though it is far from
            # satisfying, we check for the specified form fields because
            # it works and another better approach has not yet come to
            # light.
            # XXX gary 2010-03-09 bug=535122
            # Actually, the oauth_token should always be in a normal POST
            # request with a REFERER header, so we should be able to remove
            # that condition when the launchpadlib bug referenced above is
            # fixed.
            return

OK, that rationale is all good. But if you're going to provide four ways to work around the restriction, please ensure that I can't trivially satisfy any of them with my malicious requests.

 - I can easily produce a CSRF IWebServiceClientRequest through a POST to https://launchpad.net/api/devel/something. That's one way to get named op requests through.
 - I can include oauth_consumer_key or oauth_token in my normal form POSTs and the code doesn't seem to notice, except for letting me bypass the CSRF protections. This lets me perform a CSRF just as well as I could before, except that I don't even need to try to cause the Referer to be omitted.

So this fix actually enlarges the hole quite a bit. Well done, well done.

Please let me review the patch in advance in future, or at least don't make me dig around for leaked copies days afterwards.

William Grant (wgrant) wrote :

The oauth_* form field whitelisting was added to let launchpadlib tests pass (launchpadlib/docs/browser.txt, in particular). All of the launchpadlib tests pass if I remove the oauth_* whitelisting, whitelist /+request-token and /+access-token, and teach launchpadlib's test browser to send a Referer when it's forging +authorize-token requests. Since those two views are safe, it would appear to close all except the /api hole.

So something like this:

+ if (IWebServiceClientRequest.providedBy(request) or
+ not IBrowserRequest.providedBy(request) or
+ request['PATH_INFO'] in ('/+storeblob', '/+request-token',
+ '/+access-token')):

Plus adding "'Referer': self.web_root" to the headers dict in both methods of launchpadlib.credentials.SimulatedLaunchpadBrowser.

Then we just need to work out how to distinguish OAuth-authenticated webservice requests from cookie-authenticated ones.

Gary Poster (gary) on 2010-03-16
Changed in launchpad-foundations:
assignee: Gary Poster (gary) → Leonard Richardson (leonardr)
Leonard Richardson (leonardr) wrote :

I think the simplest way to distinguish OAuth-authenticated webservice requests is to create a new marker interface and apply it to the request object in WebServicePublication.getPrincipal(), which is where the OAuth authentication happens.

That part seems simple enough. However, right now I don't understand this bug well enough to know what to do with that interface once it exists...

Leonard Richardson (leonardr) wrote :

I added tests to xx-offsite-form-post that duplicated William's suggested crafted CSRF requests, and then I changed Launchpad to reject those requests. I've pushed the reusulting branch and associated it with this bug.

I still have to change launchpadlib to send requests in the appropriate scenario (SimulatedLaunchpadBrowser is already totally broken, but it's easier to fix it to make these tests pass than to remove it altogether ATM.) AND, I believe I know about another exploitation vector that hasn't been considered yet: anonymous web service access.

In my branch, if you just put random OAuth credentials in your entity-body, you will be subject to the normal referer check. BUT, if your OAuth credentials happen to be a valid signature of the empty token, you are assumed to be making an anonymous web service request and you go through the "OAuth-authenticated webservice request" loophole. The problem is you don't have to have any special knowledge to make an anonymous web service request.

I haven't verified this for sure yet, and I'm not sure how to plug that loophole.

Leonard Richardson (leonardr) wrote :

The anonymous web service access loophole might not matter, because the same code that associates IOAuthSignedRequest with the request object sets the principal to the unauthenticated principal. So you can make a CSRF POST request, but that request isn't associated with the logged-in user and can't modify the dataset. What do you think?

Gary Poster (gary) wrote :

Status: Leonard's branch is ready to go, but because of the apport troubles last time, we are trying to QA the change with apport. We have asked Martin Pitt to help us with this. Once we have done so, we will proceed with requesting a production merge, and with getting it deployed to edge.

Changed in launchpad-foundations:
status: In Progress → Fix Committed
tags: added: qa-needstesting
Gary Poster (gary) on 2010-03-24
Changed in launchpad-foundations:
status: Fix Committed → Fix Released
Gary Poster (gary) on 2010-03-29
tags: added: qa-ok
removed: qa-needstesting
William Grant (wgrant) on 2012-08-10
visibility: private → public
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers