Skip to content

Commit

Permalink
[Security] Fix SameSite cookie CSRF protection (#7539)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
driusan committed Aug 19, 2021
1 parent 3fa96fd commit 13df539
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 2 deletions.
1 change: 1 addition & 0 deletions htdocs/AjaxHelper.php
Expand Up @@ -31,6 +31,7 @@
__DIR__ . "/../project/libraries:" .
__DIR__ . "/../php/libraries"
);
ini_set('session.use_strict_mode', '1');

require_once __DIR__ . "/../vendor/autoload.php";
// Ensures the user is logged in, and parses the config file.
Expand Down
7 changes: 7 additions & 0 deletions htdocs/index.php
Expand Up @@ -22,6 +22,13 @@
// to be done before NDB_Client starts the PHP session.)
session_cache_limiter("");

// PHP documentation says this should always be enabled for session security.
// PHP documentation says this is disabled by default.
// Explicitly enable it.
// phpcs:ignore
// See: https://www.php.net/manual/en/session.configuration.php#ini.session.use-strict-mode
ini_set('session.use_strict_mode', '1');

// FIXME: The code in NDB_Client should mostly be replaced by middleware.
$client = new \NDB_Client;
$client->initialize();
Expand Down
2 changes: 2 additions & 0 deletions htdocs/survey.php
Expand Up @@ -16,6 +16,8 @@
*/
set_include_path(get_include_path().":../project/libraries:../php/libraries:");
ini_set('default_charset', 'utf-8');
ini_set('session.use_strict_mode', '1');

require_once __DIR__ . "/../vendor/autoload.php";
require_once 'NDB_Config.class.inc';
require_once 'Smarty_hook.class.inc';
Expand Down
4 changes: 2 additions & 2 deletions php/libraries/NDB_Client.class.inc
Expand Up @@ -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];
$sessionOptions['cookie_samesite'] = "Strict";

// API Detect
if (strpos(
Expand Down

0 comments on commit 13df539

Please sign in to comment.