Vulnerable to CSRF when Referer header omitted

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

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!

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

This will be cherry-picked, yes.

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

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.

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.

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...

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.

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?

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.

Fixed in stable r10561 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/10561>