diff --git a/diskover-web/src/diskover/ConfigDatabase.php b/diskover-web/src/diskover/ConfigDatabase.php index 866d7e8..5c6d51d 100644 --- a/diskover-web/src/diskover/ConfigDatabase.php +++ b/diskover-web/src/diskover/ConfigDatabase.php @@ -170,10 +170,21 @@ public function updateConfigSetting($table, $name, $value) $foundSetting = false; $value = json_encode($value); - $res = $this->db->exec("UPDATE $table SET value='$value' WHERE name = '$name'"); + if (!in_array($table, ['configweb', 'configdiskover'], true)) { + throw new \Exception('Invalid config table.'); + } + + $stmt = $this->db->prepare("UPDATE $table SET value = :value WHERE name = :name"); + if (!$stmt) { + throw new \Exception('There was an error preparing the config setting update.'); + } + $stmt->bindValue(':value', $value, SQLITE3_TEXT); + $stmt->bindValue(':name', $name, SQLITE3_TEXT); + $res = $stmt->execute(); if ($res) { $foundSetting = true; + $res->finalize(); } if (!$foundSetting) { diff --git a/diskover-web/tests/ConfigDatabaseSecurityTest.php b/diskover-web/tests/ConfigDatabaseSecurityTest.php new file mode 100644 index 0000000..bcf00db --- /dev/null +++ b/diskover-web/tests/ConfigDatabaseSecurityTest.php @@ -0,0 +1,61 @@ +exec("CREATE TABLE configweb (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL, value TEXT, UNIQUE(name))"); +$sqlite->exec("CREATE TABLE users (id INTEGER PRIMARY KEY AUTOINCREMENT, username TEXT NOT NULL, password TEXT NOT NULL)"); +$sqlite->exec("INSERT INTO configweb (name, value) VALUES ('SAFE_SETTING', '\"old\"')"); +$sqlite->exec("INSERT INTO users (username, password) VALUES ('admin', 'original')"); + +$configDatabase = new ConfigDatabase(); +$reflection = new ReflectionClass($configDatabase); +$dbProperty = $reflection->getProperty('db'); +$dbProperty->setAccessible(true); +$dbProperty->setValue($configDatabase, $sqlite); + +$configDatabase->updateConfigSetting('configweb', 'SAFE_SETTING', 'new value'); +$storedValue = $sqlite->querySingle("SELECT value FROM configweb WHERE name = 'SAFE_SETTING'"); +assertSameValue('new value', json_decode($storedValue), 'Normal config update did not preserve expected behavior.'); + +$configDatabase->updateConfigSetting('configweb', 'SAFE_SETTING', "safe'; UPDATE users SET password='pwned"); +$password = $sqlite->querySingle("SELECT password FROM users WHERE username = 'admin'"); +assertSameValue('original', $password, 'Stacked SQL in a config value modified the users table.'); + +$configDatabase->updateConfigSetting('configweb', "SAFE_SETTING'; UPDATE users SET password='pwned", 'ignored'); +$password = $sqlite->querySingle("SELECT password FROM users WHERE username = 'admin'"); +assertSameValue('original', $password, 'Stacked SQL in a config name modified the users table.'); + +assertThrows(function () use ($configDatabase) { + $configDatabase->updateConfigSetting("configweb; UPDATE users SET password='pwned'", 'SAFE_SETTING', 'ignored'); +}, 'Invalid config table name was accepted.'); + +$sqlite->close(); +unlink($databaseFile); + +echo "ConfigDatabaseSecurityTest passed\n";