[Security] Fix SameSite cookie CSRF protection #7539
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. The session variable that indicates to use it was set to "true". However, it should be a string indicating the value to use (either "strict" or "lax"). See: https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite 2. The use_strict_mode setting (See: https://www.php.net/manual/en/session.configuration.php#ini.session.use-strict-mode) in PHP is not enabled by default. This means that, even when logging out, the session_destroy/session_start procedure reuses the old cookie and does not set a new cookie with the proper samesite flag. All the PHP documentation I've seen says use_strict_mode should always be enabled, but defaults to disabled. This explicitly overrides the php.ini setting (default false) to set it at the beginning of the code, ensuring that NDB_Client properly generates a new session cookie.
driusan
added
Security
ridz1208
approved these changes
Aug 19, 2021
driusan
commented
Aug 19, 2021
| @@ -132,8 +132,8 @@ class NDB_Client | |||
| . $config_additions | |||
| ); | |||
| // start php session | |||
| $sessionOptions = array('cookie_httponly' => true); | |||
| $sessionOptions['cookie_samesite'] = true; | |||
| $sessionOptions = ['cookie_httponly' => true]; | |||
There was a problem hiding this comment.
The bracket style was changed here to avoid conflicts with main (main is using [) when pulling this forward
driusan
commented
Aug 19, 2021
| // PHP documentation says this should always be enabled for session security. | ||
| // PHP documentation says this is disabled by default. | ||
| // Explicitly enable it. | ||
| // phpcs:ignore |
There was a problem hiding this comment.
I don't think there's any choice but to ignore the next line since the URL is so long
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes 2 issues related to the SameSite cookie CSRF protection:
However, it should be a string indicating the value to use (either
"strict" or "lax"). See: https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite
in PHP is not enabled by default. This means that, even when logging out, the
session_destroy/session_start procedure reuses the old cookie and does not set a new cookie with
the proper samesite flag. All the PHP documentation I've seen says use_strict_mode should always
be enabled, but defaults to disabled. This explicitly overrides the php.ini setting (default false)
to set it at the beginning of the code, ensuring that NDB_Client properly generates a new session cookie.