Skip to content

Commit

Permalink
MDL-55273 admin: Change $CFG->cookiesecure default to on
Browse files Browse the repository at this point in the history
  • Loading branch information
@brendanheywood
brendanheywood committed Aug 21, 2016
1 parent 3ca3cc7 commit 657ddbf
Show file tree
Hide file tree
Showing 7 changed files with 135 additions and 12 deletions.
2 changes: 1 addition & 1 deletion admin/settings/security.php
View file
Expand Up @@ -109,7 +109,7 @@
// "httpsecurity" settingpage
$temp = new admin_settingpage('httpsecurity', new lang_string('httpsecurity', 'admin'));
$temp->add(new admin_setting_configcheckbox('loginhttps', new lang_string('loginhttps', 'admin'), new lang_string('configloginhttps', 'admin'), 0));
$temp->add(new admin_setting_configcheckbox('cookiesecure', new lang_string('cookiesecure', 'admin'), new lang_string('configcookiesecure', 'admin'), 0));
$temp->add(new admin_setting_configcheckbox('cookiesecure', new lang_string('cookiesecure', 'admin'), new lang_string('configcookiesecure', 'admin'), 1));
$temp->add(new admin_setting_configcheckbox('cookiehttponly', new lang_string('cookiehttponly', 'admin'), new lang_string('configcookiehttponly', 'admin'), 0));
$temp->add(new admin_setting_configcheckbox('allowframembedding', new lang_string('allowframembedding', 'admin'), new lang_string('allowframembedding_help', 'admin'), 0));
$temp->add(new admin_setting_configcheckbox('loginpasswordautocomplete', new lang_string('loginpasswordautocomplete', 'admin'), new lang_string('loginpasswordautocomplete_help', 'admin'), 0));
Expand Down
2 changes: 1 addition & 1 deletion lang/en/admin.php
View file
Expand Up @@ -151,7 +151,7 @@
$string['configcalendarcustomexport'] = 'Enable custom date range export of calendar';
$string['configcalendarexportsalt'] = 'This random text is used for improving of security of authentication tokens used for exporting of calendars. Please note that all current tokens are invalidated if you change this hash salt.';
$string['configcookiehttponly'] = 'Enables new PHP 5.2.0 feature - browsers are instructed to send cookie with real http requests only, cookies should not be accessible by scripting languages. This is not supported in all browsers and it may not be fully compatible with current code. It helps to prevent some types of XSS attacks.';
$string['configcookiesecure'] = 'If server is accepting only https connections it is recommended to enable sending of secure cookies. If enabled please make sure that web server is not accepting http:// or set up permanent redirection to https:// address. When <em>wwwroot</em> address does not start with https:// this setting is turned off automatically.';
$string['configcookiesecure'] = 'If server is accepting only https connections it is recommended to enable sending of secure cookies. If enabled please make sure that web server is not accepting http:// or set up permanent redirection to https:// address and ideally send HSTS headers. When <em>wwwroot</em> address does not start with https:// this setting is ignored.';
$string['configcountry'] = 'If you set a country here, then this country will be selected by default on new user accounts. To force users to choose a country, just leave this unset.';
$string['configcourseoverviewfilesext'] = 'A comma-separated list of allowed course summary files extensions.';
$string['configcourseoverviewfileslimit'] = 'The maximum number of files that can be attached to a course summary.';
Expand Down
6 changes: 2 additions & 4 deletions lib/classes/session/manager.php
View file
Expand Up @@ -191,9 +191,7 @@ public static function init_empty_session() {
protected static function prepare_cookies() {
global $CFG;

if (!isset($CFG->cookiesecure) or (!is_https() and empty($CFG->sslproxy))) {
$CFG->cookiesecure = 0;
}
$cookiesecure = is_moodle_cookie_secure();

if (!isset($CFG->cookiehttponly)) {
$CFG->cookiehttponly = 0;
Expand Down Expand Up @@ -254,7 +252,7 @@ protected static function prepare_cookies() {

// Set configuration.
session_name($sessionname);
session_set_cookie_params(0, $CFG->sessioncookiepath, $CFG->sessioncookiedomain, $CFG->cookiesecure, $CFG->cookiehttponly);
session_set_cookie_params(0, $CFG->sessioncookiepath, $CFG->sessioncookiedomain, $cookiesecure, $CFG->cookiehttponly);
ini_set('session.use_trans_sid', '0');
ini_set('session.use_only_cookies', '1');
ini_set('session.hash_function', '0'); // For now MD5 - we do not have room for sha-1 in sessions table.
Expand Down
29 changes: 25 additions & 4 deletions lib/sessionlib.php
View file
Expand Up @@ -86,6 +86,25 @@ function require_sesskey() {
}
}

/**
* Determine wether the secure flag should be set on cookies
* @return bool
*/
function is_moodle_cookie_secure() {
global $CFG;

if (!isset($CFG->cookiesecure)) {
return false;
}
if (!empty($CFG->loginhttps)) {
return false;
}
if (!is_https() and empty($CFG->sslproxy)) {
return false;
}
return !empty($CFG->cookiesecure);
}

/**
* Sets a moodle cookie with a weakly encrypted username
*
Expand All @@ -111,12 +130,14 @@ function set_moodle_cookie($username) {

$cookiename = 'MOODLEID1_'.$CFG->sessioncookie;

// delete old cookie
setcookie($cookiename, '', time() - HOURSECS, $CFG->sessioncookiepath, $CFG->sessioncookiedomain, $CFG->cookiesecure, $CFG->cookiehttponly);
$cookiesecure = is_moodle_cookie_secure();

// Delete old cookie.
setcookie($cookiename, '', time() - HOURSECS, $CFG->sessioncookiepath, $CFG->sessioncookiedomain, $cookiesecure, $CFG->cookiehttponly);

if ($username !== '') {
// set username cookie for 60 days
setcookie($cookiename, rc4encrypt($username), time()+(DAYSECS*60), $CFG->sessioncookiepath, $CFG->sessioncookiedomain, $CFG->cookiesecure, $CFG->cookiehttponly);
// Set username cookie for 60 days.
setcookie($cookiename, rc4encrypt($username), time() + (DAYSECS * 60), $CFG->sessioncookiepath, $CFG->sessioncookiedomain, $cookiesecure, $CFG->cookiehttponly);
}
}

Expand Down
104 changes: 104 additions & 0 deletions lib/tests/sessionlib_test.php
View file
Expand Up @@ -154,6 +154,110 @@ public function test_cron_setup_user() {
$this->assertSame($GLOBALS['USER'], $USER);
}

/**
* Test provided for secure cookie
*
* @return array of config and secure result
*/
public function moodle_cookie_secure_provider() {
return array(
array(
// Non ssl, not set.
'config' => array(
'wwwroot' => 'http://example.com',
'httpswwwroot' => 'http://example.com',
'sslproxy' => null,
'loginhttps' => null,
'cookiesecure' => null,
),
'secure' => false,
),
array(
// Non ssl, off and ignored.
'config' => array(
'wwwroot' => 'http://example.com',
'httpswwwroot' => 'http://example.com',
'sslproxy' => null,
'loginhttps' => null,
'cookiesecure' => false,
),
'secure' => false,
),
array(
// Non ssl, on and ignored.
'config' => array(
'wwwroot' => 'http://example.com',
'httpswwwroot' => 'http://example.com',
'sslproxy' => null,
'loginhttps' => null,
'cookiesecure' => true,
),
'secure' => false,
),
array(
// SSL via proxy, off.
'config' => array(
'wwwroot' => 'http://example.com',
'httpswwwroot' => 'http://example.com',
'sslproxy' => true,
'loginhttps' => null,
'cookiesecure' => false,
),
'secure' => false,
),
array(
// SSL via proxy, on.
'config' => array(
'wwwroot' => 'http://example.com',
'httpswwwroot' => 'http://example.com',
'sslproxy' => true,
'loginhttps' => null,
'cookiesecure' => true,
),
'secure' => true,
),
array(
// SSL and off.
'config' => array(
'wwwroot' => 'https://example.com',
'httpswwwroot' => 'https://example.com',
'sslproxy' => null,
'loginhttps' => null,
'cookiesecure' => false,
),
'secure' => false,
),
array(
// SSL and on.
'config' => array(
'wwwroot' => 'https://example.com',
'httpswwwroot' => 'https://example.com',
'sslproxy' => null,
'loginhttps' => null,
'cookiesecure' => true,
),
'secure' => true,
),
);
}

/**
* Test for secure cookie
*
* @dataProvider moodle_cookie_secure_provider
*
* @param array $config Array of key value config settings
* @param bool $secure Wether cookies should be secure or not
*/
public function test_is_moodle_cookie_secure($config, $secure) {

$this->resetAfterTest();
foreach ($config as $key => $value) {
set_config($key, $value);
}
$this->assertEquals($secure, is_moodle_cookie_secure());
}

public function test_sesskey() {
global $USER;
$this->resetAfterTest();
Expand Down
2 changes: 1 addition & 1 deletion report/security/lang/en/report_security.php
View file
Expand Up @@ -31,7 +31,7 @@
$string['check_configrw_name'] = 'Writable config.php';
$string['check_configrw_ok'] = 'config.php can not be modified by PHP scripts.';
$string['check_configrw_warning'] = 'PHP scripts may modify config.php.';
$string['check_cookiesecure_details'] = '<p>If you enable https communication it is recommended that you also enable secure cookies. You should also add permanent redirection from http to https.</p>';
$string['check_cookiesecure_details'] = '<p>If you enable https communication it is recommended that you also enable secure cookies. You should also add permanent redirection from http to https. Ideally also serve HSTS headers well.</p>';
$string['check_cookiesecure_error'] = 'Please enable secure cookies';
$string['check_cookiesecure_name'] = 'Secure cookies';
$string['check_cookiesecure_ok'] = 'Secure cookies enabled.';
Expand Down
2 changes: 1 addition & 1 deletion report/security/locallib.php
View file
Expand Up @@ -394,7 +394,7 @@ function report_security_check_cookiesecure($detailed=false) {
$result->status = null;
$result->link = "<a href=\"$CFG->wwwroot/$CFG->admin/settings.php?section=httpsecurity\">".get_string('httpsecurity', 'admin').'</a>';

if (empty($CFG->cookiesecure)) {
if (!is_moodle_cookie_secure()) {
$result->status = REPORT_SECURITY_SERIOUS;
$result->info = get_string('check_cookiesecure_error', 'report_security');
} else {
Expand Down

0 comments on commit 657ddbf

Please sign in to comment.