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.
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 (IWebServiceCli entRequest. providedBy( request) or .providedBy( request) or
'oauth_ consumer_ key' in request.form or
'oauth_ token' in request.form):
not IBrowserRequest
# 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 IWebServiceClie ntRequest 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.