diff --git a/composer.json b/composer.json index d78ec15..c39715d 100644 --- a/composer.json +++ b/composer.json @@ -11,11 +11,12 @@ ], "minimum-stability": "stable", "require": { - "php": "^7.4 || ^8.0", + "php": "^8.0", "doctrine/dbal": "^2.10", "symfony/console": "^5.0", "monolog/monolog": "^2.1", - "greenlion/php-sql-parser": "^4.5" + "greenlion/php-sql-parser": "^4.5", + "ext-json": "*" }, "require-dev": { "squizlabs/php_codesniffer": "^3.5", diff --git a/composer.lock b/composer.lock index 9ab38f7..0f39e54 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "7b8d6684195b638e60f856317060cd42", + "content-hash": "7552e896758ab54f300900191d90c89a", "packages": [ { "name": "doctrine/cache", @@ -5819,7 +5819,8 @@ "prefer-stable": false, "prefer-lowest": false, "platform": { - "php": "^7.4 || ^8.0" + "php": "^8.0", + "ext-json": "*" }, "platform-dev": [], "plugin-api-version": "2.3.0" diff --git a/resources/mysql/sample.sql b/resources/mysql/sample.sql index f1d1f8b..7c9769d 100644 --- a/resources/mysql/sample.sql +++ b/resources/mysql/sample.sql @@ -80,3 +80,6 @@ WITH RECURSIVE cte AS ) SELECT i, value FROM cte; + +# Create a passwordless account +CREATE USER IF NOT EXISTS 'localhost_passwordless_user'@'localhost'; \ No newline at end of file diff --git a/src/Cli/Command/RunCommand.php b/src/Cli/Command/RunCommand.php index d8d9cd8..8ad0be4 100644 --- a/src/Cli/Command/RunCommand.php +++ b/src/Cli/Command/RunCommand.php @@ -8,6 +8,7 @@ use Cadfael\Cli\Formatter\Json; use Cadfael\Engine\Check\Account\NotConnecting; use Cadfael\Engine\Check\Account\NotProperlyClosingConnections; +use Cadfael\Engine\Check\Account\PasswordlessAccount; use Cadfael\Engine\Check\Column\CorrectUtf8Encoding; use Cadfael\Engine\Check\Column\LowCardinalityExpensiveStorage; use Cadfael\Engine\Check\Column\ReservedKeywords; @@ -166,6 +167,7 @@ protected function runChecksAgainstSchema( new IndexPrefix(), new UUIDStorage(), new LowCardinalityExpensiveStorage(), + new PasswordlessAccount(), ); if ($load_performance_schema) { diff --git a/src/Engine/Check/Account/PasswordlessAccount.php b/src/Engine/Check/Account/PasswordlessAccount.php new file mode 100644 index 0000000..35ebd8c --- /dev/null +++ b/src/Engine/Check/Account/PasswordlessAccount.php @@ -0,0 +1,89 @@ +getUser()->authentication_string) + && $entity->getUser()->authentication_string; + + $version = $entity->getDatabase()->getVersion(); + + // If we're running MySQL 5.6.*, we only flag an issue if the plugin field is mysql_native_password + $pass |= ( + version_compare($version, '5.7', '<') + && version_compare($version, '5.6', '>=') + && $entity->getUser()->plugin !== 'mysql_native_password' + ); + + // If we're running MySQL 5.5.*, we only flag an issue if the plugin field is NULL or empty + $pass |= ( + version_compare($version, '5.6', '<') + && version_compare($version, '5.5', '>=') + && $entity->getUser()->plugin + ); + + if ($pass) { + return new Report( + $this, + $entity, + Report::STATUS_OK + ); + } + + $status = Report::STATUS_WARNING; + $messages = [ + "Passwordless account detected.", + ]; + + // Can this user be accessed from outside the server localhost? + if ($entity->getUser()->host !== 'localhost' && $entity->getUser()->host !== '127.0.0.1') { + $status = Report::STATUS_CRITICAL; + $messages[] = "Account has access from outside the server."; + } + + return new Report( + $this, + $entity, + $status, + $messages + ); + } + + /** + * @codeCoverageIgnore + */ + public function getReferenceUri(): string + { + return 'https://github.com/xsist10/cadfael/wiki/Passwordless-Account'; + } + + /** + * @codeCoverageIgnore + */ + public function getName(): string + { + return 'Passwordless Account'; + } + + /** + * @codeCoverageIgnore + */ + public function getDescription(): string + { + return "Accounts that don't have a password set could be abused."; + } +} diff --git a/src/Engine/Entity/Account.php b/src/Engine/Entity/Account.php index ba17832..4d315bb 100644 --- a/src/Engine/Entity/Account.php +++ b/src/Engine/Entity/Account.php @@ -6,23 +6,32 @@ use Cadfael\Engine\Entity; use Cadfael\Engine\Entity\Account\NotClosedProperly; +use Cadfael\Engine\Entity\Account\User; class Account implements Entity { - protected string $username; - protected string $host; protected int $current_connections = 0; protected int $total_connections = 0; - + protected User $user; protected Database $database; public ?NotClosedProperly $account_not_closed_properly = null; - public function __construct(string $username, string $host) + protected function __construct() + { + } + + public static function withUser(User $user): Account + { + $account = new Account(); + $account->setUser($user); + return $account; + } + + public static function withRaw(string $username, string $host): Account { - $this->username = $username; - $this->host = $host; + return self::withUser(new User($username, $host)); } /** @@ -99,7 +108,7 @@ public function getDatabase(): Database */ public function getHost(): string { - return $this->host; + return $this->user->host; } /** @@ -110,7 +119,7 @@ public function getHost(): string */ public function getName(): string { - return $this->username; + return $this->user->user; } /** @@ -127,11 +136,21 @@ public function setAccountNotClosedProperly(NotClosedProperly $account_not_close */ public function __toString(): string { - return $this->username . '@' . $this->host; + return $this->user->user . '@' . $this->user->host; } public function isVirtual(): bool { return false; } + + public function getUser(): User + { + return $this->user; + } + + public function setUser(User $user): void + { + $this->user = $user; + } } diff --git a/src/Engine/Entity/Account/User.php b/src/Engine/Entity/Account/User.php new file mode 100644 index 0000000..a699763 --- /dev/null +++ b/src/Engine/Entity/Account/User.php @@ -0,0 +1,131 @@ + $payload This is a query from mysql.user + * @return User + */ + public static function createFromUser(array $payload): User + { + return new User( + $payload['User'], + $payload['Host'], + $payload['Select_priv'] === 'Y', + $payload['Insert_priv'] === 'Y', + $payload['Update_priv'] === 'Y', + $payload['Delete_priv'] === 'Y', + $payload['Create_priv'] === 'Y', + $payload['Drop_priv'] === 'Y', + $payload['Reload_priv'] === 'Y', + $payload['Shutdown_priv'] === 'Y', + $payload['Process_priv'] === 'Y', + $payload['File_priv'] === 'Y', + $payload['Grant_priv'] === 'Y', + $payload['References_priv'] === 'Y', + $payload['Index_priv'] === 'Y', + $payload['Alter_priv'] === 'Y', + $payload['Show_db_priv'] === 'Y', + $payload['Super_priv'] === 'Y', + $payload['Create_tmp_table_priv'] === 'Y', + $payload['Lock_tables_priv'] === 'Y', + $payload['Execute_priv'] === 'Y', + $payload['Repl_slave_priv'] === 'Y', + $payload['Repl_client_priv'] === 'Y', + $payload['Create_view_priv'] === 'Y', + $payload['Show_view_priv'] === 'Y', + $payload['Create_routine_priv'] === 'Y', + $payload['Alter_routine_priv'] === 'Y', + $payload['Create_user_priv'] === 'Y', + $payload['Event_priv'] === 'Y', + $payload['Trigger_priv'] === 'Y', + $payload['Create_tablespace_priv'] === 'Y', + $payload['ssl_type'], + $payload['ssl_cipher'], + $payload['x509_issuer'], + $payload['x509_subject'], + (int)$payload['max_questions'], + (int)$payload['max_updates'], + (int)$payload['max_connections'], + (int)$payload['max_user_connections'], + $payload['plugin'], + $payload['authentication_string'], + $payload['password_expired'] === 'Y', + (int)$payload['password_last_changed'], + (int)$payload['password_lifetime'], + $payload['account_locked'] === 'Y', + $payload['Create_role_priv'] === 'Y', + $payload['Drop_role_priv'] === 'Y', + (int)$payload['Password_reuse_history'], + (int)$payload['Password_reuse_time'], + $payload['Password_require_current'] === 'Y', + json_decode($payload['User_attributes'] ?? '{}', true) + ); + } +} diff --git a/src/Engine/Factory.php b/src/Engine/Factory.php index dabe778..f1fd98e 100644 --- a/src/Engine/Factory.php +++ b/src/Engine/Factory.php @@ -6,6 +6,7 @@ use Cadfael\Engine\Entity\Account; use Cadfael\Engine\Entity\Account\NotClosedProperly; +use Cadfael\Engine\Entity\Account\User; use Cadfael\Engine\Entity\Database; use Cadfael\Engine\Entity\Index\Statistics; use Cadfael\Engine\Entity\Query; @@ -355,7 +356,7 @@ public function getAccounts(Connection $connection): array $this->log()->info("Collecting MySQL user accounts."); $query = 'SELECT * FROM mysql.user'; foreach ($connection->fetchAllAssociative($query) as $row) { - $accounts[] = new Account($row['User'], $row['Host']); + $accounts[] = Account::withUser(User::createFromUser($row)); } } return $accounts; @@ -664,7 +665,7 @@ public function buildDatabase(Connection $connection, array $schema_names, bool $accountNotClosedProperly['host'] ); if (!$account) { - $account = new Account( + $account = Account::withRaw( $accountNotClosedProperly['user'], $accountNotClosedProperly['host'] ); @@ -682,7 +683,7 @@ public function buildDatabase(Connection $connection, array $schema_names, bool foreach ($accountConnections as $accountConnection) { $account = $database->getAccount($accountConnection['USER'], $accountConnection['HOST']); if (!$account) { - $account = new Account($accountConnection['USER'], $accountConnection['HOST']); + $account = Account::withRaw($accountConnection['USER'], $accountConnection['HOST']); $database->addAccount($account); } $account->setCurrentConnections((int)$accountConnection['CURRENT_CONNECTIONS']); diff --git a/tests/Engine/BaseTest.php b/tests/Engine/BaseTest.php index cffd03e..1072ef6 100644 --- a/tests/Engine/BaseTest.php +++ b/tests/Engine/BaseTest.php @@ -4,6 +4,8 @@ namespace Cadfael\Tests\Engine; +use Cadfael\Engine\Entity\Account; +use Cadfael\Engine\Entity\Account\User; use Cadfael\Engine\Entity\Database; use Cadfael\Engine\Entity\Schema; use Cadfael\Engine\Entity\Table; @@ -88,4 +90,9 @@ protected function createVirtualTable(array $override = []): Table ) ); } + + protected function createAccount(string $user, string $host): Account + { + return Account::withRaw($user, $host); + } } \ No newline at end of file diff --git a/tests/Engine/Check/Account/NotConnectingTest.php b/tests/Engine/Check/Account/NotConnectingTest.php index 70c4bd8..736b9b8 100644 --- a/tests/Engine/Check/Account/NotConnectingTest.php +++ b/tests/Engine/Check/Account/NotConnectingTest.php @@ -4,7 +4,6 @@ namespace Cadfael\Tests\Engine\Check\Account; use Cadfael\Engine\Check\Account\NotConnecting; -use Cadfael\Engine\Entity\Account; use Cadfael\Engine\Report; use Cadfael\Tests\Engine\BaseTest; @@ -14,10 +13,10 @@ class NotConnectingTest extends BaseTest public function setUp(): void { - $no_connection_account = new Account('no_connections', 'localhost'); + $no_connection_account = $this->createAccount('no_connections', 'localhost'); $no_connection_account->setTotalConnections(0); - $many_connection_account = new Account('many_connections', 'localhost'); + $many_connection_account = $this->createAccount('many_connections', 'localhost'); $many_connection_account->setTotalConnections(100); $this->accounts = [ diff --git a/tests/Engine/Check/Account/NotProperlyClosingConnectionsTest.php b/tests/Engine/Check/Account/NotProperlyClosingConnectionsTest.php index 030da0d..3188686 100644 --- a/tests/Engine/Check/Account/NotProperlyClosingConnectionsTest.php +++ b/tests/Engine/Check/Account/NotProperlyClosingConnectionsTest.php @@ -4,7 +4,6 @@ namespace Cadfael\Tests\Engine\Check\Account; use Cadfael\Engine\Check\Account\NotProperlyClosingConnections; -use Cadfael\Engine\Entity\Account; use Cadfael\Engine\Entity\Account\NotClosedProperly; use Cadfael\Engine\Report; use Cadfael\Tests\Engine\BaseTest; @@ -15,13 +14,13 @@ class NotProperlyClosingConnectionsTest extends BaseTest public function setUp(): void { - $not_closed_properly_account = new Account('not_closed_properly', 'localhost'); + $not_closed_properly_account = $this->createAccount('not_closed_properly', 'localhost'); $not_closed_properly_account->setAccountNotClosedProperly(NotClosedProperly::createFromEventSummary([ 'not_closed' => 1, 'not_closed_perc' => 5 ])); - $closed_properly_account = new Account('closed_properly_account', 'localhost'); + $closed_properly_account = $this->createAccount('closed_properly_account', 'localhost'); $closed_properly_account->setAccountNotClosedProperly(NotClosedProperly::createFromEventSummary([ 'not_closed' => 0, 'not_closed_perc' => 0 diff --git a/tests/Engine/Check/Account/PasswordlessAccountTest.php b/tests/Engine/Check/Account/PasswordlessAccountTest.php new file mode 100644 index 0000000..dd5f66c --- /dev/null +++ b/tests/Engine/Check/Account/PasswordlessAccountTest.php @@ -0,0 +1,190 @@ +setDatabase($this->createDatabase([ 'version' => $version ])); + + $this->assertTrue( + $check->supports($account), + "Ensure that we care about all accounts." + ); + + $this->assertEquals( + $status, + $check->run($account)->getStatus(), + "test@$host on MySQL $version with plugin:$plugin and authentication_string:$password tests correctly: $status." + ); + } +} diff --git a/tests/Engine/Check/Query/FunctionsOnIndexTest.php b/tests/Engine/Check/Query/FunctionsOnIndexTest.php index 728f231..e060934 100644 --- a/tests/Engine/Check/Query/FunctionsOnIndexTest.php +++ b/tests/Engine/Check/Query/FunctionsOnIndexTest.php @@ -53,7 +53,13 @@ protected function setUp(): void public function test__moo() { $check = new FunctionsOnIndex(); + $this->assertTrue($check->supports($this->query)); + $report = $check->run($this->query); - $this->assertEquals(Report::STATUS_WARNING, $report->getStatus(), "Query should identify modified INDEX column in WHERE statement."); + $this->assertEquals( + Report::STATUS_WARNING, + $report->getStatus(), + "Query should identify modified INDEX column in WHERE statement." + ); } } diff --git a/tests/Engine/Entity/AccountTest.php b/tests/Engine/Entity/AccountTest.php index 28b1278..1f924c6 100644 --- a/tests/Engine/Entity/AccountTest.php +++ b/tests/Engine/Entity/AccountTest.php @@ -5,15 +5,15 @@ use Cadfael\Engine\Entity\Account; use Cadfael\Engine\Entity\Database; -use PHPUnit\Framework\TestCase; +use Cadfael\Tests\Engine\BaseTest; -class AccountTest extends TestCase +class AccountTest extends BaseTest { protected Account $account; protected function setUp(): void { - $this->account = new Account("root", "localhost"); + $this->account = $this->createAccount("root", "localhost"); $this->account->setDatabase(new Database(null)); } diff --git a/tests/Engine/Entity/DatabaseTest.php b/tests/Engine/Entity/DatabaseTest.php index 95cd102..5eab8da 100644 --- a/tests/Engine/Entity/DatabaseTest.php +++ b/tests/Engine/Entity/DatabaseTest.php @@ -3,13 +3,12 @@ namespace Cadfael\Tests\Engine\Entity; -use Cadfael\Engine\Entity\Account; use Cadfael\Engine\Entity\Database; use Cadfael\Engine\Entity\Schema; +use Cadfael\Tests\Engine\BaseTest; use Doctrine\DBAL\DriverManager; -use PHPUnit\Framework\TestCase; -class DatabaseTest extends TestCase +class DatabaseTest extends BaseTest { protected Database $database; @@ -33,9 +32,9 @@ protected function setUp(): void $this->database = new Database($connection); $this->database->setVariables(self::VARIABLES); - $this->database->addAccount(new Account('root', 'localhost')); - $this->database->addAccount(new Account('bob', 'localhost')); - $this->database->addAccount(new Account('alice', '%')); + $this->database->addAccount($this->createAccount('root', 'localhost')); + $this->database->addAccount($this->createAccount('bob', 'localhost')); + $this->database->addAccount($this->createAccount('alice', '%')); } public function test__getName() @@ -50,24 +49,47 @@ public function test__toString() public function test__getAccount() { - $this->assertEquals(new Account('bob', 'localhost'), $this->database->getAccount('bob', 'localhost'), "Verify that we get the expected Account."); - $this->assertNull($this->database->getAccount('bob', '%'), "Our fuzzy domain should not match for Bob."); - $this->assertEquals(new Account('alice', '%'), $this->database->getAccount('alice', 'localhost'), "Our fuzzy domain should match for Alice."); + $this->assertEquals( + $this->createAccount('bob', 'localhost'), + $this->database->getAccount('bob', 'localhost'), + "Verify that we get the expected Account." + ); + $this->assertNull( + $this->database->getAccount('bob', '%'), + "Our fuzzy domain should not match for Bob." + ); + $this->assertEquals( + $this->createAccount('alice', '%'), + $this->database->getAccount('alice', 'localhost'), + "Our fuzzy domain should match for Alice." + ); } public function test__hasPerformanceSchema() { $this->database->setVariables([ 'performance_schema' => 'ON' ]); - $this->assertTrue($this->database->hasPerformanceSchema(), "Ensure we correctly detect that performance schema is on with a normal flag"); + $this->assertTrue( + $this->database->hasPerformanceSchema(), + "Ensure we correctly detect that performance schema is on with a normal flag" + ); $this->database->setVariables([ 'performance_schema' => 'OFF' ]); - $this->assertFalse($this->database->hasPerformanceSchema(), "Ensure we correctly detect that performance schema is off with a normal flag"); + $this->assertFalse( + $this->database->hasPerformanceSchema(), + "Ensure we correctly detect that performance schema is off with a normal flag" + ); $this->database->setVariables([ 'performance_schema_something' => '1' ]); - $this->assertTrue($this->database->hasPerformanceSchema(), "Ensure we correctly detect that performance schema is on with namespace variable."); + $this->assertTrue( + $this->database->hasPerformanceSchema(), + "Ensure we correctly detect that performance schema is on with namespace variable." + ); $this->database->setVariables([]); - $this->assertFalse($this->database->hasPerformanceSchema(), "Ensure we correctly detect that performance schema is off due to lack of namespace variable."); + $this->assertFalse( + $this->database->hasPerformanceSchema(), + "Ensure we correctly detect that performance schema is off due to lack of namespace variable." + ); } public function test__setSchema()