search

Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.97k stars 887 forks source link

Security improvements for "Double-Submit Cookie" CSRF protection #3764

Open jcerjak opened 4 months ago

jcerjak commented 4 months ago

I've noticed some potential issues with the Double-Submit Cookie approach for CSRF protection, implemented by CookieCSRFStoragePolicy.

As discussed via security group email, it's ok to mention them here.

Recommendation 1: implement signed Double-Submit cookie pattern

The approach in pyramid.csrf.CookieCSRFStoragePolicy implements a naive pattern, which is now discouraged by OWASP.

Looks like there is some defense in-depth implemented in pyramid.csrf.check_csrf_origin(), which probably corresponds to OWASP recommendations for using standard headers to verify origin.

However it would still be recommended to implement a signed Double-Submit Cookie approach.

For more info about issues with the naive approach see: https://www.youtube.com/watch?v=2uvrGQEy8i4

Recommendation 2: implement additional defence in depth (host prefixes)

Additional defence in depth protection could be implemented by using cookies with host prefixes, as described in the OWASP recommendation for using cookies with host prefixes to verify origins.

Recommendation 3: use Python secrets module to generate CSRF token

And finally, the CookieCSRFStoragePolicy._token_factory uses uuid.uuid4().hex to generate a random CSRF token. As per OWASP notes on uuids, "Type 4 UUIDs are randomly generated, although whether this is done using a CSPRNG will depend on the implementation".

Python docs do not provide details; as per https://stackoverflow.com/a/53542384 and the CPython source code, uuid4() should be cryptographically secure due to usage of os.urandom(). Not sure about other Python implementations (if they even run Pyramid?), but to be on the safe side, the standard secrets Python module could be used instead (available since Python 3.6).

mmerickel commented 4 months ago

I'm interested in implementations of all of this and would be happy to review a PR. I suspect the signing would need to be done via a new SignedCookieCSRFStoragePolicy and we could deprecate the old one. Or we make the secret optional with a warning if it's not set but that seems somewhat fragile.

There are no real vulnerabilities in a well-behaving browser with the current implementation in Pyramid (which is what CSRF is designed to protect), but I think it'd be great to add these features.