From 7d238c6bc41610633b39ca929552bcb77b84fe56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0koda?= Date: Fri, 27 Dec 2013 11:23:43 +0800 Subject: [PATCH] MDL-36721 do not store passwords in config logs --- lib/adminlib.php | 33 +++++++++++-- lib/db/upgrade.php | 39 +++++++++++++++ lib/tests/admintree_test.php | 96 ++++++++++++++++++++++++++++++++++++ version.php | 2 +- 4 files changed, 166 insertions(+), 4 deletions(-) diff --git a/lib/adminlib.php b/lib/adminlib.php index 3156783e5d5..c0b42fe3f41 100644 --- a/lib/adminlib.php +++ b/lib/adminlib.php @@ -1627,7 +1627,20 @@ abstract class admin_setting { rebuild_course_cache(0, true); } - // log change + $this->add_to_config_log($name, $oldvalue, $value); + + return true; // BC only + } + + /** + * Log config changes if necessary. + * @param string $name + * @param string $oldvalue + * @param string $value + */ + protected function add_to_config_log($name, $oldvalue, $value) { + global $DB, $USER; + $log = new stdClass(); $log->userid = during_initial_install() ? 0 :$USER->id; // 0 as user id during install $log->timemodified = time(); @@ -1636,8 +1649,6 @@ abstract class admin_setting { $log->value = $value; $log->oldvalue = $oldvalue; $DB->insert_record('config_log', $log); - - return true; // BC only } /** @@ -2012,6 +2023,22 @@ class admin_setting_configpasswordunmask extends admin_setting_configtext { parent::__construct($name, $visiblename, $description, $defaultsetting, PARAM_RAW, 30); } + /** + * Log config changes if necessary. + * @param string $name + * @param string $oldvalue + * @param string $value + */ + protected function add_to_config_log($name, $oldvalue, $value) { + if ($value !== '') { + $value = '********'; + } + if ($oldvalue !== '' and $oldvalue !== null) { + $oldvalue = '********'; + } + parent::add_to_config_log($name, $oldvalue, $value); + } + /** * Returns XHTML for the field * Writes Javascript into the HTML below right before the last div diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 98d100806df..92db640bb30 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2280,6 +2280,45 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2013051403.02); } + if ($oldversion < 2013051403.09) { + // Purge stored passwords from config_log table, ideally this should be in each plugin + // but that would complicate backporting... + $items = array( + 'core/cronremotepassword', 'core/proxypassword', 'core/smtppass', 'core/jabberpassword', + 'enrol_database/dbpass', 'enrol_ldap/bind_pw', 'url/secretphrase'); + foreach ($items as $item) { + list($plugin, $name) = explode('/', $item); + if ($plugin === 'core') { + $sql = "UPDATE {config_log} + SET value = :value + WHERE name = :name AND plugin IS NULL AND value <> ''"; + $params = array('value'=>'********', 'name'=>$name); + $DB->execute($sql, $params); + + $sql = "UPDATE {config_log} + SET oldvalue = :value + WHERE name = :name AND plugin IS NULL AND oldvalue <> ''"; + $params = array('value'=>'********', 'name'=>$name); + $DB->execute($sql, $params); + + } else { + $sql = "UPDATE {config_log} + SET value = :value + WHERE name = :name AND plugin = :plugin AND value <> ''"; + $params = array('value'=>'********', 'name'=>$name, 'plugin'=>$plugin); + $DB->execute($sql, $params); + + $sql = "UPDATE {config_log} + SET oldvalue = :value + WHERE name = :name AND plugin = :plugin AND oldvalue <> ''"; + $params = array('value'=>'********', 'name'=>$name, 'plugin'=>$plugin); + $DB->execute($sql, $params); + } + } + // Main savepoint reached. + upgrade_main_savepoint(true, 2013051403.09); + } + return true; } diff --git a/lib/tests/admintree_test.php b/lib/tests/admintree_test.php index f633bd718ad..7eb68972ecc 100644 --- a/lib/tests/admintree_test.php +++ b/lib/tests/admintree_test.php @@ -111,4 +111,100 @@ class admintree_testcase extends advanced_testcase { $tree = new admin_root(true); $tree->add('root', new admin_category('bar', 'Bar'), ''); } + + /** + * Saving of values. + */ + public function test_config_logging() { + global $DB; + $this->resetAfterTest(); + + $DB->delete_records('config_log', array()); + + $adminroot = new admin_root(true); + $adminroot->add('root', $one = new admin_category('one', 'One')); + $page = new admin_settingpage('page', 'Page'); + $page->add(new admin_setting_configtext('text1', 'Text 1', '', '')); + $page->add(new admin_setting_configpasswordunmask('pass1', 'Password 1', '', '')); + $adminroot->add('one', $page); + + $this->assertEmpty($DB->get_records('config_log')); + $data = array('s__text1'=>'sometext', 's__pass1'=>''); + $count = $this->save_config_data($adminroot, $data); + + $this->assertEquals(2, $count); + $records = $DB->get_records('config_log', array(), 'id asc'); + $this->assertCount(2, $records); + reset($records); + $record = array_shift($records); + $this->assertNull($record->plugin); + $this->assertSame('text1', $record->name); + $this->assertNull($record->oldvalue); + $this->assertSame('sometext', $record->value); + $record = array_shift($records); + $this->assertNull($record->plugin); + $this->assertSame('pass1', $record->name); + $this->assertNull($record->oldvalue); + $this->assertSame('', $record->value); + + $DB->delete_records('config_log', array()); + $data = array('s__text1'=>'other', 's__pass1'=>'nice password'); + $count = $this->save_config_data($adminroot, $data); + + $this->assertEquals(2, $count); + $records = $DB->get_records('config_log', array(), 'id asc'); + $this->assertCount(2, $records); + reset($records); + $record = array_shift($records); + $this->assertNull($record->plugin); + $this->assertSame('text1', $record->name); + $this->assertSame('sometext', $record->oldvalue); + $this->assertSame('other', $record->value); + $record = array_shift($records); + $this->assertNull($record->plugin); + $this->assertSame('pass1', $record->name); + $this->assertSame('', $record->oldvalue); + $this->assertSame('********', $record->value); + + $DB->delete_records('config_log', array()); + $data = array('s__text1'=>'', 's__pass1'=>''); + $count = $this->save_config_data($adminroot, $data); + + $this->assertEquals(2, $count); + $records = $DB->get_records('config_log', array(), 'id asc'); + $this->assertCount(2, $records); + reset($records); + $record = array_shift($records); + $this->assertNull($record->plugin); + $this->assertSame('text1', $record->name); + $this->assertSame('other', $record->oldvalue); + $this->assertSame('', $record->value); + $record = array_shift($records); + $this->assertNull($record->plugin); + $this->assertSame('pass1', $record->name); + $this->assertSame('********', $record->oldvalue); + $this->assertSame('', $record->value); + } + + protected function save_config_data(admin_root $adminroot, array $data) { + $adminroot->errors = array(); + + $settings = admin_find_write_settings($adminroot, $data); + + $count = 0; + foreach ($settings as $fullname=>$setting) { + /** @var $setting admin_setting */ + $original = $setting->get_setting(); + $error = $setting->write_setting($data[$fullname]); + if ($error !== '') { + $adminroot->errors[$fullname] = new stdClass(); + $adminroot->errors[$fullname]->data = $data[$fullname]; + $adminroot->errors[$fullname]->id = $setting->get_id(); + $adminroot->errors[$fullname]->error = $error; + } + $count++; + } + + return $count; + } } diff --git a/version.php b/version.php index 6d1b3371396..9716aa1ab9e 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2013051403.08; // 20130514 = branching date YYYYMMDD - do not modify! +$version = 2013051403.09; // 20130514 = branching date YYYYMMDD - do not modify! // RR = release increments - 00 in DEV branches // .XX = incremental changes