nelmio / NelmioSecurityBundle

Adds extra security-related features in your Symfony application
https://symfony.com/bundles/NelmioSecurityBundle/
MIT License
651 stars 85 forks source link

Add `legacy_hash_algo` to support backward-compatible `hash_algo` changes #351

Closed martijnc closed 3 months ago

martijnc commented 3 months ago

The goal of this PR is to allow backward-compatible signed cookie hash_algo upgrades. With the current code any existing signed cookies would be rejected when the hash_algo is changed which makes upgrading to a more secure hashing algorithm tricky.

This PR addresses this by;

This is a stepping stone to address #324 but does not yet fix it. Changing the hash_algo default value would still break any existing cookies that don't include the hash algorithm in their signed value yet; the legacy_hash_algo would need to be configured manually for them to keep working. I don't currently see a backward-compatible way to change the hash_algo default value without also (potentially) opening the door for downgrade attacks (e.g. by setting a default value for legacy_hash_algo or hardcoding a sha256 fallback).

This PR also adds a SignerInterface to allow for custom signers.

Seldaek commented 3 months ago

I think it is great to have the option for setting a legacy hash algo, so people can smoothly upgrade cookies, thanks!

IMO having the algo set together with the cookie doesn't add much value, we could simply try to sign using algo, and if that fails try legacy algo if there is one, if it still fails then fail the check?

Another point, it would be nice if when detecting a cookie signed with a legacy algo, a response listener would be registered to update the cookie with a new signature, that way we ensure that cookies get rotated ASAP, and it's easier to say "we set legacy algo for 30days, and after that remove it", making sure that any user hitting the website in those 30days will be fully migrated. Obviously that response listener should be careful to not overwrite cookie removals, or newly written cookies with older values.

Finally just thinking of how we can upgrade the default algo, I guess we'd need to trigger a deprecation if algo is unset to make people set it to something else than sha256, and then in the next major we can change the default.

martijnc commented 3 months ago

Thanks for the feedback!

Updated this PR and removed the changes that added the used algorithm to the cookie value.

Also opened #355 to deprecated the default hash algorithm in the configuration for the next minor.

The upgrade listener is a bit trickier because the bundle needs extra information (expiration, path, ...) than it gets from the browser (only cookie name and value). The application will need to provide the other options for this to work correctly and securely. I've explored this in #356.

Seldaek commented 3 months ago

Thanks!