search

SAML-Toolkits / wordpress-saml

OneLogin SAML plugin for Wordpress
MIT License
65 stars 74 forks source link

Use constants for storing cookie names #55

Closed david-binda closed 6 years ago

david-binda commented 6 years ago

Storing cookie names in constants allows overriding the cookie names.

Eg.: on the VIP Go platform (part of the WordPress.com VIP offering), we do automatically whitelist cookies prefixed with wordpress_ in our Varnish configuration. Being able to change cookie names allows us to not introduce new Varnish and Nginx rules for this plugin specifically.

Also, actually, for clients using the plugin on our platform, we had to modify the names which makes it's updates more involved.

david-binda commented 6 years ago

Hey @pitbulk ! Thanks for looking into this PR, really appreciate that.

Could you, please, elaborate on your concerns with alternative cookie names? Especially in relation to deep security review? What security measures in place could be affected?

Cookie names are still present in the code and as they are tight to constants, they can only be changed via other code, thus it does not happen on the fly.

Any platform or user who is doing code review, would still be capable of noticing the change when checking code which is being committed. Also, cookie names are likely to be set once, upon plugin introduction and setup, as changing them inevitably leads to affecting logged in users and thus it's not clever to do so after initial setup.

pitbulk commented 6 years ago

I don't consider myself a security expert, but I was worried with the fact that maybe that change makes a possible "Session Hijacking attack" or similar easier..that why the approve of that change will require an extra review.

david-binda commented 6 years ago

Thanks for the details!

Regarding the "Session Hijacking attack" specifically, the cookie name is not what makes any of techniques mentioned by OWASP possible. Session Hijacking attack, in case of cookie session identification, is more about predictability of the value of the cookie, as that's what differs for individual users. Cookie name stays the same. Also, the cookie used by this pluing is publicly known due to the open source nature of the code :)

Regarding the technique of using overridable constants for cookie names is actually the same to what WordPress itself does - https://core.trac.wordpress.org/browser/trunk/src/wp-includes/default-constants.php?rev=41959#L236

For clarification, the COOKIEHASH part of the cookie name refers to site URL (https://core.trac.wordpress.org/browser/trunk/src/wp-includes/default-constants.php?rev=41959#L206) and helps differentiating cookies for individual sites within multisite install. But again, it's overridable via wp-config.php in case the installation want to use the same cookies across all sites in MS install.

But I do totally get your point about being cautious. Please, let me know if there is something I could help with when it comes to the extra review thing.