Fix for Issue #1: Protection from CSRF for OAuth 2.0

Issue tracker:
http://code.google.com/p/gae-simpleauth/issues/detail?id=1

[alexv, 2012-09-13 12:54:59]

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py
File tests/handler_test.py (right):

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py#newcode6
tests/handler_test.py:6: import hmac
Doesn't look like hmac is really needed in the tests

[alexv, 2012-09-13 13:04:12]

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py
File tests/handler_test.py (right):

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py#newcode13
tests/handler_test.py:13: from webapp2 import WSGIApplication, Route,
RequestHandler, cached_property
cached_property isn't needed here either.

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py#newcode14
tests/handler_test.py:14: from webob import Request
BTW, why did I needed to import Request here?

[alexv, 2012-09-13 13:20:12]

http://codereview.appspot.com/6499104/diff/1/README
File README (right):

http://codereview.appspot.com/6499104/diff/1/README#newcode118
README:118: with the provided 'state' parameter on callback.
I should probably mention that this requires a self.session present on the
handler instance.

[alexv, 2012-09-13 13:26:52]

http://codereview.appspot.com/6499104/diff/1/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/1/simpleauth/handler.py#newcode477
simpleauth/handler.py:477: _CSRF_TOKEN_TIMEOUT = 1*60*60 # 1 hour
Rename it to CSRF_TOKEN_TIMEOUT and move it above, close to
OAUTH2_CSRF_SESSION_PARAM so that it'll look more like a configurable constant.

http://codereview.appspot.com/6499104/diff/1/simpleauth/handler.py#newcode521
simpleauth/handler.py:521: _, actual = self._generate_csrf_token(secret,
_time=ts, _base=base)
rename actual to expected.

[alexv, 2012-09-13 19:40:42]

http://codereview.appspot.com/6499104/diff/1/README
File README (right):

http://codereview.appspot.com/6499104/diff/1/README#newcode118
README:118: with the provided 'state' parameter on callback.
On 2012/09/13 13:20:12, crhyme wrote:
> I should probably mention that this requires a self.session present on the
> handler instance.

Done.

http://codereview.appspot.com/6499104/diff/1/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/1/simpleauth/handler.py#newcode477
simpleauth/handler.py:477: _CSRF_TOKEN_TIMEOUT = 1*60*60 # 1 hour
On 2012/09/13 13:26:52, crhyme wrote:
> Rename it to CSRF_TOKEN_TIMEOUT and move it above, close to
> OAUTH2_CSRF_SESSION_PARAM so that it'll look more like a configurable
constant.

Done.

http://codereview.appspot.com/6499104/diff/1/simpleauth/handler.py#newcode521
simpleauth/handler.py:521: _, actual = self._generate_csrf_token(secret,
_time=ts, _base=base)
On 2012/09/13 13:26:52, crhyme wrote:
> rename actual to expected.

Done.

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py
File tests/handler_test.py (right):

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py#newcode6
tests/handler_test.py:6: import hmac
On 2012/09/13 12:54:59, crhyme wrote:
> Doesn't look like hmac is really needed in the tests

Done.

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py#newcode13
tests/handler_test.py:13: from webapp2 import WSGIApplication, Route,
RequestHandler, cached_property
On 2012/09/13 13:04:12, crhyme wrote:
> cached_property isn't needed here either.

Done.

http://codereview.appspot.com/6499104/diff/1/tests/handler_test.py#newcode14
tests/handler_test.py:14: from webob import Request
On 2012/09/13 13:04:12, crhyme wrote:
> BTW, why did I needed to import Request here?

Done.

[alexv, 2012-09-13 21:39:43]

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode483
simpleauth/handler.py:483: Token would normally be stored in a user session
passed as 'state'
... user session *and* passed as ...

[erichiggins, 2012-09-15 02:32:54]

Just a few comments (some are nitpicks), but one potential security issue.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode100
simpleauth/handler.py:100: OAUTH2_CSRF_TOKEN_TIMEOUT = 1*60*60 # 1 hour
Simplify: Just make this 3600 and leave the 1 hour comment.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode188
simpleauth/handler.py:188: self._provider_not_supported(provider)
Optional/FYI/Off-topic: This pattern seems to come up often. Consider raising a
custom exception instead.

class Error(Exception):
  pass

