maxcountryman / flask-seasurf

SeaSurf is a Flask extension for preventing cross-site request forgery (CSRF).
http://readthedocs.org/docs/flask-seasurf/
Other
190 stars 49 forks source link

remove secret from hashed cookie value #25

Closed alanhamlett closed 11 years ago

alanhamlett commented 11 years ago

I would like to discuss the reason behind using the app's secret key in the hashed csrf token cookie value.

Hashing the secret key in the CSRF token cookie value means if someone discovers how to predict python's random number generation then they will have an unsalted hash of the app's secret key.

The point of FlaskSeaSurf is to validate the X-CSRFToken header value equals the randomly generated CSRF token cookie value. This ensures the request is coming code running on the same domain as the csrf token cookie. How does using the app's secret key provide any added security?

Some info on when Python's random number generator becomes predictable:

http://www.godsmonsters.com/Features/how-random-python/

http://stackoverflow.com/questions/2145510/python-random-is-barely-random-at-all

https://spideroak.com/blog/20121205114003-exploit-information-leaks-in-random-numbers-from-python-ruby-and-php

maxcountryman commented 11 years ago

Remember that the salt is not solely the app secret key but the key concatenated with a random number which in the best case uses the system random number generator; on BSD at least this provides decent security.

See: http://en.wikipedia.org/wiki/Yarrow_algorithm

alanhamlett commented 11 years ago

Yes, but how does adding the app's secret key increase the randomness of the random number?

maxcountryman commented 11 years ago

The key itself should be randomly generated, e.g. via system random.

alanhamlett commented 11 years ago

Ok, so if we replace the secret key with another randomly generated number then we don't need to use the app's secret key?

maxcountryman commented 11 years ago

You don't have to use a second number at all, it just makes the salt harder to guess.

maxcountryman commented 11 years ago

I don't see any reason to remove the app secret key.

alanhamlett commented 11 years ago

Let's just pretend the random number is predictable.

Scenario 1: Using the app's secret key the csrf token hash

We would then have a hash of the secret key plus this known random number and we can brute force the app's secret key using this hash. Once the secret key is known, it can be used to create login session tokens for any user on the app.

Worst Possible Outcome:

Malicious user can login as any user.

Scenario 2: Removing the app's secret key from csrf token hash

The alternative is to remove the secret key and only use a random number, or create a CSRF_SECRET that we use in the place of the app's secret key. Now if we still pretend the random number is predictable, the secret key is never exposed and the attacker can only do CSRF attacks on users.

Worst Possible Outcome:

Malicious user can send malicious links to users.

Out of these 2 scenarios, which is worse?

maxcountryman commented 11 years ago

Scenario 1 misses the fact that the app key is not the salt. In fact the salt is already using a random number and the app secret key.

I still don't see a case for removing it.

alanhamlett commented 11 years ago

A different explanation of Scenario 1:

The returned value below is the value of the csrf cookie:

salt = (randrange(0, _MAX_CSRF_KEY), self._secret_key)
    return str(hashlib.sha1('%s%s' % salt).hexdigest())

Pretend we can predict the output of randrange() so we will call salt:

salt = (1234, self._secret_key)

Now the sha1 of string '1234%s' % self._secret_key is the csrf cookie value. Since the only unknown is self._secret_key we can guess the value of self._secret_key right?

Can you explain what you mean by "the app key is not the salt"?

maxcountryman commented 11 years ago

Is the only unknown value in Flask's session cookie signing the app secret key?

alanhamlett commented 11 years ago

Yes, unless someone implements a custom session interface for their app.

maxcountryman commented 11 years ago

I guess I don't have a problem with removing the app secret if it makes you feel better. It just seems like there are plenty of other vectors, e.g. the session cookie which is readily available, for an attacker to brute force. So I wonder how viable a brute force attack really is and how likely that could be coupled with an attack that undermines the randomness of non-SystemRandom calls? For what it's worth, it looks like BSD, Linux, and Windows all support SystemRandom and while I can't individually vouch for the cryptographic security of each platform independently I feel confident about BSD variants; I have no idea about Windows.

alanhamlett commented 11 years ago

Continued in issue #26 and closing this.