Comment 6 for bug 529348

Revision history for this message
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.