class UnsupportedProvider(Error):
  pass

...

raise UnsupportedProvider

http://docs.python.org/tutorial/errors.html#user-defined-exceptions

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode206
simpleauth/handler.py:206: logging.debug('Redirecting user to %s' % target_url)
FYI: You can use args for string formatting in logging statements (i.e. ","
instead of "%").
logging.debug('foo: %s', 'bar') -> 'foo: bar'

http://docs.python.org/library/logging.html#logging.debug

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode478
simpleauth/handler.py:478: _CSRF_DELIMITER = ':' # token:timestamp
Consider moving this below Line 100 with the other CSRF property definitions.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode486
simpleauth/handler.py:486: if _time is None:
Optional/Style preference:

now = str(_time or long(time.time()))

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode501
simpleauth/handler.py:501: if not(expected and actual) or expected != actual:
You don't need the "not" statement here, since the try/except block below will 
still throw a ValueError.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode505
simpleauth/handler.py:505: token_time =
long(expected.rsplit(self._CSRF_DELIMITER, 1)[-1])
Consider splitting into a tuple as a separate operation to ensure it throws an
exception if malformed. Then you should also validate token_key in some manner
(perhaps ensuring that it at least decodes from base64?).

token_key, token_time = expected.rsplit(self._CSRF_DELIMITER, 1)

Otherwise an evil token can bypass this block:
>>> evil_expected = '1347674724'
>>> long(evil_expected.rsplit(':', 1)[-1])
1347674724L

[alexv, 2012-09-15 11:25:41]

Hey, Eric. Thanks so much for the review. Could you take a look at it one more
time?

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode100
simpleauth/handler.py:100: OAUTH2_CSRF_TOKEN_TIMEOUT = 1*60*60 # 1 hour
On 2012/09/15 02:32:54, erichiggins wrote:
> Simplify: Just make this 3600 and leave the 1 hour comment.

Done.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode188
simpleauth/handler.py:188: self._provider_not_supported(provider)
On 2012/09/15 02:32:54, erichiggins wrote:
> Optional/FYI/Off-topic: This pattern seems to come up often. Consider raising
a
> custom exception instead.
> 
> class Error(Exception):
>   pass
> 
> class UnsupportedProvider(Error):
>   pass
> 
> ...
> 
> raise UnsupportedProvider
> 
> http://docs.python.org/tutorial/errors.html#user-defined-exceptions

Exactly. I will definitely do this in a separate commit replacing them all at
once. Already created another issue so that I won't forget:
http://code.google.com/p/gae-simpleauth/issues/detail?id=2

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode206
simpleauth/handler.py:206: logging.debug('Redirecting user to %s' % target_url)
On 2012/09/15 02:32:54, erichiggins wrote:
> FYI: You can use args for string formatting in logging statements (i.e. ","
> instead of "%").
> logging.debug('foo: %s', 'bar') -> 'foo: bar'
> 
> http://docs.python.org/library/logging.html#logging.debug

Will definitely do this in a separate commit for all logging calls at once, for
consistency.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode478
simpleauth/handler.py:478: _CSRF_DELIMITER = ':' # token:timestamp
On 2012/09/15 02:32:54, erichiggins wrote:
> Consider moving this below Line 100 with the other CSRF property definitions.

Done.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode483
simpleauth/handler.py:483: Token would normally be stored in a user session
passed as 'state'
On 2012/09/13 21:39:43, crhyme wrote:
> ... user session *and* passed as ...

Done.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode486
simpleauth/handler.py:486: if _time is None:
On 2012/09/15 02:32:54, erichiggins wrote:
> Optional/Style preference:
> 
> now = str(_time or long(time.time()))

Done.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode501
simpleauth/handler.py:501: if not(expected and actual) or expected != actual:
On 2012/09/15 02:32:54, erichiggins wrote:
> You don't need the "not" statement here, since the try/except block below will

> still throw a ValueError.

Done.

http://codereview.appspot.com/6499104/diff/7002/simpleauth/handler.py#newcode505
simpleauth/handler.py:505: token_time =
long(expected.rsplit(self._CSRF_DELIMITER, 1)[-1])
On 2012/09/15 02:32:54, erichiggins wrote:
> Consider splitting into a tuple as a separate operation to ensure it throws an
> exception if malformed. Then you should also validate token_key in some manner
> (perhaps ensuring that it at least decodes from base64?).
> 
> token_key, token_time = expected.rsplit(self._CSRF_DELIMITER, 1)
> 
> Otherwise an evil token can bypass this block:
> >>> evil_expected = '1347674724'
> >>> long(evil_expected.rsplit(':', 1)[-1])
> 1347674724L

Done. Nice catch!

[erichiggins, 2012-09-15 15:54:41]

Sure thing, Alex. Great work so far! Just a couple of comments below for your
consideration, but otherwise it LGTM.

http://codereview.appspot.com/6499104/diff/2002/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/2002/simpleauth/handler.py#newcode487
simpleauth/handler.py:487: token = base64.urlsafe_b64encode(os.urandom(30))
I thought about this a little more and wanted to bounce the idea off of you. If
you were to join the random string and timestamp *before* encoding, then it
might make better use of the decode process in _validate_csrf_token.

http://codereview.appspot.com/6499104/diff/2002/simpleauth/handler.py#newcode487
simpleauth/handler.py:487: token = base64.urlsafe_b64encode(os.urandom(30))
Optional: Consider using the random string generator provided by webapp2 instead
of os.urandom. You can always "upgrade" this later.
http://webapp-improved.appspot.com/api/webapp2_extras/security.html

[alexv, 2012-09-16 12:14:30]

Actually, having b64encode(token:timestamp) not only makes it a better use, it
just feels right.  

I've been playing around with this version deployed on
https://simpleauth.appspot.com, and it seems OK to me.

Thanks!

http://codereview.appspot.com/6499104/diff/2002/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/2002/simpleauth/handler.py#newcode487
simpleauth/handler.py:487: token = base64.urlsafe_b64encode(os.urandom(30))
On 2012/09/15 15:54:41, erichiggins wrote:
> I thought about this a little more and wanted to bounce the idea off of you.
If
> you were to join the random string and timestamp *before* encoding, then it
> might make better use of the decode process in _validate_csrf_token.

Done. This does make it a better use!

http://codereview.appspot.com/6499104/diff/2002/simpleauth/handler.py#newcode487
simpleauth/handler.py:487: token = base64.urlsafe_b64encode(os.urandom(30))
On 2012/09/15 15:54:41, erichiggins wrote:
> Optional: Consider using the random string generator provided by webapp2
instead
> of os.urandom. You can always "upgrade" this later.
> http://webapp-improved.appspot.com/api/webapp2_extras/security.html

Agree. Also, better for consistency. Although I did want to make App Engine's
urlfetch and users optional so that this thing could be used with other
frameworks/platforms, I'll probably do it in another CL, all at once, if there's
a feature request.

http://codereview.appspot.com/6499104/diff/2002/simpleauth/handler.py#newcode505
simpleauth/handler.py:505: # Most likely that would mean the user session can be
tampered with
Moved this to the bottom of README file.

http://codereview.appspot.com/6499104/diff/9011/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/9011/simpleauth/handler.py#newcode505
simpleauth/handler.py:505: if not token_key:
Not sure whether it's redundant at this point. OTOH having it here doesn't
really hurt.

[erichiggins, 2012-09-16 19:53:31]

LGTM!

The API docs confirm that using urllib, urllib2, and httplib all go through
urlfetch on GAE servers, so you would be safe using those if you wanted to
support folks using other infrastructures.
https://developers.google.com/appengine/docs/python/urlfetch/overview#Fetchin...

http://codereview.appspot.com/6499104/diff/9011/simpleauth/handler.py
File simpleauth/handler.py (right):

http://codereview.appspot.com/6499104/diff/9011/simpleauth/handler.py#newcode505
simpleauth/handler.py:505: if not token_key:
On 2012/09/16 12:14:31, crhyme wrote:
> Not sure whether it's redundant at this point. OTOH having it here doesn't
> really hurt.
If you don't want to store/verify the original random string (or even it's
length), then check out this optional simplification:

    ...
  assert token_key
except (TypeError, ValueError, UnicodeDecodeError, AssertionError):
  return False

[alexv, 2012-09-16 20:52:31]

Awesome. Thanks again, Eric!

Committed at 
http://code.google.com/p/gae-simpleauth/source/detail?r=72441f4509949f2593a4f...