From d829e24aa33d1408709d3ffc3980f6dc8264f58d Mon Sep 17 00:00:00 2001 From: gggeek Date: Mon, 14 Oct 2024 23:32:51 +0000 Subject: [PATCH 01/38] WIP add firewall and login form; improve form input fields html; refactoring and improviding phpdocs --- .env | 6 + public/admin/index.php | 20 ++- public/admin/login.php | 41 ++++++ public/admin/logout.php | 13 ++ public/upload/index.php | 4 +- resources/templates/admin/index.html.twig | 5 +- resources/templates/admin/login.html.twig | 26 ++++ .../parts/forms/cr_common_fields.html.twig | 2 +- .../templates/parts/forms/macros.html.twig | 16 ++- resources/templates/upload/index.html.twig | 6 +- src/DotEnvLoader.php | 5 +- src/Entity/User.php | 22 ++- src/Entity/UserRole.php | 10 ++ src/Exception/AccountExpiredException.php | 8 ++ src/Exception/AuthenticationException.php | 8 ++ src/Exception/AuthorizationException.php | 8 ++ src/Exception/BadCredentialsException.php | 7 + src/Exception/FirewallException.php | 8 ++ src/Exception/UserNotFoundException.php | 7 + src/Form/Field.php | 33 ++++- src/Form/Form.php | 7 + src/Form/LoginForm.php | 22 +++ src/Logger.php | 56 ++++++++ src/PHPErrorHandler.php | 6 +- src/Repository/CrashReportRepository.php | 11 +- src/Repository/Field.php | 16 ++- src/Repository/Repository.php | 86 ++---------- src/Repository/Storage/DatabaseTable.php | 53 ------- src/Repository/UserRepository.php | 33 ++++- src/Router.php | 48 ++++++- src/Security/AnonymousUser.php | 23 +++ src/Security/Firewall.php | 131 ++++++++++++++++++ src/Security/Session.php | 85 ++++++++++++ src/Security/UserInterface.php | 20 +++ src/Security/UserProvider.php | 31 +++++ src/Security/UserProviderInterface.php | 8 ++ .../UsernamePasswordAuthenticator.php | 54 ++++++++ src/Singleton.php | 17 +++ src/{Repository => }/Storage/Database.php | 12 +- src/Storage/DatabaseColumn.php | 9 ++ src/Storage/DatabaseTable.php | 110 +++++++++++++++ src/Storage/Session.php | 96 +++++++++++++ src/Templating.php | 2 +- 43 files changed, 1017 insertions(+), 174 deletions(-) create mode 100644 public/admin/login.php create mode 100644 public/admin/logout.php create mode 100644 resources/templates/admin/login.html.twig create mode 100644 src/Entity/UserRole.php create mode 100644 src/Exception/AccountExpiredException.php create mode 100644 src/Exception/AuthenticationException.php create mode 100644 src/Exception/AuthorizationException.php create mode 100644 src/Exception/BadCredentialsException.php create mode 100644 src/Exception/FirewallException.php create mode 100644 src/Exception/UserNotFoundException.php create mode 100644 src/Form/LoginForm.php create mode 100644 src/Logger.php delete mode 100644 src/Repository/Storage/DatabaseTable.php create mode 100644 src/Security/AnonymousUser.php create mode 100644 src/Security/Firewall.php create mode 100644 src/Security/Session.php create mode 100644 src/Security/UserInterface.php create mode 100644 src/Security/UserProvider.php create mode 100644 src/Security/UserProviderInterface.php create mode 100644 src/Security/UsernamePasswordAuthenticator.php create mode 100644 src/Singleton.php rename src/{Repository => }/Storage/Database.php (82%) create mode 100644 src/Storage/DatabaseColumn.php create mode 100644 src/Storage/DatabaseTable.php create mode 100644 src/Storage/Session.php diff --git a/.env b/.env index f68a743..cd4ae1e 100644 --- a/.env +++ b/.env @@ -6,8 +6,14 @@ DB_PASSWORD= APP_DEBUG=false +# NB: should always have a trailing slash ROOT_URL=/ +LOG_DIR=/var/www/VeraCrypt-CrashCollector/var/logs +AUDIT_LOG_FILE=audit.log +# see Psr\Log\LogLevel for valid values +AUDIT_LOG_LEVEL=info + # Algorithm can be set to '2y' (bcrypt), 'argon2i', 'argon2id', the latter 2 only if an appropriate extension is loaded. # If left unspecified, the php default algorithm will be used PWD_HASH_ALGORITHM= diff --git a/public/admin/index.php b/public/admin/index.php index ffe6b4c..7cea1e0 100644 --- a/public/admin/index.php +++ b/public/admin/index.php @@ -2,15 +2,28 @@ require_once(__DIR__ . '/../../autoload.php'); +use Veracrypt\CrashCollector\Entity\UserRole; +use Veracrypt\CrashCollector\Exception\AuthorizationException; use Veracrypt\CrashCollector\Form\CrashReportSearchForm; use Veracrypt\CrashCollector\Repository\CrashReportRepository; use Veracrypt\CrashCollector\Router; +use Veracrypt\CrashCollector\Security\Firewall; use Veracrypt\CrashCollector\Templating; /// @todo get from .env? $pageSizes = [10, 25, 50]; -/// @todo add ACLs +$firewall = Firewall::getInstance(); +$router = new Router(); +$tpl = new Templating(); + +try { + $firewall->require(UserRole::User); + $user = $firewall->getUser(); +} catch (AuthorizationException $e) { + $firewall->displayAdminLoginPage($router->generate(__FILE__, $_GET)); + exit(); +} $reports = []; $numReports = 0; @@ -37,10 +50,9 @@ } } -$tpl = new Templating(); -$router = new Router(); echo $tpl->render('admin/index.html.twig', [ 'form' => $form, 'reports' => $reports, 'num_reports' => $numReports, 'current_page' => $pageNum, 'page_size' => $pageSize, 'num_pages' => ceil($numReports / $pageSize), 'page_sizes' => $pageSizes, - 'form_url' => $router->generate(__FILE__, $form->getQueryStringParts(true)), + 'form_url' => $router->generate(__FILE__, $form->getQueryStringParts(true)), 'user' => $user, + 'logout_url' => $router->generate(__DIR__ . '/logout.php'), 'root_url' => $router->generate(__DIR__ . '/..'), ]); diff --git a/public/admin/login.php b/public/admin/login.php new file mode 100644 index 0000000..0629b08 --- /dev/null +++ b/public/admin/login.php @@ -0,0 +1,41 @@ +generate(__DIR__ . '/index.php')); + +if ($form->isSubmitted()) { + $form->handleRequest(); + if ($form->isValid()) { + $authenticator = new UsernamePasswordAuthenticator(); + $data = $form->getData(); + $redirectUrl = $data['redirect']; + if (!$router->match($redirectUrl)) { + $form->setError('Tsk tsk tsk'); + } else { + unset($data['redirect']); + try { + $authenticator->authenticate(...$data); + header('Location: ' . $redirectUrl, true, 303); + exit(); + } catch (AuthenticationException $e) { + /// @todo should we reduce the level of info shown? Eg. not tell apart unknown user from bad password + $form->setError($e->getMessage()); + } + } + } +} else { + /// @todo should we give some info or warning if the user is logged in already? +} + +$tpl = new Templating(); +echo $tpl->render('admin/login.html.twig', [ + 'form' => $form, 'form_url' => $router->generate(__FILE__), 'root_url' => $router->generate(__DIR__ . '/..') +]); diff --git a/public/admin/logout.php b/public/admin/logout.php new file mode 100644 index 0000000..60bcc9f --- /dev/null +++ b/public/admin/logout.php @@ -0,0 +1,13 @@ +logoutUser(true); + +$router = new Router(); +// Note: unlike 301, 303 responses are not cacheable by default, unless accompanied by cache-control headers, as +// specified in https://datatracker.ietf.org/doc/html/rfc7231#section-6.1 +header('Location: ' . $router->generate(__DIR__ . '/index.php'), true, 303); diff --git a/public/upload/index.php b/public/upload/index.php index b0e6d49..4558991 100644 --- a/public/upload/index.php +++ b/public/upload/index.php @@ -22,8 +22,8 @@ $form->handleRequest($_GET); } -$tpl = new Templating(); $router = new Router(); +$tpl = new Templating(); echo $tpl->render('upload/index.html.twig', [ - 'form' => $form, 'form_url' => $router->generate(__FILE__) + 'form' => $form, 'form_url' => $router->generate(__FILE__), 'root_url' => $router->generate(__DIR__ . '/..') ]); diff --git a/resources/templates/admin/index.html.twig b/resources/templates/admin/index.html.twig index 5f039e7..42b4508 100644 --- a/resources/templates/admin/index.html.twig +++ b/resources/templates/admin/index.html.twig @@ -3,7 +3,10 @@ {% block body %} {% import "parts/forms/macros.html.twig" as forms %}
-

VeraCrypt Crash Reporter search

+
{{ include('parts/forms/cr_common_fields.html.twig') }}
diff --git a/resources/templates/admin/login.html.twig b/resources/templates/admin/login.html.twig new file mode 100644 index 0000000..bfcf207 --- /dev/null +++ b/resources/templates/admin/login.html.twig @@ -0,0 +1,26 @@ +{% extends "base.html.twig" %} + +{% block body %} +{% import "parts/forms/macros.html.twig" as forms %} +
+ +{# @todo make the form a bit less wide #} + +
+ {{ forms.input(form.getField('username'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }} +
+
+ {{ forms.input(form.getField('password'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} +
+ {{ forms.input(form.getField('redirect'), {}) }} + {% if not form.isValid() %} +

{{ form.errorMessage }}

+ {% endif %} +
+ {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }} +
+ +
+{% endblock %} diff --git a/resources/templates/parts/forms/cr_common_fields.html.twig b/resources/templates/parts/forms/cr_common_fields.html.twig index 93480af..0729e71 100644 --- a/resources/templates/parts/forms/cr_common_fields.html.twig +++ b/resources/templates/parts/forms/cr_common_fields.html.twig @@ -2,6 +2,6 @@ {% for field_name in ['programVersion', 'osVersion', 'hwArchitecture', 'executableChecksum', 'errorCategory', 'errorAddress'] %}
- {{ forms.input(form.getField(field_name), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} + {{ forms.input(form.getField(field_name), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }}
{% endfor %} diff --git a/resources/templates/parts/forms/macros.html.twig b/resources/templates/parts/forms/macros.html.twig index dbfa5b0..5c0a43f 100644 --- a/resources/templates/parts/forms/macros.html.twig +++ b/resources/templates/parts/forms/macros.html.twig @@ -1,20 +1,24 @@ -{% macro input(field, css_classes=[]) %} +{% macro input(field, css_classes=[], input_attributes=[]) %} {# @todo allow better styling of the error message: align it below the input field #} {% if field.inputType == 'datetime' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'hidden' %} - + {% elseif field.inputType == 'submit' %} -
+
+ {% elseif field.inputType == 'password' %} + +
+ {% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'text' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'textarea' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% endif %} {% endmacro %} diff --git a/resources/templates/upload/index.html.twig b/resources/templates/upload/index.html.twig index b76b208..c68262a 100644 --- a/resources/templates/upload/index.html.twig +++ b/resources/templates/upload/index.html.twig @@ -3,14 +3,16 @@ {% block body %} {% import "parts/forms/macros.html.twig" as forms %}
-

VeraCrypt Crash Reporter

+ {% if form.isSubmitted() and form.isValid() %}

Thank you for taking the time to submit the information and sorry for your hassles.

{% else %}
{{ include('parts/forms/cr_common_fields.html.twig') }}
- {{ forms.input(form.getField('callStack'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} + {{ forms.input(form.getField('callStack'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }}
{{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }} diff --git a/src/DotEnvLoader.php b/src/DotEnvLoader.php index 2773438..87c4bd9 100644 --- a/src/DotEnvLoader.php +++ b/src/DotEnvLoader.php @@ -7,7 +7,7 @@ class DotEnvLoader { /// @see https://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap08.html - protected static $VARNAME_REGEX = '/^(?:export[ \t]+)?([a-zA-Z_][a-zA-Z0-9_]*)/'; + protected static string $VARNAME_REGEX = '/^(?:export[ \t]+)?([a-zA-Z_][a-zA-Z0-9_]*)/'; /** * Loads values from a .env file and the corresponding .env.local file if they exist. @@ -108,7 +108,8 @@ protected function populate(array $values, bool $overrideExistingVars = true): v } $_ENV[$name] = $value; - /// @todo should we log a warning in case of a .env var starting with HTTP_ ? + /// @todo should we log a warning in case of a .env var starting with HTTP_ ? Note hat the logger class + /// depends on the dotenv config having been set up already... if (!str_starts_with($name, 'HTTP_')) { $_SERVER[$name] = $value; } diff --git a/src/Entity/User.php b/src/Entity/User.php index 77428c7..d282724 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -4,6 +4,7 @@ use DateTimeImmutable; use DateTimeInterface; +use Veracrypt\CrashCollector\Security\UserInterface; /** * @property-read int $dateJoined @@ -11,7 +12,7 @@ * @property-read int|null $lastLogin * @property-read DateTimeImmutable|null $lastLoginDT */ -class User +class User implements UserInterface { // we store timestamps as properties instead of datetimes in order to be able to use PDO automatic hydration to object private int $dateJoined; @@ -41,6 +42,25 @@ public function __construct( } } + public function getRoles(): array + { + $roles = [UserRole::User]; + if ($this->isSuperuser) { + $roles[] = UserRole::Admin; + } + return $roles; + } + + public function getUserIdentifier(): string + { + return $this->username; + } + + public function isActive(): bool + { + return $this->isActive; + } + public function __get($name) { switch ($name) { diff --git a/src/Entity/UserRole.php b/src/Entity/UserRole.php new file mode 100644 index 0000000..12c676c --- /dev/null +++ b/src/Entity/UserRole.php @@ -0,0 +1,10 @@ +value = $value; + // constraint validation + foreach ($this->constraints as $constraint => $targetValue) { + switch ($constraint) { + case FC::Required: + break; + case FC::MaxLength: + if ($targetValue < 0) { + throw new \DomainException("Unsupported field maxlength: $targetValue"); + } + break; + default: + throw new \DomainException("Unsupported field constraint: '$constraint"); + } + } } /** @@ -68,7 +84,6 @@ public function setValue(mixed $value): bool } break; case FC::MaxLength: - /// @todo throw if $targetValue < 0 if ($targetValue > 0 && strlen($value) > $targetValue) { $this->errorMessage = "Value should not be longer than {$targetValue} characters"; return false; @@ -93,6 +108,16 @@ public function getData(): null|string|\DateTimeImmutable }; } + public function isRequired(): bool + { + return array_key_exists(FC::Required, $this->constraints) && $this->constraints[FC::Required]; + } + + public function getMaxLength(): ?int + { + return array_key_exists(FC::MaxLength, $this->constraints) ? (int)$this->constraints[FC::MaxLength] : null; + } + public function __get($name) { switch ($name) { diff --git a/src/Form/Form.php b/src/Form/Form.php index c681f87..35c683c 100644 --- a/src/Form/Form.php +++ b/src/Form/Form.php @@ -58,6 +58,7 @@ public function isValid(): bool return $this->isValid; } + /// @todo if a hidden field is invalid, set $this->errorMessage to something (what?) public function handleRequest(?array $request = null): void { $this->isValid = true; @@ -113,6 +114,12 @@ protected function getRequest() } } + public function setError(string $errorMessage) + { + $this->errorMessage = $errorMessage; + $this->isValid = false; + } + public function __get($name) { switch ($name) { diff --git a/src/Form/LoginForm.php b/src/Form/LoginForm.php new file mode 100644 index 0000000..2b7015b --- /dev/null +++ b/src/Form/LoginForm.php @@ -0,0 +1,22 @@ +fields = [ + /// @todo get the field length from the Repo fields + 'username' => new Field('Username', 'un', 'text', [FC::Required => true, FC::MaxLength => 180]), + 'password' => new Field('Password', 'pw', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'redirect' => new Field('', 'r', 'hidden', [FC::Required => true], $redirect) + ]; + } +} diff --git a/src/Logger.php b/src/Logger.php new file mode 100644 index 0000000..172261c --- /dev/null +++ b/src/Logger.php @@ -0,0 +1,56 @@ + 100, + LogLevel::INFO => 200, + LogLevel::NOTICE => 250, + LogLevel::WARNING => 300, + LogLevel::ERROR => 400, + LogLevel::CRITICAL => 500, + LogLevel::ALERT => 550, + LogLevel::EMERGENCY => 600, + ]; + + public function __construct(string|\Stringable $logFile, $logLevel) + { + if (!str_contains('/', $logFile)) { + $logDir = $_ENV['LOG_DIR']; + if ($logDir != '') { + $logDir = rtrim($logDir, '/') . '/'; + } + $logFile = $logDir . $logFile; + } + $this->logFile = $logFile; + $this->logLevel = $logLevel; + } + + /** + * Logs with an arbitrary level. + * + * @param string $level + */ + public function log($level, string|\Stringable $message, array $context = []): void + { + /// @todo throw a \DomainException if $level is unsupported + + if ($this->levelWeights[$this->logLevel] > $this->levelWeights[$level]) { + return; + } + + file_put_contents($this->logFile, + date('c') . ' ' . strtoupper($level) . ' ' . $message . ' ' . str_replace("\n", ' ', json_encode($context)) . "\n", + FILE_APPEND + ); + } +} diff --git a/src/PHPErrorHandler.php b/src/PHPErrorHandler.php index aa4d60f..e46337a 100644 --- a/src/PHPErrorHandler.php +++ b/src/PHPErrorHandler.php @@ -7,13 +7,13 @@ */ class PHPErrorHandler { - protected $errorTypesToHandle = array( + protected array $errorTypesToHandle = [ E_ERROR => 'E_ERROR', E_PARSE => 'E_PARSE', E_USER_ERROR => 'E_USER_ERROR', E_COMPILE_ERROR => 'E_COMPILE_ERROR', E_RECOVERABLE_ERROR => 'E_RECOVERABLE_ERROR', - ); + ]; public function handle(): void { @@ -64,7 +64,7 @@ protected function shouldHandleError(array $error): bool /** * NB: this only works insofar at least the dotenv-based loading of env vars has succeeded - * @todo decide how to handle - log, email, other? + * @todo decide how to handle - log, email, Slack, other? */ protected function notifyOfError(array $error): void { diff --git a/src/Repository/CrashReportRepository.php b/src/Repository/CrashReportRepository.php index aaa26b0..a187989 100644 --- a/src/Repository/CrashReportRepository.php +++ b/src/Repository/CrashReportRepository.php @@ -8,8 +8,12 @@ class CrashReportRepository extends Repository { - protected $tableName = 'crash_report'; + protected string $tableName = 'crash_report'; + /** + * @throws \DomainException in case of unsupported database type + * @trows \PDOException + */ public function __construct() { /// @todo add a 'hash' column as PK instead of the ID? If so, it could/should probably include the source IP too... @@ -30,6 +34,7 @@ public function __construct() /** * Note: this does not validate the length of the fields, nor truncate them. The length validation is left to the Form + * @throws \PDOException */ public function createReport(string $programVersion, string $osVersion, string $hwArchitecture, string $executableChecksum, string $errorCategory, string $errorAddress, string $callStack): CrashReport @@ -43,6 +48,7 @@ public function createReport(string $programVersion, string $osVersion, string $ /** * @return CrashReport[] + * @throws \PDOException */ public function searchReports(int $limit, int $offset = 0, ?string $programVersion = null, ?string $osVersion = null, ?string $hwArchitecture = null, ?string $executableChecksum = null, ?string $errorCategory = null, ?string $errorAddress = null, @@ -69,6 +75,9 @@ public function searchReports(int $limit, int $offset = 0, ?string $programVersi return array_map(static fn($result) => new CrashReport(...$result), $results); } + /** + * @throws \PDOException + */ public function countReports(?string $programVersion = null, ?string $osVersion = null, ?string $hwArchitecture = null, ?string $executableChecksum = null, ?string $errorCategory = null, ?string $errorAddress = null, null|int|DateTimeInterface $minDate = null, null|int|DateTimeInterface $maxDate = null): int diff --git a/src/Repository/Field.php b/src/Repository/Field.php index 111d252..4990805 100644 --- a/src/Repository/Field.php +++ b/src/Repository/Field.php @@ -2,12 +2,22 @@ namespace Veracrypt\CrashCollector\Repository; +use Veracrypt\CrashCollector\Storage\DatabaseColumn; + class Field { + use DatabaseColumn; + + /** + * @param mixed $constraints keys must be FieldConstraint constants + */ public function __construct( public readonly ?string $entityField, - public readonly string $type, - public readonly array $constraints - ) { + string $type, + array $constraints + ) + { + $this->type = $type; + $this->constraints = $constraints; } } diff --git a/src/Repository/Repository.php b/src/Repository/Repository.php index 56bb3e8..8d38cf4 100644 --- a/src/Repository/Repository.php +++ b/src/Repository/Repository.php @@ -2,18 +2,15 @@ namespace Veracrypt\CrashCollector\Repository; -use Veracrypt\CrashCollector\Repository\Storage\DatabaseTable; -use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; +use Veracrypt\CrashCollector\Storage\DatabaseTable; abstract class Repository { use DatabaseTable; - /** @var Field[] $fields NB: has to be set up before calling parent::__construct */ - protected array $fields = []; - /** - * @throws \DomainException, \PDOException + * @throws \DomainException + * @trows \PDOException */ public function __construct() { @@ -24,7 +21,7 @@ public function __construct() /** * @throws \DomainException */ - public function getField(string $entityFieldName): Field + /*public function getField(string $entityFieldName): Field { foreach($this->fields as $field) { if ($field->entityField === $entityFieldName) { @@ -32,74 +29,17 @@ public function getField(string $entityFieldName): Field } } throw new \DomainException("Repository has no field named '$entityFieldName'"); - } - - /** - * A few notes on the SQLite type system (full docs at https://www.sqlite.org/datatype3.html), for the unwary: - * - any column can hold any type! - * - type juggling is in effect, but type-conversion rules are not the same as in php. Notably, expressions have no type! - * - there is no 'bool' or 'date' column/data type. Columns defined as such get a 'numeric' preferential type (aka 'affinity') - * - the length limit on varchar columns is ignored - * @throws \DomainException, \PDOException - */ - protected function createTable(): void - { - $query = 'CREATE TABLE ' . $this->tableName . ' ('; - foreach ($this->fields as $colName => $f) { - $query .= $colName . ' ' . $f->type . ' '; - $constraints = $f->constraints; - if (isset($constraints[FC::Length])) { - $query .= '(' . $f->constraints[FC::Length] . ') '; - unset($constraints[FC::Length]); - } - foreach($constraints as $cn => $cv) { - switch($cn) { - case FC::PK: - if ($cv) { - $query .= 'primary key '; - } - break; - case FC::Autoincrement: - if ($cv) { - $query .= 'autoincrement '; - } - break; - case FC::NotNull: - if ($cv) { - $query .= 'not null '; - } - break; - case FC::Unique: - if ($cv) { - $query .= 'unique '; - } - break; - case FC::Default: - if ($cv !== null) { - $query .= 'default ' . $cv . ' '; - } - break; - default: - throw new \DomainException("Unsupported Field constraint '$cn'"); - } - } - $query = substr($query, 0, -1) . ', '; - } - $query = substr($query, 0, -2) . ')'; - - /// @todo convert PDO exceptions into repository exceptions? - self::$dbh->exec($query); - } + }*/ protected function buildFetchEntityQuery(): string { $query = 'select '; - foreach($this->fields as $col => $field) { + foreach($this->fields as $colName => $field) { if ($field->entityField == '') { continue; } - $query .= $col; - if ($field->entityField !== $col) { + $query .= $colName; + if ($field->entityField !== $colName) { $query .= ' as ' . $field->entityField; } $query .= ', '; @@ -114,18 +54,18 @@ protected function storeEntity($value): void { $query = 'insert into ' . $this->tableName . ' ('; $vq = ''; - foreach($this->fields as $col => $field) { + foreach($this->fields as $colName => $field) { if ($field->entityField == '') { continue; } - $query .= $col . ', '; - $vq .= ":$col" . ', '; + $query .= $colName . ', '; + $vq .= ":$colName" . ', '; } $query = substr($query, 0, -2) . ') values (' . substr($vq, 0, -2) . ')'; $stmt = self::$dbh->prepare($query); /// @todo test: can `bindvalue` or `execute` fail without throwing? - foreach($this->fields as $col => $field) { + foreach($this->fields as $colName => $field) { if ($field->entityField == '') { continue; } @@ -135,7 +75,7 @@ protected function storeEntity($value): void // we cast to int as otherwise SQLite will store php false as ''... $val = (int)$val; } - $stmt->bindValue(":$col", $val); + $stmt->bindValue(":$colName", $val); } $stmt->execute(); } diff --git a/src/Repository/Storage/DatabaseTable.php b/src/Repository/Storage/DatabaseTable.php deleted file mode 100644 index 7ff3214..0000000 --- a/src/Repository/Storage/DatabaseTable.php +++ /dev/null @@ -1,53 +0,0 @@ -tableExists) { - return false; - } - - if ($this->tableExists($this->tableName)) { - $this->tableExists = true; - return false; - } - - $this->createTable(); - - $this->tableExists = true; - return true; - } - - /** - * @return void - * @throws \DomainException in case of bad config (unsupported database) - * @throws \PDOException in case of failure creating the db table - */ - abstract protected function createTable(): void; -} diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index c99f813..e25961f 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -7,13 +7,16 @@ class UserRepository extends Repository { - protected $tableName = 'auth_user'; + protected string $tableName = 'auth_user'; + /** + * @throws \DomainException in case of unsupported database type + * @trows \PDOException + */ public function __construct() { // Col names, type and size are inspired from Django + Symfony. $this->fields = [ - /// @todo should we disallow '' as value for all non-null string fields? Sqlite f.e. supports `CHECK()` /// @todo make the SQL more portable - `autoincrement` does not exist in either MySQL or Postgresql - drop the id col altogether? 'id' => new Field(null, 'integer', [FC::NotNull => true, FC::PK => true, FC::Autoincrement => true]), 'username' => new Field('username', 'varchar', [FC::Length => 180, FC::NotNull => true, FC::Unique => true]), @@ -33,6 +36,7 @@ public function __construct() /** * Note: this does not validate the length of the fields, nor truncate or validate them + * @throws \PDOException */ public function createUser(string $username, string $passwordHash, string $email, string $firstName, string $lastName, bool $isSuperUser = false, bool $isActive = true): User @@ -43,23 +47,25 @@ public function createUser(string $username, string $passwordHash, string $email return $user; } - /* + /** + * @throws \PDOException + */ public function fetchUser(string $username): User|null { $query = $this->buildFetchEntityQuery() . ' where username = :username'; $stmt = self::$dbh->prepare($query); $stmt->bindValue(':username', $username); $stmt->execute(); - $result = $stmt->fetchObject(User::class); - return $result ? $result : null; + $result = $stmt->fetch(\PDO::FETCH_ASSOC); + return $result ? new User(...$result) : null; } - */ /** * NB: passing in an empty string for any value will trigger the data to be updated in the DB, unlike passing in a NULL. * This might be unexpected... - * @todo we could allow the username to be changed too, by adding a $newUsername argument + * @todo we could allow the username to be changed too, by adding a $newUsername argument (unless we make it the PK...) * @throws \BadMethodCallException + * @throws \PDOException */ public function updateUser(string $username, ?string $passwordHash = null, ?string $email = null, ?string $firstName = null, ?string $lastName = null, ?bool $isSuperUser = null, ?bool $isActive = null): bool @@ -101,6 +107,9 @@ public function updateUser(string $username, ?string $passwordHash = null, ?stri return (bool)$stmt->rowCount(); } + /** + * @throws \PDOException + */ public function deleteUser(string $username): bool { $query = 'delete from ' . $this->tableName . ' where username = :username'; @@ -110,6 +119,9 @@ public function deleteUser(string $username): bool return (bool)$stmt->rowCount(); } + /** + * @throws \PDOException + */ public function activateUser(string $username): bool { $query = 'update ' . $this->tableName . ' set is_active = true where username = :username'; @@ -119,6 +131,9 @@ public function activateUser(string $username): bool return (bool)$stmt->rowCount(); } + /** + * @throws \PDOException + */ public function deactivateUser(string $username): bool { $query = 'update ' . $this->tableName . ' set is_active = false where username = :username'; @@ -128,6 +143,9 @@ public function deactivateUser(string $username): bool return (bool)$stmt->rowCount(); } + /** + * @throws \PDOException + */ public function userLoggedIn(string $username): bool { /// @todo add a condition on existing last_login not being later than the new one? @@ -142,6 +160,7 @@ public function userLoggedIn(string $username): bool /** * @return mixed[][] we return arrays instead of value-objects, to make it easy for the console table helper. * This is also why the password hash is omitted. + * @throws \PDOException */ public function listUsers(): Array { diff --git a/src/Router.php b/src/Router.php index b3d4944..62a51b6 100644 --- a/src/Router.php +++ b/src/Router.php @@ -6,27 +6,38 @@ class Router { protected string $rootUrl; protected string $rootDir; + protected bool $stripPhpExtension = false; + protected bool $stripIndexDotPhp = false; public function __construct() { + /// @todo allow setting $stripPhpExtension and $stripIndexDotPhp from $_ENV + $this->rootUrl = $_ENV['ROOT_URL']; + // nb: realpath trims the trailing slash $this->rootDir = realpath(__DIR__ . '/../public/'); } /** * @todo add support for generating absolute URLs, etc... * see fe. https://github.com/symfony/routing/blob/7.1/Generator/UrlGeneratorInterface.php + * @param string $fileName the absolute file path. If an empty string is passed, the current execution directory is used * @throws \DomainException */ public function generate(string $fileName, array $queryStringParts = []): string { - $fileName = realpath($fileName); - if (!str_starts_with($fileName, $this->rootDir)) { + $realFileName = realpath($fileName); + if (!str_starts_with($realFileName, $this->rootDir . '/') && $realFileName !== $this->rootDir) { throw new \DomainException("Given file path is outside web root: '$fileName'"); } - $url = $this->rootUrl . substr($fileName, strlen($this->rootDir) + 1); + $url = $this->rootUrl . substr($realFileName, strlen($this->rootDir) + 1); - /// @todo strip trailing `/index.php` based on analysis of $_SERVER + if ($this->stripIndexDotPhp) { + $url = preg_replace('#/index\.php$#', '/', $url); + } + if ($this->stripPhpExtension) { + $url = preg_replace('#\.php$#', '', $url); + } if ($queryStringParts) { $parts = []; @@ -37,4 +48,33 @@ public function generate(string $fileName, array $queryStringParts = []): string } return $url; } + + /** + * @todo add support for absolute urls, etc... + */ + public function match(string $pathInfo): string|false + { + $parts = parse_url($pathInfo); + if (!$parts || !array_key_exists('path', $parts) || $parts['path'] === '' || + array_intersect_key(['scheme' => 0, 'host' => 0, 'port' => 0, 'user' => 0, 'pass' => 0], $parts)) { + return false; + } + + // nb: realpath normalizes excess slashes - no need to ltrim them. It also removes trailing ones + $fileName = realpath($this->rootDir . '/' . $parts['path']); + + if ($fileName === false && $this->stripPhpExtension && !preg_match('#(/|\.php)$#', $parts['path'])) { + $fileName = realpath($this->rootDir . '/' . $parts['path'] . '.php'); + } + + if ($fileName === false || (!str_starts_with($fileName, $this->rootDir . '/') && $fileName !== $this->rootDir)) { + return false; + } + + if (is_dir($fileName) && $this->stripIndexDotPhp && is_file($fileName . '/index.php')) { + $fileName = $fileName . '/index.php'; + } + + return $fileName; + } } diff --git a/src/Security/AnonymousUser.php b/src/Security/AnonymousUser.php new file mode 100644 index 0000000..5516940 --- /dev/null +++ b/src/Security/AnonymousUser.php @@ -0,0 +1,23 @@ +supportsRequest()) { + $this->authenticate(); + } + } + + protected function supportsRequest() + { + return true; + } + + protected function authenticate(): void + { + $session = Session::getInstance(); + $username = $session->get($this->storageKey); + if ($username !== null) { + // we always refresh the user details from the repo, to check if his/her roles or active status have changed + $userProvider = new UserProvider(); + try { + // NB: we do not catch \PDOException - in case f.e. the db connection is down, we let the error bubble all the way up + $user = $userProvider->loadUserByIdentifier($username); + if ($user->isActive()) { + $this->user = $user; + /// @todo here we could add $session->commit(); + return; + } + } catch (UserNotFoundException $e) { + } + + // the current user disappeared from the db or got invalidated - clean up the session stuff + $this->logoutUser(true); + } + } + + public function getUser(): UserInterface + { + return $this->user === null ? new AnonymousUser() : $this->user; + } + + /** + * NB: this does not check for user roles nor active status. Doing that is left to the caller! + */ + public function loginUser(UserInterface $user): void + { + if ($user instanceof AnonymousUser) { + throw new \DomainException('Anonymous user can not be used for logging in'); + } + + if ($user === $this->user) { + return; + } + + $this->user = $user; + + $session = Session::getInstance(); + $previousUserIdentifier = $session->get($this->storageKey); + if ($user->getUserIdentifier() !== $previousUserIdentifier) { + $session->regenerate(); + } + $session->set($this->storageKey, $user->getUserIdentifier()); + /// @todo here we could add $session->commit(); + } + + public function logoutUser(bool $force = false): void + { + if ($this->user === null && !$force) { + return; + } + + $this->user = null; + + $session = Session::getInstance(); + $session->destroySession(); + + /// @todo clean up csrf tokens and remember-me tokens if not stored in the Session + /// @todo send Clear-Site-Data header? (using an event system?) + } + + /** + * @param UserRole|UserRole[] $roles + * @throws AuthorizationException + */ + public function require(UserRole|array $roles): void + { + foreach(array($roles) as $role) { + if (!in_array($role, $this->getUser()->getRoles())) { + throw new AuthorizationException("Current user does not have required role " . $role->value); + } + } + } + + public function displayAdminLoginPage(string $successRedirectUrl): void + { + /// @todo should we give some info or warning if the user is logged in already? Eg. if this is used to + /// display the login form on a page which requires admin perms... + + // avoid browsers and proxies caching the login-form version of the current page - we send he same no-cache headers + // as sent by php when setting session_cache_limiter to nocache + if (!headers_sent()) { + header('Expires: Thu, 19 Nov 1981 08:52:00 GMT'); + header('Cache-Control: no-store, no-cache, must-revalidate'); + header('Pragma: no-cache'); + } + $router = new Router(); + $tpl = new Templating(); + echo $tpl->render('admin/login.html.twig', [ + 'form' => new LoginForm($successRedirectUrl), + 'form_url' => $router->generate(__DIR__ . '/../../public/admin/login.php'), 'root_url' => $router->generate(__DIR__ . '/../../public') + ]); + } +} diff --git a/src/Security/Session.php b/src/Security/Session.php new file mode 100644 index 0000000..0f2aa58 --- /dev/null +++ b/src/Security/Session.php @@ -0,0 +1,85 @@ +sessionOptions = [ + 'use_strict_mode' => 1, 'use_cookies' => 1, 'use_only_cookies' => 1, 'cookie_httponly' => 1, + 'cookie_samesite' => 'Strict'/*, 'cache_limiter' => '', 'cache_expire' => 0*/, 'use_trans_sid' => 0, 'lazy_write' => 1, + ]; + + $this->autoCommit($autoCommit); + } + + public function regenerate(): void + { + /// @todo check: if there is already a session, should we remove existing csrf tokens from the storage? + /// @todo Should we delete the previous session data? See example 2 at + /// https://www.php.net/manual/en/function.session-regenerate-id.php#refsect1-function.session-regenerate-id-examples + /// for the recommended way to generate new session ids while leaving the old session available for a + /// little while, to avoid issues with unstable networks and race conditions between requests + session_regenerate_id(); + } + + public function destroySession(): void + { + if (\PHP_SESSION_ACTIVE === session_status()) { + $_SESSION = []; + /// @todo check php source code: does it make sense to call session_write_close() here, or does session_destroy do that already? + session_destroy(); + } + + // Expire the session cookie. + // Note that, in theory at least, this is not required when session.strict_mode is enabled (which we _try_ to force), + // as subsequent requests with the same session id cookie will not trigger creation of a session. + // We prefer taking a belt-and-suspenders approach, and leave no dead cookies on the browser. + $sessionName = session_name(); + if (isset($_COOKIE[$sessionName])) { + $params = session_get_cookie_params(); + unset($params['lifetime']); + setcookie($sessionName, '', $params); + } + + $this->sessionStarted = false; + } + + /** + * Saves data and unlocks the session + * @throws \RuntimeException + */ + public function commit(): void + { + $this->doCommit(); + } +} diff --git a/src/Security/UserInterface.php b/src/Security/UserInterface.php new file mode 100644 index 0000000..ed09c54 --- /dev/null +++ b/src/Security/UserInterface.php @@ -0,0 +1,20 @@ +userRepository = new UserRepository(); + } + + /** + * @throws UserNotFoundException + * @throws \PDOException + */ + public function loadUserByIdentifier(string $identifier): User + { + $user = $this->userRepository->fetchUser($identifier); + if ($user === null) { + throw new UserNotFoundException("No such user: $identifier"); + } + return $user; + } +} diff --git a/src/Security/UserProviderInterface.php b/src/Security/UserProviderInterface.php new file mode 100644 index 0000000..c15cdfe --- /dev/null +++ b/src/Security/UserProviderInterface.php @@ -0,0 +1,8 @@ +userProvider = new UserProvider(); + $this->passwordHasher = new PasswordHasher(); + $this->logger = new Logger($_ENV['AUDIT_LOG_FILE'], $_ENV['AUDIT_LOG_LEVEL']); + } + + /** + * @throws AuthenticationException + */ + public function authenticate(string $username, #[\SensitiveParameter] string $password): UserInterface + { + try { + $user = $this->userProvider->loadUserByIdentifier($username); + } catch (UserNotFoundException $e) { + $this->logger->info("User '$username' failed logging in: not found"); + throw $e; + } + if (!$this->passwordHasher->verify($user->passwordHash, $password)) { + $this->logger->info("User '$username' failed logging in: bad password"); + throw new BadCredentialsException('Invalid username/password'); + } + if (!$user->isActive()) { + $this->logger->info("User '$username' failed logging in: it is not active"); + throw new AccountExpiredException('User account is not active'); + } + + Firewall::getInstance()->loginUser($user); + + $this->logger->debug("User '$username' logged in"); + + return $user; + } +} diff --git a/src/Singleton.php b/src/Singleton.php new file mode 100644 index 0000000..52f9146 --- /dev/null +++ b/src/Singleton.php @@ -0,0 +1,17 @@ +tableExists) { + return false; + } + + if ($this->tableExists($this->tableName)) { + $this->tableExists = true; + return false; + } + + $this->createTable(); + + $this->tableExists = true; + return true; + } + + /** + * A few notes on the SQLite type system (full docs at https://www.sqlite.org/datatype3.html), for the unwary: + * - any column can hold any type! + * - type juggling is in effect, but type-conversion rules are not the same as in php. Notably, expressions have no type! + * - there is no 'bool' or 'date' column/data type. Columns defined as such get a 'numeric' preferential type (aka 'affinity') + * - the length limit on varchar columns is ignored + * @throws \DomainException in case of unsupported field constraint + * @throws \PDOException + * @todo should we disallow '' as value for all non-null string fields (or via a custom constraint)? Sqlite f.e. + * supports `CHECK()`, or we could use the cross-database attribute PDO::NULL_EMPTY_STRING + */ + protected function createTable(): void + { + $query = 'CREATE TABLE ' . $this->tableName . ' ('; + foreach ($this->fields as $colName => $f) { + $query .= $colName . ' ' . $f->type . ' '; + $constraints = $f->constraints; + if (isset($constraints[FC::Length])) { + $query .= '(' . $f->constraints[FC::Length] . ') '; + unset($constraints[FC::Length]); + } + foreach($constraints as $cn => $cv) { + switch($cn) { + case FC::PK: + if ($cv) { + $query .= 'primary key '; + } + break; + case FC::Autoincrement: + if ($cv) { + $query .= 'autoincrement '; + } + break; + case FC::NotNull: + if ($cv) { + $query .= 'not null '; + } + break; + case FC::Unique: + if ($cv) { + $query .= 'unique '; + } + break; + case FC::Default: + if ($cv !== null) { + $query .= 'default ' . $cv . ' '; + } + break; + default: + throw new \DomainException("Unsupported Field constraint '$cn'"); + } + } + $query = substr($query, 0, -1) . ', '; + } + $query = substr($query, 0, -2) . ')'; + + self::$dbh->exec($query); + } +} diff --git a/src/Storage/Session.php b/src/Storage/Session.php new file mode 100644 index 0000000..750b63c --- /dev/null +++ b/src/Storage/Session.php @@ -0,0 +1,96 @@ +sessionStarted) { + $this->startSession(); + } + + $_SESSION[$key] = $value; + + if ($this->doAutoCommit) { + $this->doCommit(); + } + } + + /** + * @todo cache the whole of $_SESSION in memory so that we can avoid further calls to session_start on later calls + * (and either add a $forceRefresh argument, or a separate `refresh` method) + * @throws \RuntimeException + */ + public function get(string|int $key, mixed $default = null): mixed + { + if (!$this->sessionStarted) { + $this->startSession(); + } + + if (array_key_exists($key, $_SESSION)) { + $default = $_SESSION[$key]; + } + + if ($this->doAutoCommit) { + $this->doCommit(); + } + + return $default; + } + + /** + * @throws \RuntimeException + */ + protected function startSession(): void + { + if (\PHP_SESSION_NONE === session_status()) { + + /// @todo should we throw if headers_sent() returns true? + + /// @todo look if there is a cookie matching `session_name()`. If there is, validate that the session_id + /// matches a regexp like `/^[a-zA-Z0-9,-]{22,250}$/` (but built upon live values of session.sid_bits_per_character + /// and session.sid_length) and if it does not, call `session_id(session_create_id())` + /// see Sf NativeSessionStorage::start for an explanation + + /// @todo once we have implemented `regenerate` so that it keeps around the old session and adds specific + /// data to it, check here for its presence + + if (!session_start($this->sessionOptions)) { + throw new \RuntimeException('Failed to start the session'); + } + } + + $this->sessionStarted = true; + } + + /** + * Sets/gets the autocommit mode + */ + protected function autoCommit(?bool $autoCommit): bool + { + if ($autoCommit !== null) { + $this->doAutoCommit = $autoCommit; + } + return $this->doAutoCommit; + } + + /** + * Saves data and unlocks the session + * @throws \RuntimeException + */ + protected function doCommit(): void + { + if (!session_write_close()) { + throw new \RuntimeException('Failed to save the session'); + } + $this->sessionStarted = false; + } +} diff --git a/src/Templating.php b/src/Templating.php index dceb2a9..69a9e25 100644 --- a/src/Templating.php +++ b/src/Templating.php @@ -7,7 +7,7 @@ class Templating { - protected $twig; + protected TwigEnvironment $twig; public function __construct() { From 012fe06769e7243bf9cea76ad753460115fb22b1 Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 15 Oct 2024 09:32:15 +0000 Subject: [PATCH 02/38] Fix: factory method for singleton should not allow different args on every call --- src/Security/Session.php | 4 +--- src/Singleton.php | 4 ++-- src/Storage/Session.php | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Security/Session.php b/src/Security/Session.php index 0f2aa58..7ed2551 100644 --- a/src/Security/Session.php +++ b/src/Security/Session.php @@ -20,7 +20,7 @@ class Session * one session could be accessed at a time. Even different sessions would wait for another to finish. So saving * sessions in SQLite is not recommended. */ - protected function __construct(bool $autoCommit = false) + protected function __construct() { if (!\extension_loaded('session')) { throw new \LogicException('PHP extension "session" is required'); @@ -38,8 +38,6 @@ protected function __construct(bool $autoCommit = false) 'use_strict_mode' => 1, 'use_cookies' => 1, 'use_only_cookies' => 1, 'cookie_httponly' => 1, 'cookie_samesite' => 'Strict'/*, 'cache_limiter' => '', 'cache_expire' => 0*/, 'use_trans_sid' => 0, 'lazy_write' => 1, ]; - - $this->autoCommit($autoCommit); } public function regenerate(): void diff --git a/src/Singleton.php b/src/Singleton.php index 52f9146..6a35231 100644 --- a/src/Singleton.php +++ b/src/Singleton.php @@ -6,10 +6,10 @@ trait Singleton { protected static $_instance; - public static function getInstance(...$args) + public static function getInstance() { if (self::$_instance === null) { - self::$_instance = new self(...$args); + self::$_instance = new self(); } return self::$_instance; diff --git a/src/Storage/Session.php b/src/Storage/Session.php index 750b63c..42931a7 100644 --- a/src/Storage/Session.php +++ b/src/Storage/Session.php @@ -60,7 +60,7 @@ protected function startSession(): void /// and session.sid_length) and if it does not, call `session_id(session_create_id())` /// see Sf NativeSessionStorage::start for an explanation - /// @todo once we have implemented `regenerate` so that it keeps around the old session and adds specific + /// @todo once we have improved `regenerate` so that it keeps around the old session and adds specific /// data to it, check here for its presence if (!session_start($this->sessionOptions)) { @@ -74,7 +74,7 @@ protected function startSession(): void /** * Sets/gets the autocommit mode */ - protected function autoCommit(?bool $autoCommit): bool + public function autoCommit(?bool $autoCommit): bool { if ($autoCommit !== null) { $this->doAutoCommit = $autoCommit; From d4962ee288564b422c5dcd51e1b377376c9939a8 Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 15 Oct 2024 10:17:46 +0000 Subject: [PATCH 03/38] Keep trace of more events in the audit log; update user profile with last login timestamp --- .env | 1 + bin/console | 1 + public/admin/login.php | 3 ++ src/Logger.php | 22 ++++++++++++ src/Repository/UserRepository.php | 35 +++++++++++++++---- src/Security/Firewall.php | 18 +++++++++- .../UsernamePasswordAuthenticator.php | 3 +- 7 files changed, 74 insertions(+), 9 deletions(-) diff --git a/.env b/.env index cd4ae1e..0fba4ad 100644 --- a/.env +++ b/.env @@ -10,6 +10,7 @@ APP_DEBUG=false ROOT_URL=/ LOG_DIR=/var/www/VeraCrypt-CrashCollector/var/logs +# The audit log traces user events such as login, password changes, etc AUDIT_LOG_FILE=audit.log # see Psr\Log\LogLevel for valid values AUDIT_LOG_LEVEL=info diff --git a/bin/console b/bin/console index ad30bde..a5904bb 100755 --- a/bin/console +++ b/bin/console @@ -14,6 +14,7 @@ use Symfony\Component\Console\Output\ConsoleOutputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Question\Question; use Symfony\Component\Console\Style\SymfonyStyle; +use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Repository\UserRepository; use Veracrypt\CrashCollector\Security\PasswordHasher; diff --git a/public/admin/login.php b/public/admin/login.php index 0629b08..0ffacbe 100644 --- a/public/admin/login.php +++ b/public/admin/login.php @@ -4,6 +4,7 @@ use Veracrypt\CrashCollector\Exception\AuthenticationException; use Veracrypt\CrashCollector\Form\LoginForm; +use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Router; use Veracrypt\CrashCollector\Security\UsernamePasswordAuthenticator; use Veracrypt\CrashCollector\Templating; @@ -19,6 +20,8 @@ $redirectUrl = $data['redirect']; if (!$router->match($redirectUrl)) { $form->setError('Tsk tsk tsk'); + $logger = Logger::getInstance('audit'); + $logger->warning("Hacking attempt: login fom submitted with invalid redirect url '$redirectUrl'"); } else { unset($data['redirect']); try { diff --git a/src/Logger.php b/src/Logger.php index 172261c..67e5b20 100644 --- a/src/Logger.php +++ b/src/Logger.php @@ -9,6 +9,8 @@ class Logger extends AbstractLogger { protected $logFile; protected $logLevel; + /** @var Logger[] $_loggers */ + protected static array $_loggers = []; // same weights as monolog protected $levelWeights = [ @@ -22,6 +24,26 @@ class Logger extends AbstractLogger LogLevel::EMERGENCY => 600, ]; + /** + * Loggers do not really need to be singletons, but doing things this way helps with keeping the configuration + * for each logger tucked up neatly in a single place + * @throws \DomainException + */ + public static function getInstance(string $name): Logger + { + if (!array_key_exists($name, self::$_loggers)) { + switch ($name) { + case 'audit': + self::$_loggers[$name] = new self($_ENV['AUDIT_LOG_FILE'], $_ENV['AUDIT_LOG_LEVEL']); + break; + default: + throw new \DomainException("Logger '$name' in not configured"); + } + } + + return self::$_loggers[$name]; + } + public function __construct(string|\Stringable $logFile, $logLevel) { if (!str_contains('/', $logFile)) { diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index e25961f..c84a36c 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -3,11 +3,13 @@ namespace Veracrypt\CrashCollector\Repository; use Veracrypt\CrashCollector\Entity\User; +use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; class UserRepository extends Repository { protected string $tableName = 'auth_user'; + protected Logger $logger; /** * @throws \DomainException in case of unsupported database type @@ -30,6 +32,7 @@ public function __construct() //'is_staff' => new Field('isStaff', 'bool', [FC::NotNull => true, FC::Default => 'false']), 'is_superuser' => new Field('isSuperuser', 'bool', [FC::NotNull => true, FC::Default => 'false']), ]; + $this->logger = Logger::getInstance('audit'); parent::__construct(); } @@ -44,6 +47,7 @@ public function createUser(string $username, string $passwordHash, string $email $dateJoined = time(); $user = new User($username, $passwordHash, $email, $firstName, $lastName, $dateJoined, null, $isActive, $isSuperUser); $this->storeEntity($user); + $this->logger->debug("User '$username' was created"); return $user; } @@ -104,7 +108,11 @@ public function updateUser(string $username, ?string $passwordHash = null, ?stri } $stmt->bindValue(":username", $username); $stmt->execute(); - return (bool)$stmt->rowCount(); + $updated = (bool)$stmt->rowCount(); + if ($updated) { + $this->logger->debug("User '$username' was updated"); + } + return $updated; } /** @@ -116,34 +124,49 @@ public function deleteUser(string $username): bool $stmt = self::$dbh->prepare($query); $stmt->bindValue(':username', $username); $stmt->execute(); - return (bool)$stmt->rowCount(); + $deleted = (bool)$stmt->rowCount(); + if ($deleted) { + $this->logger->debug("User '$username' was deleted"); + } + return $deleted; } /** + * NB: this returns false if the user exists and if it was already activated * @throws \PDOException */ public function activateUser(string $username): bool { - $query = 'update ' . $this->tableName . ' set is_active = true where username = :username'; + $query = 'update ' . $this->tableName . ' set is_active = true where username = :username and is_active = false'; $stmt = self::$dbh->prepare($query); $stmt->bindValue(':username', $username); $stmt->execute(); - return (bool)$stmt->rowCount(); + $activated = (bool)$stmt->rowCount(); + if ($activated) { + $this->logger->debug("User '$username' was activated"); + } + return $activated; } /** + * NB: this returns false if the user exists and it was already deactivated * @throws \PDOException */ public function deactivateUser(string $username): bool { - $query = 'update ' . $this->tableName . ' set is_active = false where username = :username'; + $query = 'update ' . $this->tableName . ' set is_active = false where username = :username and is_active = true'; $stmt = self::$dbh->prepare($query); $stmt->bindValue(':username', $username); $stmt->execute(); - return (bool)$stmt->rowCount(); + $deactivated = (bool)$stmt->rowCount(); + if ($deactivated) { + $this->logger->debug("User '$username' was deactivated"); + } + return $deactivated; } /** + * Call this when the user logged in * @throws \PDOException */ public function userLoggedIn(string $username): bool diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index bc865eb..24da8e5 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -2,10 +2,13 @@ namespace Veracrypt\CrashCollector\Security; +use Veracrypt\CrashCollector\Entity\User; use Veracrypt\CrashCollector\Entity\UserRole; use Veracrypt\CrashCollector\Exception\AuthorizationException; use Veracrypt\CrashCollector\Exception\UserNotFoundException; use Veracrypt\CrashCollector\Form\LoginForm; +use Veracrypt\CrashCollector\Logger; +use Veracrypt\CrashCollector\Repository\UserRepository; use Veracrypt\CrashCollector\Router; use Veracrypt\CrashCollector\Singleton; use Veracrypt\CrashCollector\Templating; @@ -47,8 +50,11 @@ protected function authenticate(): void } catch (UserNotFoundException $e) { } - // the current user disappeared from the db or got invalidated - clean up the session stuff + // the current user either disappeared from the db or got invalidated - clean up the session stuff $this->logoutUser(true); + + $logger = Logger::getInstance('audit'); + $logger->debug("A session for user '$username' has been forcibly terminated as his/her profile was updated"); } } @@ -79,6 +85,16 @@ public function loginUser(UserInterface $user): void } $session->set($this->storageKey, $user->getUserIdentifier()); /// @todo here we could add $session->commit(); + + if ($user instanceof User) { + $repo = new UserRepository(); + try { + $repo->userLoggedIn($user->username); + } catch (\PDOException) { + $logger = Logger::getInstance('audit'); + $logger->warning("Failed updating last login time for user '{$user->username}'"); + } + } } public function logoutUser(bool $force = false): void diff --git a/src/Security/UsernamePasswordAuthenticator.php b/src/Security/UsernamePasswordAuthenticator.php index 37642f6..a8e5dba 100644 --- a/src/Security/UsernamePasswordAuthenticator.php +++ b/src/Security/UsernamePasswordAuthenticator.php @@ -2,7 +2,6 @@ namespace Veracrypt\CrashCollector\Security; -use Veracrypt\CrashCollector\Entity\User; use Veracrypt\CrashCollector\Exception\AccountExpiredException; use Veracrypt\CrashCollector\Exception\AuthenticationException; use Veracrypt\CrashCollector\Exception\BadCredentialsException; @@ -22,7 +21,7 @@ public function __construct() { $this->userProvider = new UserProvider(); $this->passwordHasher = new PasswordHasher(); - $this->logger = new Logger($_ENV['AUDIT_LOG_FILE'], $_ENV['AUDIT_LOG_LEVEL']); + $this->logger = Logger::getInstance('audit'); } /** From 513f8572931eff86008a91d8739669a38e44a545 Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 15 Oct 2024 11:31:03 +0000 Subject: [PATCH 04/38] avoid php warning if trying to start a session after headers have been sent; improve comments --- src/Security/Firewall.php | 8 ++++---- src/Security/Session.php | 16 ++++++++-------- src/Storage/Session.php | 30 +++++++++++++++++++----------- 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index 24da8e5..3fce42c 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -44,7 +44,7 @@ protected function authenticate(): void $user = $userProvider->loadUserByIdentifier($username); if ($user->isActive()) { $this->user = $user; - /// @todo here we could add $session->commit(); + /// @todo here we could add $session->commit() - or use autocommitting sessions return; } } catch (UserNotFoundException $e) { @@ -84,7 +84,7 @@ public function loginUser(UserInterface $user): void $session->regenerate(); } $session->set($this->storageKey, $user->getUserIdentifier()); - /// @todo here we could add $session->commit(); + /// @todo here we could add $session->commit() - or use autocommitting sessions if ($user instanceof User) { $repo = new UserRepository(); @@ -127,8 +127,8 @@ public function require(UserRole|array $roles): void public function displayAdminLoginPage(string $successRedirectUrl): void { - /// @todo should we give some info or warning if the user is logged in already? Eg. if this is used to - /// display the login form on a page which requires admin perms... + /// @todo should we give some info or warning if the user is logged in already? Eg. if this is used to display + /// the login form on a page which requires admin perms... // avoid browsers and proxies caching the login-form version of the current page - we send he same no-cache headers // as sent by php when setting session_cache_limiter to nocache diff --git a/src/Security/Session.php b/src/Security/Session.php index 7ed2551..02a071c 100644 --- a/src/Security/Session.php +++ b/src/Security/Session.php @@ -15,10 +15,10 @@ class Session /** * NB: we could allow passing in an optional instance of \SessionHandlerInterface, and provide a default one which - * stores session data in the same database used for the CR data (or a separate sqlite file). + * stores session data in the same database used for the CR data (or, preferably, a separate sqlite file). * However, SQLite does not support row level locks but locks the whole database, with the results that only * one session could be accessed at a time. Even different sessions would wait for another to finish. So saving - * sessions in SQLite is not recommended. + * sessions in SQLite is not recommended, unless we also use session autocommit. */ protected function __construct() { @@ -31,7 +31,7 @@ protected function __construct() // for both reading and writing for the whole duration of the php script. In order to improve concurrency, // one option could be to use the `read_and_close` option for the `session_start` call. // However, it seems that doing so has negative effects when the session GC is triggered via an external - // cronjob instead of via session.gc_probability, as used by default on Debian/Ubuntu. + // cronjob instead of via session.gc_probability, as configured by default on Debian/Ubuntu. // See: https://stackoverflow.com/questions/37789172/php-session-randomly-dies-when-read-and-close-is-active /// @todo should we allow values for session-related options be set via .env? $this->sessionOptions = [ @@ -43,10 +43,10 @@ protected function __construct() public function regenerate(): void { /// @todo check: if there is already a session, should we remove existing csrf tokens from the storage? - /// @todo Should we delete the previous session data? See example 2 at + /// @todo Should we delete the previous session data, passing in $true, or keep the old session around? See example 2 at /// https://www.php.net/manual/en/function.session-regenerate-id.php#refsect1-function.session-regenerate-id-examples - /// for the recommended way to generate new session ids while leaving the old session available for a - /// little while, to avoid issues with unstable networks and race conditions between requests + /// for the recommended way to generate new session ids while avoiding issues with unstable networks and + /// race conditions between requests session_regenerate_id(); } @@ -58,6 +58,8 @@ public function destroySession(): void session_destroy(); } + $this->sessionStarted = false; + // Expire the session cookie. // Note that, in theory at least, this is not required when session.strict_mode is enabled (which we _try_ to force), // as subsequent requests with the same session id cookie will not trigger creation of a session. @@ -68,8 +70,6 @@ public function destroySession(): void unset($params['lifetime']); setcookie($sessionName, '', $params); } - - $this->sessionStarted = false; } /** diff --git a/src/Storage/Session.php b/src/Storage/Session.php index 42931a7..5ca05c7 100644 --- a/src/Storage/Session.php +++ b/src/Storage/Session.php @@ -5,11 +5,12 @@ trait Session { protected bool $sessionStarted = false; - private bool $doAutoCommit = false; protected array $sessionOptions = []; + private bool $doAutoCommit = false; /** - * @throws \RuntimeException + * @throws \RuntimeException if the session was not yet started and either http headers have already been sent, or + * (presumably) the session storage fails */ public function set(string|int $key, mixed $value): void { @@ -25,9 +26,11 @@ public function set(string|int $key, mixed $value): void } /** - * @todo cache the whole of $_SESSION in memory so that we can avoid further calls to session_start on later calls - * (and either add a $forceRefresh argument, or a separate `refresh` method) - * @throws \RuntimeException + * @todo we could cache the whole of $_SESSION in memory so that we can avoid further calls to session_start on later + * calls to get (and either add a $forceRefresh argument, or a separate `refresh` method). This is esp. + * useful when in autocommit mode + * @throws \RuntimeException if the session was not yet started and either http headers have already been sent, or + * (presumably) the session storage fails */ public function get(string|int $key, mixed $default = null): mixed { @@ -47,18 +50,23 @@ public function get(string|int $key, mixed $default = null): mixed } /** - * @throws \RuntimeException + * @throws \RuntimeException if the session was not yet started and either http headers have already been sent, or + * (presumably) the session storage fails */ protected function startSession(): void { if (\PHP_SESSION_NONE === session_status()) { - /// @todo should we throw if headers_sent() returns true? + // we throw to avoid the php warning 'Session cannot be started after headers have already been sent' + // which would be generated later by calling `session start` + if (headers_sent()) { + throw new \RuntimeException('Session cannot be started after headers have already been sent'); + } - /// @todo look if there is a cookie matching `session_name()`. If there is, validate that the session_id - /// matches a regexp like `/^[a-zA-Z0-9,-]{22,250}$/` (but built upon live values of session.sid_bits_per_character - /// and session.sid_length) and if it does not, call `session_id(session_create_id())` - /// see Sf NativeSessionStorage::start for an explanation + // NB: in case we did not enforce use_strict_mode, we could look if there is a cookie matching `session_name()`. + // If there is, validate that the session_id matches a regexp like `/^[a-zA-Z0-9,-]{22,250}$/` (but built upon + // live values of session.sid_bits_per_character and session.sid_length) and if it does not, call + // `session_id(session_create_id())` and log the event. See Sf NativeSessionStorage::start for an explanation /// @todo once we have improved `regenerate` so that it keeps around the old session and adds specific /// data to it, check here for its presence From 1b3d749113f843edf56f39824d6d48f38092ea82 Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 15 Oct 2024 12:54:33 +0000 Subject: [PATCH 05/38] add self-service password reset --- public/admin/index.php | 7 +-- public/admin/login.php | 5 +- public/admin/resetpassword.php | 40 ++++++++++++++++ public/upload/index.php | 3 +- resources/templates/admin/index.html.twig | 10 ++-- resources/templates/admin/login.html.twig | 9 +--- .../templates/admin/resetpassword.html.twig | 24 ++++++++++ resources/templates/base.html.twig | 10 +++- resources/templates/upload/index.html.twig | 9 +--- src/Form/Field.php | 5 ++ src/Form/ResetPasswordForm.php | 47 +++++++++++++++++++ src/Security/AnonymousUser.php | 2 +- src/Security/Firewall.php | 17 ++++++- 13 files changed, 159 insertions(+), 29 deletions(-) create mode 100644 public/admin/resetpassword.php create mode 100644 resources/templates/admin/resetpassword.html.twig create mode 100644 src/Form/ResetPasswordForm.php diff --git a/public/admin/index.php b/public/admin/index.php index 7cea1e0..7487639 100644 --- a/public/admin/index.php +++ b/public/admin/index.php @@ -51,8 +51,9 @@ } echo $tpl->render('admin/index.html.twig', [ - 'form' => $form, 'reports' => $reports, 'num_reports' => $numReports, 'current_page' => $pageNum, + 'user' => $user, 'form' => $form, 'reports' => $reports, 'num_reports' => $numReports, 'current_page' => $pageNum, 'page_size' => $pageSize, 'num_pages' => ceil($numReports / $pageSize), 'page_sizes' => $pageSizes, - 'form_url' => $router->generate(__FILE__, $form->getQueryStringParts(true)), 'user' => $user, - 'logout_url' => $router->generate(__DIR__ . '/logout.php'), 'root_url' => $router->generate(__DIR__ . '/..'), + 'urls' => array_merge($firewall->getAdminUrls(), [ + 'form' => $router->generate(__FILE__, $form->getQueryStringParts(true)) + ]), ]); diff --git a/public/admin/login.php b/public/admin/login.php index 0ffacbe..e092bfe 100644 --- a/public/admin/login.php +++ b/public/admin/login.php @@ -3,6 +3,7 @@ require_once(__DIR__ . '/../../autoload.php'); use Veracrypt\CrashCollector\Exception\AuthenticationException; +use Veracrypt\CrashCollector\Security\Firewall; use Veracrypt\CrashCollector\Form\LoginForm; use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Router; @@ -38,7 +39,9 @@ /// @todo should we give some info or warning if the user is logged in already? } +$firewall = Firewall::getInstance(); $tpl = new Templating(); echo $tpl->render('admin/login.html.twig', [ - 'form' => $form, 'form_url' => $router->generate(__FILE__), 'root_url' => $router->generate(__DIR__ . '/..') + 'form' => $form, + 'urls' => array_merge($firewall->getAdminUrls(), ['form' => $router->generate(__FILE__)]), ]); diff --git a/public/admin/resetpassword.php b/public/admin/resetpassword.php new file mode 100644 index 0000000..2db0ddf --- /dev/null +++ b/public/admin/resetpassword.php @@ -0,0 +1,40 @@ +require(UserRole::User); + $user = $firewall->getUser(); +} catch (AuthorizationException $e) { + $firewall->displayAdminLoginPage($router->generate(__FILE__)); + exit(); +} + +$form = new ResetPasswordForm($user); +if ($form->isSubmitted()) { + $form->handleRequest(); + if ($form->isValid()) { + $passwordHasher = new PasswordHasher(); + $repository = new UserRepository(); + $repository->updateUser($user->getUserIdentifier(), $passwordHasher->hash($form->getField('newPassword')->getData())); + } +} + +echo $tpl->render('admin/resetpassword.html.twig', [ + 'user' => $user, + 'form' => $form, + 'urls' => array_merge($firewall->getAdminUrls(), ['form' => $router->generate(__FILE__)]), +]); diff --git a/public/upload/index.php b/public/upload/index.php index 4558991..9f2f1f8 100644 --- a/public/upload/index.php +++ b/public/upload/index.php @@ -25,5 +25,6 @@ $router = new Router(); $tpl = new Templating(); echo $tpl->render('upload/index.html.twig', [ - 'form' => $form, 'form_url' => $router->generate(__FILE__), 'root_url' => $router->generate(__DIR__ . '/..') + 'form' => $form, + 'urls' => ['form' => $router->generate(__FILE__), 'root' => $router->generate(__DIR__ . '/..')], ]); diff --git a/resources/templates/admin/index.html.twig b/resources/templates/admin/index.html.twig index 42b4508..e324619 100644 --- a/resources/templates/admin/index.html.twig +++ b/resources/templates/admin/index.html.twig @@ -1,12 +1,8 @@ {% extends "base.html.twig" %} -{% block body %} +{% block content %} {% import "parts/forms/macros.html.twig" as forms %}
- {{ include('parts/forms/cr_common_fields.html.twig') }}
@@ -33,7 +29,7 @@
{% if form.isSubmitted() and form.isValid() %}

Found {{ num_reports }} crash reports{% if num_reports > page_size %}, showing {{ page_size }} per page{% endif %}

-{{ forms.paginator(current_page, num_pages, form_url) }} +{{ forms.paginator(current_page, num_pages, urls.form) }} @@ -64,7 +60,7 @@ {% endfor %}
-{{ forms.paginator(current_page, num_pages, form_url) }} +{{ forms.paginator(current_page, num_pages, urls.form) }} {% else %} Note: accepted wildcard characters are the ones for SQL LIKE statements: '_' for any one char, and '?' for zero or more chars diff --git a/resources/templates/admin/login.html.twig b/resources/templates/admin/login.html.twig index bfcf207..2d77ea4 100644 --- a/resources/templates/admin/login.html.twig +++ b/resources/templates/admin/login.html.twig @@ -1,13 +1,9 @@ {% extends "base.html.twig" %} -{% block body %} +{% block content %} {% import "parts/forms/macros.html.twig" as forms %} -
- {# @todo make the form a bit less wide #} - +
{{ forms.input(form.getField('username'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }}
@@ -22,5 +18,4 @@ {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }}
-
{% endblock %} diff --git a/resources/templates/admin/resetpassword.html.twig b/resources/templates/admin/resetpassword.html.twig new file mode 100644 index 0000000..c06bebc --- /dev/null +++ b/resources/templates/admin/resetpassword.html.twig @@ -0,0 +1,24 @@ +{% extends "base.html.twig" %} + +{% block content %} +{% import "parts/forms/macros.html.twig" as forms %} +{% if form.isSubmitted() and form.isValid() %} +

The password has been updated.

+{% else %} + {# @todo make the form a bit less wide #} +
+
+ {{ forms.input(form.getField('oldPassword'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} +
+
+ {{ forms.input(form.getField('newPassword'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} +
+
+ {{ forms.input(form.getField('newPasswordConfirm'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} +
+
+ {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }} +
+
+{% endif %} +{% endblock %} diff --git a/resources/templates/base.html.twig b/resources/templates/base.html.twig index a466131..a8e53f1 100644 --- a/resources/templates/base.html.twig +++ b/resources/templates/base.html.twig @@ -13,7 +13,15 @@ {# no js needed yet #} -{% block body %} +
+{% block navbar %} + {% endblock %} +{% block content %} +{% endblock %} +
diff --git a/resources/templates/upload/index.html.twig b/resources/templates/upload/index.html.twig index c68262a..dd4c924 100644 --- a/resources/templates/upload/index.html.twig +++ b/resources/templates/upload/index.html.twig @@ -1,15 +1,11 @@ {% extends "base.html.twig" %} -{% block body %} +{% block content %} {% import "parts/forms/macros.html.twig" as forms %} -
- {% if form.isSubmitted() and form.isValid() %}

Thank you for taking the time to submit the information and sorry for your hassles.

{% else %} -
+ {{ include('parts/forms/cr_common_fields.html.twig') }}
{{ forms.input(form.getField('callStack'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }} @@ -19,5 +15,4 @@
{% endif %} -
{% endblock %} diff --git a/src/Form/Field.php b/src/Form/Field.php index 8765681..a4595af 100644 --- a/src/Form/Field.php +++ b/src/Form/Field.php @@ -97,6 +97,11 @@ public function setValue(mixed $value): bool return true; } + public function setError(string $erroMessage) + { + $this->errorMessage = $erroMessage; + } + public function getData(): null|string|\DateTimeImmutable { if ($this->value === null) { diff --git a/src/Form/ResetPasswordForm.php b/src/Form/ResetPasswordForm.php new file mode 100644 index 0000000..563b884 --- /dev/null +++ b/src/Form/ResetPasswordForm.php @@ -0,0 +1,47 @@ +fields = [ + 'oldPassword' => new Field('Current Password', 'cp', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'newPassword' => new Field('New Password', 'np', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'newPasswordConfirm' => new Field('Confirm new Password', 'npc', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + ]; + $this->currentUser = $currentUser; + } + + public function handleRequest(?array $request = null): void + { + parent::handleRequest($request); + + if ($this->isValid) { + if ($this->fields['newPassword']->getData() !== $this->fields['newPasswordConfirm']->getData()) { + $this->fields['newPasswordConfirm']->setError('The password does not match'); + $this->isValid = false; + } else { + $authenticator = new UsernamePasswordAuthenticator(); + try { + $authenticator->authenticate($this->currentUser->getUserIdentifier(), $this->fields['oldPassword']->getData()); + } catch (BadCredentialsException) { + $this->fields['oldPassword']->setError('The current password is wrong'); + $this->isValid = false; + } + /// @todo what to do in case we get an AccountExpiredException or UserNotFoundException? + /// This can happen, hopefully infrequently, when the form is displayed to a user still active, and + /// then submitted after the user got deactivated/deleted + } + } + } +} diff --git a/src/Security/AnonymousUser.php b/src/Security/AnonymousUser.php index 5516940..6181847 100644 --- a/src/Security/AnonymousUser.php +++ b/src/Security/AnonymousUser.php @@ -18,6 +18,6 @@ public function getUserIdentifier(): string public function isActive(): bool { - return true; + return false; } } diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index 3fce42c..c823009 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -141,7 +141,22 @@ public function displayAdminLoginPage(string $successRedirectUrl): void $tpl = new Templating(); echo $tpl->render('admin/login.html.twig', [ 'form' => new LoginForm($successRedirectUrl), - 'form_url' => $router->generate(__DIR__ . '/../../public/admin/login.php'), 'root_url' => $router->generate(__DIR__ . '/../../public') + 'urls' => array_merge($this->getAdminUrls(), ['form' => $router->generate(__DIR__ . '/../../public/admin/login.php')]), ]); } + + /** + * @return string[] + */ + public function getAdminUrls(): array + { + $router = new Router(); + return [ + 'root' => $router->generate(__DIR__ . '/../../public'), + 'home' => $router->generate(__DIR__ . '/../../public/admin/index.php'), + 'login' => $router->generate(__DIR__ . '/../../public/admin/login.php'), + 'logout' => $router->generate(__DIR__ . '/../../public/admin/logout.php'), + 'resetpassword' => $router->generate(__DIR__ . '/../../public/admin/resetpassword.php'), + ]; + } } From 0309d8a946a75472b791230f11d8c1958a8a7bd6 Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 15 Oct 2024 12:57:53 +0000 Subject: [PATCH 06/38] whitespace fixes --- src/Exception/AccountExpiredException.php | 1 - src/Exception/AuthenticationException.php | 1 - src/Exception/AuthorizationException.php | 1 - src/Exception/FirewallException.php | 1 - src/Security/PasswordHasher.php | 1 - 5 files changed, 5 deletions(-) diff --git a/src/Exception/AccountExpiredException.php b/src/Exception/AccountExpiredException.php index 26ab58a..c873745 100644 --- a/src/Exception/AccountExpiredException.php +++ b/src/Exception/AccountExpiredException.php @@ -4,5 +4,4 @@ class AccountExpiredException extends AuthenticationException { - } diff --git a/src/Exception/AuthenticationException.php b/src/Exception/AuthenticationException.php index 2d2ee1e..6feb768 100644 --- a/src/Exception/AuthenticationException.php +++ b/src/Exception/AuthenticationException.php @@ -4,5 +4,4 @@ class AuthenticationException extends FirewallException { - } diff --git a/src/Exception/AuthorizationException.php b/src/Exception/AuthorizationException.php index 9fcc66d..f371b86 100644 --- a/src/Exception/AuthorizationException.php +++ b/src/Exception/AuthorizationException.php @@ -4,5 +4,4 @@ class AuthorizationException extends FirewallException { - } diff --git a/src/Exception/FirewallException.php b/src/Exception/FirewallException.php index f9d28f0..3562d00 100644 --- a/src/Exception/FirewallException.php +++ b/src/Exception/FirewallException.php @@ -4,5 +4,4 @@ abstract class FirewallException extends \RuntimeException { - } diff --git a/src/Security/PasswordHasher.php b/src/Security/PasswordHasher.php index 23f1627..5452095 100644 --- a/src/Security/PasswordHasher.php +++ b/src/Security/PasswordHasher.php @@ -38,7 +38,6 @@ public function __construct() $opsLimit = max((int)@$_ENV['PWD_HASH_OPSLIMIT'], 4, defined('SODIUM_CRYPTO_PWHASH_OPSLIMIT_INTERACTIVE') ? \SODIUM_CRYPTO_PWHASH_OPSLIMIT_INTERACTIVE : 4); $memLimit = max((int)@$_ENV['PWD_HASH_MEMLIMIT'], 64 * 1024 * 1024, defined('SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE') ? \SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE : 64 * 1024 * 1024); - $this->options = [ 'cost' => $cost, 'time_cost' => $opsLimit, From 4ef543c88c74e081851c06d0cc751ed59b8b5e62 Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 15 Oct 2024 21:05:43 +0000 Subject: [PATCH 07/38] add support for anti-CSRF tokens; enable it in the password-change form; related refactoring --- public/admin/index.php | 4 +- public/admin/login.php | 4 +- public/admin/resetpassword.php | 4 +- public/upload/index.php | 6 +- resources/templates/admin/index.html.twig | 9 +- resources/templates/admin/login.html.twig | 5 +- .../templates/admin/resetpassword.html.twig | 8 +- .../templates/parts/forms/macros.html.twig | 11 +- resources/templates/upload/index.html.twig | 5 +- src/Exception/AntiCSRFException.php | 7 ++ .../TokenHasInvalidFormatException.php | 7 ++ src/Exception/TokenHashMismatchException.php | 7 ++ .../TokenIndexNotInSessionException.php | 7 ++ src/Exception/TokenIsMissingException.php | 7 ++ .../TokenMatchesWrongFormException.php | 7 ++ src/Form/CrashReportBaseForm.php | 4 +- src/Form/CrashReportSearchForm.php | 4 +- src/Form/CrashReportSubmitForm.php | 4 +- src/Form/Field.php | 59 +++++++-- src/Form/Form.php | 21 +++- src/Form/LoginForm.php | 4 +- src/Form/ResetPasswordForm.php | 12 +- src/Security/AntiCSRF.php | 112 ++++++++++++++++++ src/Security/Firewall.php | 4 +- 24 files changed, 280 insertions(+), 42 deletions(-) create mode 100644 src/Exception/AntiCSRFException.php create mode 100644 src/Exception/TokenHasInvalidFormatException.php create mode 100644 src/Exception/TokenHashMismatchException.php create mode 100644 src/Exception/TokenIndexNotInSessionException.php create mode 100644 src/Exception/TokenIsMissingException.php create mode 100644 src/Exception/TokenMatchesWrongFormException.php create mode 100644 src/Security/AntiCSRF.php diff --git a/public/admin/index.php b/public/admin/index.php index 7487639..9915166 100644 --- a/public/admin/index.php +++ b/public/admin/index.php @@ -38,7 +38,7 @@ $pageNum = 0; } -$form = new CrashReportSearchForm(); +$form = new CrashReportSearchForm($router->generate(__FILE__)); if ($form->isSubmitted()) { $form->handleRequest(); if ($form->isValid()) { @@ -54,6 +54,6 @@ 'user' => $user, 'form' => $form, 'reports' => $reports, 'num_reports' => $numReports, 'current_page' => $pageNum, 'page_size' => $pageSize, 'num_pages' => ceil($numReports / $pageSize), 'page_sizes' => $pageSizes, 'urls' => array_merge($firewall->getAdminUrls(), [ - 'form' => $router->generate(__FILE__, $form->getQueryStringParts(true)) + 'paginator' => $router->generate(__FILE__, $form->getQueryStringParts(true)) ]), ]); diff --git a/public/admin/login.php b/public/admin/login.php index e092bfe..c30a9d4 100644 --- a/public/admin/login.php +++ b/public/admin/login.php @@ -11,7 +11,7 @@ use Veracrypt\CrashCollector\Templating; $router = new Router(); -$form = new LoginForm($router->generate(__DIR__ . '/index.php')); +$form = new LoginForm($router->generate(__FILE__), $router->generate(__DIR__ . '/index.php')); if ($form->isSubmitted()) { $form->handleRequest(); @@ -43,5 +43,5 @@ $tpl = new Templating(); echo $tpl->render('admin/login.html.twig', [ 'form' => $form, - 'urls' => array_merge($firewall->getAdminUrls(), ['form' => $router->generate(__FILE__)]), + 'urls' => $firewall->getAdminUrls(), ]); diff --git a/public/admin/resetpassword.php b/public/admin/resetpassword.php index 2db0ddf..0db8cd7 100644 --- a/public/admin/resetpassword.php +++ b/public/admin/resetpassword.php @@ -23,7 +23,7 @@ exit(); } -$form = new ResetPasswordForm($user); +$form = new ResetPasswordForm($router->generate(__FILE__), $user); if ($form->isSubmitted()) { $form->handleRequest(); if ($form->isValid()) { @@ -36,5 +36,5 @@ echo $tpl->render('admin/resetpassword.html.twig', [ 'user' => $user, 'form' => $form, - 'urls' => array_merge($firewall->getAdminUrls(), ['form' => $router->generate(__FILE__)]), + 'urls' => $firewall->getAdminUrls(), ]); diff --git a/public/upload/index.php b/public/upload/index.php index 9f2f1f8..614827b 100644 --- a/public/upload/index.php +++ b/public/upload/index.php @@ -7,7 +7,8 @@ use Veracrypt\CrashCollector\Router; use Veracrypt\CrashCollector\Templating; -$form = new CrashReportSubmitForm(); +$router = new Router(); +$form = new CrashReportSubmitForm($router->generate(__FILE__)); // allow to pre-fill form fields using GET request, but only act on POST if ($form->isSubmitted()) { @@ -22,9 +23,8 @@ $form->handleRequest($_GET); } -$router = new Router(); $tpl = new Templating(); echo $tpl->render('upload/index.html.twig', [ 'form' => $form, - 'urls' => ['form' => $router->generate(__FILE__), 'root' => $router->generate(__DIR__ . '/..')], + 'urls' => ['root' => $router->generate(__DIR__ . '/..')], ]); diff --git a/resources/templates/admin/index.html.twig b/resources/templates/admin/index.html.twig index e324619..ba71a9f 100644 --- a/resources/templates/admin/index.html.twig +++ b/resources/templates/admin/index.html.twig @@ -3,7 +3,10 @@ {% block content %} {% import "parts/forms/macros.html.twig" as forms %}
-
+ + {% if form.errorMessage|default('') != '' %} +

{{ form.errorMessage }}

+ {% endif %} {{ include('parts/forms/cr_common_fields.html.twig') }}
{{ forms.input(form.getField('minDate'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-4', 'input': 'form-control'}) }} @@ -29,7 +32,7 @@
{% if form.isSubmitted() and form.isValid() %}

Found {{ num_reports }} crash reports{% if num_reports > page_size %}, showing {{ page_size }} per page{% endif %}

-{{ forms.paginator(current_page, num_pages, urls.form) }} +{{ forms.paginator(current_page, num_pages, urls.paginator) }} @@ -60,7 +63,7 @@ {% endfor %}
-{{ forms.paginator(current_page, num_pages, urls.form) }} +{{ forms.paginator(current_page, num_pages, urls.paginator) }} {% else %} Note: accepted wildcard characters are the ones for SQL LIKE statements: '_' for any one char, and '?' for zero or more chars diff --git a/resources/templates/admin/login.html.twig b/resources/templates/admin/login.html.twig index 2d77ea4..2035049 100644 --- a/resources/templates/admin/login.html.twig +++ b/resources/templates/admin/login.html.twig @@ -3,7 +3,10 @@ {% block content %} {% import "parts/forms/macros.html.twig" as forms %} {# @todo make the form a bit less wide #} - + + {% if form.errorMessage|default('') != '' %} +

{{ form.errorMessage }}

+ {% endif %}
{{ forms.input(form.getField('username'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }}
diff --git a/resources/templates/admin/resetpassword.html.twig b/resources/templates/admin/resetpassword.html.twig index c06bebc..76d99d6 100644 --- a/resources/templates/admin/resetpassword.html.twig +++ b/resources/templates/admin/resetpassword.html.twig @@ -6,7 +6,10 @@

The password has been updated.

{% else %} {# @todo make the form a bit less wide #} - + + {% if form.errorMessage|default('') != '' %} +

{{ form.errorMessage }}

+ {% endif %}
{{ forms.input(form.getField('oldPassword'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }}
@@ -17,6 +20,9 @@ {{ forms.input(form.getField('newPasswordConfirm'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }}
+ {# note: we could make the form transparently inject the antiCSRF input, to help devs not forget displaying it... + but then it would be easy to find out that the form never submits succesfully ;-) #} + {{ forms.input(form.getField('antiCSRF')) }} {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }}
diff --git a/resources/templates/parts/forms/macros.html.twig b/resources/templates/parts/forms/macros.html.twig index 5c0a43f..54ea708 100644 --- a/resources/templates/parts/forms/macros.html.twig +++ b/resources/templates/parts/forms/macros.html.twig @@ -1,9 +1,15 @@ {% macro input(field, css_classes=[], input_attributes=[]) %} {# @todo allow better styling of the error message: align it below the input field #} - {% if field.inputType == 'datetime' %} + {% if field.inputType == 'anticsrf' %} + + {% elseif field.inputType == 'datetime' %}
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% elseif field.inputType == 'email' %} + +
+ {% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'hidden' %} {% elseif field.inputType == 'submit' %} @@ -20,6 +26,9 @@
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% else %} + {# @todo we should really throw a \DomainException... #} +
ERROR! unsupported form field type: '{{ field.inputType }}'
{% endif %} {% endmacro %} diff --git a/resources/templates/upload/index.html.twig b/resources/templates/upload/index.html.twig index dd4c924..bc6495a 100644 --- a/resources/templates/upload/index.html.twig +++ b/resources/templates/upload/index.html.twig @@ -5,7 +5,10 @@ {% if form.isSubmitted() and form.isValid() %}

Thank you for taking the time to submit the information and sorry for your hassles.

{% else %} -
+ + {% if form.errorMessage|default('') != '' %} +

{{ form.errorMessage }}

+ {% endif %} {{ include('parts/forms/cr_common_fields.html.twig') }}
{{ forms.input(form.getField('callStack'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }} diff --git a/src/Exception/AntiCSRFException.php b/src/Exception/AntiCSRFException.php new file mode 100644 index 0000000..217a572 --- /dev/null +++ b/src/Exception/AntiCSRFException.php @@ -0,0 +1,7 @@ +fields = [ /// @todo get the field lengths from the Repo fields @@ -20,5 +20,7 @@ public function __construct() 'errorCategory' => new Field('Error category', 'ec', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), 'errorAddress' => new Field('Error address', 'ea', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), ]; + + parent::__construct($actionUrl); } } diff --git a/src/Form/CrashReportSearchForm.php b/src/Form/CrashReportSearchForm.php index bb4f463..d26a25d 100644 --- a/src/Form/CrashReportSearchForm.php +++ b/src/Form/CrashReportSearchForm.php @@ -9,9 +9,9 @@ class CrashReportSearchForm extends CrashReportBaseForm protected string $submitLabel = 'Search'; protected int $submitOn = self::ON_GET; - public function __construct() + public function __construct(string $actionUrl) { - parent::__construct(); + parent::__construct($actionUrl); $this->fields['minDate'] = new Field('After', 'da', 'datetime'); $this->fields['maxDate'] = new Field('Before', 'db', 'datetime'); } diff --git a/src/Form/CrashReportSubmitForm.php b/src/Form/CrashReportSubmitForm.php index b2b8fff..83e959e 100644 --- a/src/Form/CrashReportSubmitForm.php +++ b/src/Form/CrashReportSubmitForm.php @@ -8,9 +8,9 @@ class CrashReportSubmitForm extends CrashReportBaseForm { protected bool $requireAllFieldsByDefault = true; - public function __construct() + public function __construct(string $actionUrl) { - parent::__construct(); + parent::__construct($actionUrl); $this->fields['callStack'] = new Field('Call stack', 'cs', 'textarea', [FC::Required => $this->requireAllFieldsByDefault]); } } diff --git a/src/Form/Field.php b/src/Form/Field.php index a4595af..7fb16c3 100644 --- a/src/Form/Field.php +++ b/src/Form/Field.php @@ -2,7 +2,10 @@ namespace Veracrypt\CrashCollector\Form; +use Veracrypt\CrashCollector\Exception\AntiCSRFException; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; +use Veracrypt\CrashCollector\Logger; +use Veracrypt\CrashCollector\Security\AntiCSRF; /** * @property-read ?string $value @@ -10,12 +13,11 @@ */ class Field { + const SupportedFieldTypes = ['anticsrf', 'datetime', 'email', 'hidden', 'password', 'submit', 'text', 'textarea']; + protected mixed $value = null; protected ?string $errorMessage = null; - /** - * NB: constraints are checked in the order they are defined - */ public function __construct( public readonly string $label, public readonly string $inputName, @@ -24,7 +26,14 @@ public function __construct( mixed $value = null ) { - $this->value = $value; + // type validation + if (!in_array($this->inputType, self::SupportedFieldTypes)) { + throw new \DomainException("Unsupported field type: $inputType"); + } + + /// @todo for 'anticsrf' fields, check that there are no constraints defined, as it does not check those anyway. + /// Also, we should prevent those fields to be used on forms which submit via GET + // constraint validation foreach ($this->constraints as $constraint => $targetValue) { switch ($constraint) { @@ -39,21 +48,37 @@ public function __construct( throw new \DomainException("Unsupported field constraint: '$constraint"); } } + $this->value = $value; } /** - * @param mixed $value + * Used to set (and validate) the submitted value. + * NB: constraints are checked in the order they are defined * @return bool false when the value is not valid - * @throws \DomainException */ public function setValue(mixed $value): bool { $isValid = true; - if ($value !== null) { + if ($value !== null || $this->inputType === 'anticsrf') { $value = trim($value); switch($this->inputType) { + case 'anticsrf': + // we always store null, so that the csrf token will be omitted from the form's getData + $this->value = null; + $antiCSRF = new AntiCSRF(); + try { + /// @todo allow checking the token against the form it was generated on + $antiCSRF->validateToken($value); + return true; + } catch (AntiCSRFException $e) { + $this->errorMessage = 'The ANTI-CSRF token has been tampered or is missing'; + $logger = Logger::getInstance('audit'); + $logger->info('ANTI-CSRF token tampering attempt. ' . $e->getMessage()); + return false; + } + break; case 'datetime': if ($value !== '') { if (strtotime($value) === false) { @@ -89,8 +114,9 @@ public function setValue(mixed $value): bool return false; } break; - default: - throw new \DomainException("Unsupported field constraint: '$constraint"); + // this is checked at constructor time + //default: + // throw new \DomainException("Unsupported field constraint: '$constraint"); } } @@ -123,6 +149,21 @@ public function getMaxLength(): ?int return array_key_exists(FC::MaxLength, $this->constraints) ? (int)$this->constraints[FC::MaxLength] : null; } + public function isVisible(): bool + { + return !in_array($this->inputType, ['anticsrf', 'hidden']); + } + + public function getAntiCSRFToken() + { + if ($this->inputType !== 'anticsrf') { + throw new \DomainException("getAntiCSRFToken called on form field of type '{$this->inputType}'"); + } + + $antiCSRF = new AntiCSRF(); + return $antiCSRF->getToken(); + } + public function __get($name) { switch ($name) { diff --git a/src/Form/Form.php b/src/Form/Form.php index 35c683c..a1b41f1 100644 --- a/src/Form/Form.php +++ b/src/Form/Form.php @@ -4,6 +4,7 @@ /** * @property-read ?string $errorMessage + * @property-read string $actionUrl */ abstract class Form { @@ -11,12 +12,18 @@ abstract class Form const ON_POST = 2; const ON_BOTH = 3; - /** @var Field[] */ + /** @var Field[] $fields */ protected array $fields = []; protected string $submitLabel = 'Submit'; protected int $submitOn = self::ON_POST; protected bool $isValid = false; - protected string $errorMessage; + protected ?string $errorMessage = null; + protected string $actionUrl; + + public function __construct(string $actionUrl) + { + $this->actionUrl = $actionUrl; + } /** * @throws \DomainException @@ -39,9 +46,7 @@ public function getMethod(): string public function getSubmit(): Field { - $f = new Field($this->submitLabel, 's', 'submit'); - $f->setValue(1); - return $f; + return new Field($this->submitLabel, 's', 'submit', [], 1); } public function isSubmitted(?array $request = null): bool @@ -58,7 +63,6 @@ public function isValid(): bool return $this->isValid; } - /// @todo if a hidden field is invalid, set $this->errorMessage to something (what?) public function handleRequest(?array $request = null): void { $this->isValid = true; @@ -67,6 +71,9 @@ public function handleRequest(?array $request = null): void } foreach($this->fields as &$field) { if (!$field->setValue(array_key_exists($field->inputName, $request) ? $request[$field->inputName] : null)) { + if (!$field->isVisible()) { + $this->setError($field->errorMessage); + } $this->isValid = false; } } @@ -123,6 +130,7 @@ public function setError(string $errorMessage) public function __get($name) { switch ($name) { + case 'actionUrl': case 'errorMessage': return $this->$name; default: @@ -135,6 +143,7 @@ public function __get($name) public function __isset($name) { return match ($name) { + 'actionUrl' => true, 'errorMessage' => isset($this->$name), default => false }; diff --git a/src/Form/LoginForm.php b/src/Form/LoginForm.php index 2b7015b..969d117 100644 --- a/src/Form/LoginForm.php +++ b/src/Form/LoginForm.php @@ -10,7 +10,7 @@ class LoginForm extends BaseForm { protected string $submitLabel = 'Login'; - public function __construct(string $redirect) + public function __construct(string $actionUrl, string $redirect) { $this->fields = [ /// @todo get the field length from the Repo fields @@ -18,5 +18,7 @@ public function __construct(string $redirect) 'password' => new Field('Password', 'pw', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), 'redirect' => new Field('', 'r', 'hidden', [FC::Required => true], $redirect) ]; + + parent::__construct($actionUrl); } } diff --git a/src/Form/ResetPasswordForm.php b/src/Form/ResetPasswordForm.php index 563b884..b0c5b99 100644 --- a/src/Form/ResetPasswordForm.php +++ b/src/Form/ResetPasswordForm.php @@ -12,14 +12,18 @@ class ResetPasswordForm extends Form { protected UserInterface $currentUser; - public function __construct(UserInterface $currentUser) + public function __construct(string $actionUrl, UserInterface $currentUser) { $this->fields = [ + /// @todo add min pwd length constraints, maybe even a regex? 'oldPassword' => new Field('Current Password', 'cp', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), 'newPassword' => new Field('New Password', 'np', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), 'newPasswordConfirm' => new Field('Confirm new Password', 'npc', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'antiCSRF' => new Field('', 'ac', 'anticsrf'), ]; $this->currentUser = $currentUser; + + parent::__construct($actionUrl); } public function handleRequest(?array $request = null): void @@ -27,8 +31,10 @@ public function handleRequest(?array $request = null): void parent::handleRequest($request); if ($this->isValid) { - if ($this->fields['newPassword']->getData() !== $this->fields['newPasswordConfirm']->getData()) { - $this->fields['newPasswordConfirm']->setError('The password does not match'); + /** @var Field $npcField */ + $npcField =& $this->fields['newPasswordConfirm']; + if ($this->fields['newPassword']->getData() !== $npcField->getData()) { + $npcField->setError('The password does not match'); $this->isValid = false; } else { $authenticator = new UsernamePasswordAuthenticator(); diff --git a/src/Security/AntiCSRF.php b/src/Security/AntiCSRF.php new file mode 100644 index 0000000..6dc73da --- /dev/null +++ b/src/Security/AntiCSRF.php @@ -0,0 +1,112 @@ +get($this->storageKey, []); + list($tokenIndex, $tokenData) = $this->generateToken($lockTo); + $tokens[$tokenIndex] = $tokenData; + $tokens = $this->pruneTokens($tokens); + $session->set($this->storageKey, $tokens); + return "{$tokenIndex}:{$tokenData['token']}"; + } + + /** + * @param ?string $lockTo + * @return array 1st element: token index (string), 2nd: token data (array) + * @throws \Exception in case not enough randomness can be mustered + * @todo allow to optionally lock down to the current client's IP + */ + protected function generateToken(?string $lockTo): array + { + $index = base64_encode(random_bytes(18)); + $token = base64_encode(random_bytes(33)); + + return [$index, [ + 'created' => time(), + 'token' => $token, + 'lockTo' => $lockTo, + 'uri' => isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : $_SERVER['SCRIPT_NAME'], + ]]; + } + + /** + * Enforce an upper limit on the number of tokens stored by removing the oldest tokens first. + */ + protected function pruneTokens($tokensArray): array + { + if (count($tokensArray) <= $this->maxTokens) { + return $tokensArray; + } + // sort newest first (bigger creation time) + uasort($tokensArray, function (array $a, array $b): int { + return -1 * ($a['created'] <=> $b['created']); + }); + $tokensArray = array_slice($tokensArray, 0, $this->maxTokens, true); + return $tokensArray; + } + + /** + * @throws AntiCSRFException + * @throws \RuntimeException + */ + public function validateToken(string $token, ?string $lockTo = ''): void + { + if ($token === '') { + throw new TokenIsMissingException('Anti-CSRF token not found'); + } + $parts = explode(':', $token); + if (count($parts) !== 2) { + throw new TokenHasInvalidFormatException('Anti-CSRF token has an unsupported format'); + } + $tokenIndex = $parts[0]; + $tokenHash = $parts[1]; + + $session = Session::getInstance(); + $tokens = $session->get($this->storageKey, []); + if (!array_key_exists($tokenIndex, $tokens)) { + throw new TokenIndexNotInSessionException('Anti-CSRF token index not found in the session'); + } + $token = $tokens[$tokenIndex]; + + // remove the token from the session + unset($tokens[$tokenIndex]); + $session->set($this->storageKey, $tokens); + + // note: we do not check that the token creation date make is recent because we store them in the session, + // so their lifetime is naturally limited (based on the session configuration) + + if (!hash_equals($tokenHash, $token['token'])) { + throw new TokenHashMismatchException('Anti-CSRF did not match the stored value'); + } + if ($lockTo !== null && $token['lockTo'] !== null && !hash_equals($lockTo, $token['lockTo'])) { + throw new TokenMatchesWrongFormException('Anti-CSRF token used on the wrong form'); + } + } +} diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index c823009..9ffecc2 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -140,8 +140,8 @@ public function displayAdminLoginPage(string $successRedirectUrl): void $router = new Router(); $tpl = new Templating(); echo $tpl->render('admin/login.html.twig', [ - 'form' => new LoginForm($successRedirectUrl), - 'urls' => array_merge($this->getAdminUrls(), ['form' => $router->generate(__DIR__ . '/../../public/admin/login.php')]), + 'form' => new LoginForm($router->generate(__DIR__ . '/../../public/admin/login.php'), $successRedirectUrl), + 'urls' => $this->getAdminUrls(), ]); } From 88a061e7c1454f25da1b063a7d3ed95c47970776 Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 16 Oct 2024 12:26:25 +0000 Subject: [PATCH 08/38] refactor the form fields class; tie anticsrf tokens to the form showing them --- public/admin/login.php | 3 +- .../templates/parts/forms/macros.html.twig | 4 +- src/Form/CrashReportBaseForm.php | 12 +- src/Form/CrashReportSearchForm.php | 9 +- src/Form/CrashReportSubmitForm.php | 2 +- src/Form/Field.php | 144 ++++++++---------- src/Form/Field/AntiCSRF.php | 43 ++++++ src/Form/Field/DateTime.php | 38 +++++ src/Form/Field/Email.php | 25 +++ src/Form/Field/Hidden.php | 13 ++ src/Form/Field/Password.php | 16 ++ src/Form/Field/SubmitButton.php | 16 ++ src/Form/Field/Text.php | 13 ++ src/Form/Field/TextArea.php | 13 ++ src/Form/FieldConstraint.php | 1 + src/Form/Form.php | 25 ++- src/Form/LoginForm.php | 6 +- src/Form/ResetPasswordForm.php | 42 +++-- 18 files changed, 301 insertions(+), 124 deletions(-) create mode 100644 src/Form/Field/AntiCSRF.php create mode 100644 src/Form/Field/DateTime.php create mode 100644 src/Form/Field/Email.php create mode 100644 src/Form/Field/Hidden.php create mode 100644 src/Form/Field/Password.php create mode 100644 src/Form/Field/SubmitButton.php create mode 100644 src/Form/Field/Text.php create mode 100644 src/Form/Field/TextArea.php diff --git a/public/admin/login.php b/public/admin/login.php index c30a9d4..30ff9e0 100644 --- a/public/admin/login.php +++ b/public/admin/login.php @@ -16,15 +16,16 @@ if ($form->isSubmitted()) { $form->handleRequest(); if ($form->isValid()) { - $authenticator = new UsernamePasswordAuthenticator(); $data = $form->getData(); $redirectUrl = $data['redirect']; + /// @todo we should move this validation to the LoginForm if (!$router->match($redirectUrl)) { $form->setError('Tsk tsk tsk'); $logger = Logger::getInstance('audit'); $logger->warning("Hacking attempt: login fom submitted with invalid redirect url '$redirectUrl'"); } else { unset($data['redirect']); + $authenticator = new UsernamePasswordAuthenticator(); try { $authenticator->authenticate(...$data); header('Location: ' . $redirectUrl, true, 303); diff --git a/resources/templates/parts/forms/macros.html.twig b/resources/templates/parts/forms/macros.html.twig index 54ea708..5fb5da1 100644 --- a/resources/templates/parts/forms/macros.html.twig +++ b/resources/templates/parts/forms/macros.html.twig @@ -2,7 +2,7 @@ {# @todo allow better styling of the error message: align it below the input field #} {% if field.inputType == 'anticsrf' %} - {% elseif field.inputType == 'datetime' %} + {% elseif field.inputType == 'datetime-local' %}
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} @@ -12,7 +12,7 @@ {% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'hidden' %} - {% elseif field.inputType == 'submit' %} + {% elseif field.inputType == 'submit-button' %}
{% elseif field.inputType == 'password' %} diff --git a/src/Form/CrashReportBaseForm.php b/src/Form/CrashReportBaseForm.php index 587e4b6..ee0558a 100644 --- a/src/Form/CrashReportBaseForm.php +++ b/src/Form/CrashReportBaseForm.php @@ -13,12 +13,12 @@ public function __construct(string $actionUrl) { $this->fields = [ /// @todo get the field lengths from the Repo fields - 'programVersion' => new Field('Program version', 'pv', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'osVersion' => new Field('OS version', 'ov', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'hwArchitecture' => new Field('Architecture', 'ha', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'executableChecksum' => new Field('Executable checksum', 'ck', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'errorCategory' => new Field('Error category', 'ec', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'errorAddress' => new Field('Error address', 'ea', 'text', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), + 'programVersion' => new Field\Text('Program version', 'pv', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), + 'osVersion' => new Field\Text('OS version', 'ov', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), + 'hwArchitecture' => new Field\Text('Architecture', 'ha', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), + 'executableChecksum' => new Field\Text('Executable checksum', 'ck', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), + 'errorCategory' => new Field\Text('Error category', 'ec', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), + 'errorAddress' => new Field\Text('Error address', 'ea', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), ]; parent::__construct($actionUrl); diff --git a/src/Form/CrashReportSearchForm.php b/src/Form/CrashReportSearchForm.php index d26a25d..a2ef0e6 100644 --- a/src/Form/CrashReportSearchForm.php +++ b/src/Form/CrashReportSearchForm.php @@ -2,8 +2,9 @@ namespace Veracrypt\CrashCollector\Form; -use Veracrypt\CrashCollector\Form\FieldConstraint as FC; - +/** + * @todo we could implement custom validation checks in handleRequest + */ class CrashReportSearchForm extends CrashReportBaseForm { protected string $submitLabel = 'Search'; @@ -12,7 +13,7 @@ class CrashReportSearchForm extends CrashReportBaseForm public function __construct(string $actionUrl) { parent::__construct($actionUrl); - $this->fields['minDate'] = new Field('After', 'da', 'datetime'); - $this->fields['maxDate'] = new Field('Before', 'db', 'datetime'); + $this->fields['minDate'] = new Field\DateTime('After', 'da'); + $this->fields['maxDate'] = new Field\DateTime('Before', 'db'); } } diff --git a/src/Form/CrashReportSubmitForm.php b/src/Form/CrashReportSubmitForm.php index 83e959e..bb12148 100644 --- a/src/Form/CrashReportSubmitForm.php +++ b/src/Form/CrashReportSubmitForm.php @@ -11,6 +11,6 @@ class CrashReportSubmitForm extends CrashReportBaseForm public function __construct(string $actionUrl) { parent::__construct($actionUrl); - $this->fields['callStack'] = new Field('Call stack', 'cs', 'textarea', [FC::Required => $this->requireAllFieldsByDefault]); + $this->fields['callStack'] = new Field\TextArea('Call stack', 'cs', [FC::Required => $this->requireAllFieldsByDefault]); } } diff --git a/src/Form/Field.php b/src/Form/Field.php index 7fb16c3..034db60 100644 --- a/src/Form/Field.php +++ b/src/Form/Field.php @@ -2,40 +2,40 @@ namespace Veracrypt\CrashCollector\Form; -use Veracrypt\CrashCollector\Exception\AntiCSRFException; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; -use Veracrypt\CrashCollector\Logger; -use Veracrypt\CrashCollector\Security\AntiCSRF; /** * @property-read ?string $value * @property-read ?string $errorMessage + * @property-read bool isValid */ -class Field +abstract class Field { - const SupportedFieldTypes = ['anticsrf', 'datetime', 'email', 'hidden', 'password', 'submit', 'text', 'textarea']; - - protected mixed $value = null; + //public readonly string $label; + //public readonly string $inputName; + //public readonly string $inputType; + //public readonly array $constraints; + //protected mixed $value = null; protected ?string $errorMessage = null; - public function __construct( + protected function __construct( + public readonly string $inputType, public readonly string $label, public readonly string $inputName, - public readonly string $inputType, public readonly array $constraints = [], - mixed $value = null + protected mixed $value = null, + public readonly bool $isVisible = true ) { - // type validation - if (!in_array($this->inputType, self::SupportedFieldTypes)) { - throw new \DomainException("Unsupported field type: $inputType"); - } - - /// @todo for 'anticsrf' fields, check that there are no constraints defined, as it does not check those anyway. - /// Also, we should prevent those fields to be used on forms which submit via GET + $this->validateConstraintsDefinitions($constraints); + } - // constraint validation - foreach ($this->constraints as $constraint => $targetValue) { + /** + * @throws \DomainException + */ + protected function validateConstraintsDefinitions(array $constraints): void + { + foreach ($constraints as $constraint => $targetValue) { switch ($constraint) { case FC::Required: break; @@ -48,59 +48,47 @@ public function __construct( throw new \DomainException("Unsupported field constraint: '$constraint"); } } - $this->value = $value; } /** - * Used to set (and validate) the submitted value. - * NB: constraints are checked in the order they are defined + * Used to set (and validate) the value submitted. + * NB: constraints are checked in the order they are defined. + * @param mixed $value null is used when the field is not present in the request received * @return bool false when the value is not valid */ public function setValue(mixed $value): bool { - $isValid = true; - - if ($value !== null || $this->inputType === 'anticsrf') { - $value = trim($value); - - switch($this->inputType) { - case 'anticsrf': - // we always store null, so that the csrf token will be omitted from the form's getData - $this->value = null; - $antiCSRF = new AntiCSRF(); - try { - /// @todo allow checking the token against the form it was generated on - $antiCSRF->validateToken($value); - return true; - } catch (AntiCSRFException $e) { - $this->errorMessage = 'The ANTI-CSRF token has been tampered or is missing'; - $logger = Logger::getInstance('audit'); - $logger->info('ANTI-CSRF token tampering attempt. ' . $e->getMessage()); - return false; - } - break; - case 'datetime': - if ($value !== '') { - if (strtotime($value) === false) { - $this->errorMessage = 'Value is not a valid datetime'; - $isValid = false; - } - } else { - // we allow either valid datetime strings, or null. No empty strings - $value = null; - } - break; - } - } - - $this->value = $value; + $this->value = $this->validateValue($value); - if (!$isValid) { + if (null !== $this->errorMessage && '' !== $this->errorMessage) { return false; } + return $this->validateConstraints($value); + } + + /** + * Used to validate and optionally convert to the desired representation the value submitted. + * By default, it converts non-null values to strings and trims whitespace. Null is received when the field is not + * present in the request received and it should generally be let through unchanged. + * NB: should set $this->errorMessage if a non-constraint is violated. + */ + protected function validateValue(mixed $value): null|string + { + return match ($value) { + null => null, + default => trim($value), + }; + } + + /** + * Used to validate the value submitted. + * NB: sets $this->errorMessage if a constraint is violated. + * @todo add support for more constraints: regex, ... + */ + protected function validateConstraints(?string $value): bool + { foreach ($this->constraints as $constraint => $targetValue) { - /// @todo add validation for minLength, regex, integer fields, datetimes, etc... switch ($constraint) { case FC::Required: if ($targetValue && ($value === '' || $value === null)) { @@ -114,6 +102,12 @@ public function setValue(mixed $value): bool return false; } break; + case FC::MinLength: + if ($targetValue > 0 && strlen($value) < $targetValue) { + $this->errorMessage = "Value should not be shorter than {$targetValue} characters"; + return false; + } + break; // this is checked at constructor time //default: // throw new \DomainException("Unsupported field constraint: '$constraint"); @@ -128,15 +122,12 @@ public function setError(string $erroMessage) $this->errorMessage = $erroMessage; } - public function getData(): null|string|\DateTimeImmutable + /** + * Returns the field value as usable by php code, which might differ from what is output + */ + public function getData(): mixed { - if ($this->value === null) { - return $this->value; - } - return match($this->inputType) { - 'datetime' => new \DateTimeImmutable($this->value), - default => $this->value, - }; + return $this->value; } public function isRequired(): bool @@ -149,19 +140,9 @@ public function getMaxLength(): ?int return array_key_exists(FC::MaxLength, $this->constraints) ? (int)$this->constraints[FC::MaxLength] : null; } - public function isVisible(): bool + public function getMinLength(): ?int { - return !in_array($this->inputType, ['anticsrf', 'hidden']); - } - - public function getAntiCSRFToken() - { - if ($this->inputType !== 'anticsrf') { - throw new \DomainException("getAntiCSRFToken called on form field of type '{$this->inputType}'"); - } - - $antiCSRF = new AntiCSRF(); - return $antiCSRF->getToken(); + return array_key_exists(FC::MinLength, $this->constraints) ? (int)$this->constraints[FC::MinLength] : null; } public function __get($name) @@ -170,6 +151,8 @@ public function __get($name) case 'value': case 'errorMessage': return $this->$name; + case 'isValid': + return null !== $this->errorMessage && '' !== $this->errorMessage; default: $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 1); trigger_error('Undefined property via __get(): ' . $name . ' in ' . $trace[0]['file'] . ' on line ' . @@ -181,6 +164,7 @@ public function __isset($name) { return match ($name) { 'value', 'errorMessage' => isset($this->$name), + 'isValid' => true, default => false }; } diff --git a/src/Form/Field/AntiCSRF.php b/src/Form/Field/AntiCSRF.php new file mode 100644 index 0000000..db08223 --- /dev/null +++ b/src/Form/Field/AntiCSRF.php @@ -0,0 +1,43 @@ +validateToken($value, $this->formActionUrl); + } catch (AntiCSRFException $e) { + $this->errorMessage = 'The ANTI-CSRF token has been tampered or is missing'; + $logger = Logger::getInstance('audit'); + $logger->info('ANTI-CSRF token tampering attempt. ' . $e->getMessage()); + } + return null; + } + + public function getAntiCSRFToken(): string + { + $antiCSRF = new AntiCSRFTokenManager(); + return $antiCSRF->getToken($this->formActionUrl); + } +} diff --git a/src/Form/Field/DateTime.php b/src/Form/Field/DateTime.php new file mode 100644 index 0000000..9c34d84 --- /dev/null +++ b/src/Form/Field/DateTime.php @@ -0,0 +1,38 @@ +errorMessage = 'Value is not a valid datetime'; + } + return $value; + } + + public function getData(): null|\DateTimeImmutable + { + return match($this->value) { + null => null, + default => new \DateTimeImmutable($this->value), + }; + } +} diff --git a/src/Form/Field/Email.php b/src/Form/Field/Email.php new file mode 100644 index 0000000..ce0c430 --- /dev/null +++ b/src/Form/Field/Email.php @@ -0,0 +1,25 @@ +errorMessage = 'Value is not a valid email address'; + } + return $value; + } +} diff --git a/src/Form/Field/Hidden.php b/src/Form/Field/Hidden.php new file mode 100644 index 0000000..e3aa372 --- /dev/null +++ b/src/Form/Field/Hidden.php @@ -0,0 +1,13 @@ +submitLabel, 's', 'submit', [], 1); + return new SubmitButton($this->submitLabel, 's', [], 1); } public function isSubmitted(?array $request = null): bool @@ -71,12 +73,27 @@ public function handleRequest(?array $request = null): void } foreach($this->fields as &$field) { if (!$field->setValue(array_key_exists($field->inputName, $request) ? $request[$field->inputName] : null)) { - if (!$field->isVisible()) { + // in case the field is not shown to the end user, we show its error message as the form's error message + if (!$field->isVisible) { $this->setError($field->errorMessage); } $this->isValid = false; } } + + if ($this->isValid()) { + $this->validateSubmit($request); + } + } + + /** + * To be overridden in forms which have custom validation rules besides single field validation. + * Called after field validation, only if all the fields did validate. + * Should set $this->isValid and $this->errorMessage if there's anything wrong. + * Should work preferably with values from $this->fields rather than $request, which is passed in as a commodity + */ + protected function validateSubmit(?array $request = null): void + { } public function getData(): array @@ -121,10 +138,10 @@ protected function getRequest() } } - public function setError(string $errorMessage) + public function setError(?string $errorMessage) { $this->errorMessage = $errorMessage; - $this->isValid = false; + $this->isValid = ($errorMessage !== null && $errorMessage !== ''); } public function __get($name) diff --git a/src/Form/LoginForm.php b/src/Form/LoginForm.php index 969d117..ea6e71d 100644 --- a/src/Form/LoginForm.php +++ b/src/Form/LoginForm.php @@ -14,9 +14,9 @@ public function __construct(string $actionUrl, string $redirect) { $this->fields = [ /// @todo get the field length from the Repo fields - 'username' => new Field('Username', 'un', 'text', [FC::Required => true, FC::MaxLength => 180]), - 'password' => new Field('Password', 'pw', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'redirect' => new Field('', 'r', 'hidden', [FC::Required => true], $redirect) + 'username' => new Field\Text('Username', 'un', [FC::Required => true, FC::MaxLength => 180]), + 'password' => new Field\Password('Password', 'pw', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'redirect' => new Field\Hidden('r', [FC::Required => true], $redirect) ]; parent::__construct($actionUrl); diff --git a/src/Form/ResetPasswordForm.php b/src/Form/ResetPasswordForm.php index b0c5b99..8e76fe2 100644 --- a/src/Form/ResetPasswordForm.php +++ b/src/Form/ResetPasswordForm.php @@ -16,38 +16,34 @@ public function __construct(string $actionUrl, UserInterface $currentUser) { $this->fields = [ /// @todo add min pwd length constraints, maybe even a regex? - 'oldPassword' => new Field('Current Password', 'cp', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'newPassword' => new Field('New Password', 'np', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'newPasswordConfirm' => new Field('Confirm new Password', 'npc', 'password', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'antiCSRF' => new Field('', 'ac', 'anticsrf'), + 'oldPassword' => new Field\Password('Current Password', 'cp', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'newPassword' => new Field\Password('New Password', 'np', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'newPasswordConfirm' => new Field\Password('Confirm new Password', 'npc', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'antiCSRF' => new Field\AntiCSRF('ac', $actionUrl), ]; $this->currentUser = $currentUser; parent::__construct($actionUrl); } - public function handleRequest(?array $request = null): void + protected function validateSubmit(?array $request = null): void { - parent::handleRequest($request); - - if ($this->isValid) { - /** @var Field $npcField */ - $npcField =& $this->fields['newPasswordConfirm']; - if ($this->fields['newPassword']->getData() !== $npcField->getData()) { - $npcField->setError('The password does not match'); + /** @var Field $npcField */ + $npcField =& $this->fields['newPasswordConfirm']; + if ($this->fields['newPassword']->getData() !== $npcField->getData()) { + $npcField->setError('The password does not match'); + $this->isValid = false; + } else { + $authenticator = new UsernamePasswordAuthenticator(); + try { + $authenticator->authenticate($this->currentUser->getUserIdentifier(), $this->fields['oldPassword']->getData()); + } catch (BadCredentialsException) { + $this->fields['oldPassword']->setError('The current password is wrong'); $this->isValid = false; - } else { - $authenticator = new UsernamePasswordAuthenticator(); - try { - $authenticator->authenticate($this->currentUser->getUserIdentifier(), $this->fields['oldPassword']->getData()); - } catch (BadCredentialsException) { - $this->fields['oldPassword']->setError('The current password is wrong'); - $this->isValid = false; - } - /// @todo what to do in case we get an AccountExpiredException or UserNotFoundException? - /// This can happen, hopefully infrequently, when the form is displayed to a user still active, and - /// then submitted after the user got deactivated/deleted } + /// @todo what to do in case we get an AccountExpiredException or UserNotFoundException? + /// This can happen, hopefully infrequently, when the form is displayed to a user still active, and + /// then submitted after the user got deactivated/deleted } } } From 8b4324b9b8fe245dd18de24a22578491357d19ee Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 16 Oct 2024 13:46:43 +0000 Subject: [PATCH 09/38] complete form field refactoring: avoid double error message in login form; improve code validating of redirect fields; fix php error if post to pwd change form misses the csrf token; add to html inputs minLength contraint if specified by the field --- public/admin/login.php | 26 +++++++----------- resources/templates/admin/login.html.twig | 3 --- .../templates/parts/forms/macros.html.twig | 16 ++++++++--- src/Form/Field/AntiCSRF.php | 2 +- src/Form/Field/Redirect.php | 27 +++++++++++++++++++ src/Form/LoginForm.php | 2 +- 6 files changed, 51 insertions(+), 25 deletions(-) create mode 100644 src/Form/Field/Redirect.php diff --git a/public/admin/login.php b/public/admin/login.php index 30ff9e0..959df85 100644 --- a/public/admin/login.php +++ b/public/admin/login.php @@ -17,23 +17,17 @@ $form->handleRequest(); if ($form->isValid()) { $data = $form->getData(); + // the redirect url is validated by the form to match a local file within the web root $redirectUrl = $data['redirect']; - /// @todo we should move this validation to the LoginForm - if (!$router->match($redirectUrl)) { - $form->setError('Tsk tsk tsk'); - $logger = Logger::getInstance('audit'); - $logger->warning("Hacking attempt: login fom submitted with invalid redirect url '$redirectUrl'"); - } else { - unset($data['redirect']); - $authenticator = new UsernamePasswordAuthenticator(); - try { - $authenticator->authenticate(...$data); - header('Location: ' . $redirectUrl, true, 303); - exit(); - } catch (AuthenticationException $e) { - /// @todo should we reduce the level of info shown? Eg. not tell apart unknown user from bad password - $form->setError($e->getMessage()); - } + unset($data['redirect']); + $authenticator = new UsernamePasswordAuthenticator(); + try { + $authenticator->authenticate(...$data); + header('Location: ' . $redirectUrl, true, 303); + exit(); + } catch (AuthenticationException $e) { + /// @todo should we reduce the level of info shown? Eg. not tell apart unknown user from bad password + $form->setError($e->getMessage()); } } } else { diff --git a/resources/templates/admin/login.html.twig b/resources/templates/admin/login.html.twig index 2035049..9e683db 100644 --- a/resources/templates/admin/login.html.twig +++ b/resources/templates/admin/login.html.twig @@ -14,9 +14,6 @@ {{ forms.input(form.getField('password'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }}
{{ forms.input(form.getField('redirect'), {}) }} - {% if not form.isValid() %} -

{{ form.errorMessage }}

- {% endif %}
{{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }}
diff --git a/resources/templates/parts/forms/macros.html.twig b/resources/templates/parts/forms/macros.html.twig index 5fb5da1..005f048 100644 --- a/resources/templates/parts/forms/macros.html.twig +++ b/resources/templates/parts/forms/macros.html.twig @@ -2,30 +2,38 @@ {# @todo allow better styling of the error message: align it below the input field #} {% if field.inputType == 'anticsrf' %} + {% elseif field.inputType == 'datetime-local' %}
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% elseif field.inputType == 'email' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% elseif field.inputType == 'hidden' %} + {% elseif field.inputType == 'submit-button' %}
+ {% elseif field.inputType == 'password' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% elseif field.inputType == 'text' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% elseif field.inputType == 'textarea' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% else %} {# @todo we should really throw a \DomainException... #}
ERROR! unsupported form field type: '{{ field.inputType }}'
diff --git a/src/Form/Field/AntiCSRF.php b/src/Form/Field/AntiCSRF.php index db08223..5ec3334 100644 --- a/src/Form/Field/AntiCSRF.php +++ b/src/Form/Field/AntiCSRF.php @@ -26,7 +26,7 @@ protected function validateValue(mixed $value): null|string { $antiCSRF = new AntiCSRFTokenManager(); try { - $antiCSRF->validateToken($value, $this->formActionUrl); + $antiCSRF->validateToken((string)$value, $this->formActionUrl); } catch (AntiCSRFException $e) { $this->errorMessage = 'The ANTI-CSRF token has been tampered or is missing'; $logger = Logger::getInstance('audit'); diff --git a/src/Form/Field/Redirect.php b/src/Form/Field/Redirect.php new file mode 100644 index 0000000..71fe238 --- /dev/null +++ b/src/Form/Field/Redirect.php @@ -0,0 +1,27 @@ +match($value)) { + $this->errorMessage = 'Tsk tsk tsk. Pen testing redirects?'; + + $logger = Logger::getInstance('audit'); + $logger->info("Hacking attempt? fom submitted with invalid redirect url '$value'"); + } + // We reset the redirect to the previous (current) value - as otherwise the displayed form will keep showing + // a non-acceptable value. Also, pen-testing tools might believe that they achieve some injection of sorts... + return $this->value; + } +} diff --git a/src/Form/LoginForm.php b/src/Form/LoginForm.php index ea6e71d..d704b95 100644 --- a/src/Form/LoginForm.php +++ b/src/Form/LoginForm.php @@ -16,7 +16,7 @@ public function __construct(string $actionUrl, string $redirect) /// @todo get the field length from the Repo fields 'username' => new Field\Text('Username', 'un', [FC::Required => true, FC::MaxLength => 180]), 'password' => new Field\Password('Password', 'pw', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'redirect' => new Field\Hidden('r', [FC::Required => true], $redirect) + 'redirect' => new Field\Redirect('r', [FC::Required => true], $redirect) ]; parent::__construct($actionUrl); From e7781ca7f95d24b26d4c7e4d4f3592079b7342f7 Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 16 Oct 2024 13:51:56 +0000 Subject: [PATCH 10/38] one comment --- public/upload/index.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/public/upload/index.php b/public/upload/index.php index 614827b..74d0d68 100644 --- a/public/upload/index.php +++ b/public/upload/index.php @@ -11,6 +11,8 @@ $form = new CrashReportSubmitForm($router->generate(__FILE__)); // allow to pre-fill form fields using GET request, but only act on POST +/// @todo should we change this behaviour to pre-fill the form on POST requests which just miss the Submit button? +/// It might make sense, as the call-stack field is not a good candidate for being serialized in the URL... if ($form->isSubmitted()) { $form->handleRequest(); if ($form->isValid()) { From 8a186227863a87698ee4faea13e37e4cdf41250e Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 16 Oct 2024 13:53:53 +0000 Subject: [PATCH 11/38] remove debugging data --- src/Security/AntiCSRF.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Security/AntiCSRF.php b/src/Security/AntiCSRF.php index 6dc73da..9e2f989 100644 --- a/src/Security/AntiCSRF.php +++ b/src/Security/AntiCSRF.php @@ -52,7 +52,8 @@ protected function generateToken(?string $lockTo): array 'created' => time(), 'token' => $token, 'lockTo' => $lockTo, - 'uri' => isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : $_SERVER['SCRIPT_NAME'], + // debugging info + //'uri' => isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : $_SERVER['SCRIPT_NAME'], ]]; } From 2bcc140038a526a5198b9c0419b7521069bc481e Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 16 Oct 2024 14:23:12 +0000 Subject: [PATCH 12/38] comments --- src/Security/Firewall.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index 9ffecc2..cc06abb 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -108,8 +108,8 @@ public function logoutUser(bool $force = false): void $session = Session::getInstance(); $session->destroySession(); - /// @todo clean up csrf tokens and remember-me tokens if not stored in the Session - /// @todo send Clear-Site-Data header? (using an event system?) + /// @todo clean up remember-me tokens if not stored in the Session + /// @todo send Clear-Site-Data header? Investigate more the pros and cons... } /** From 752f4fe2e0811026896abead55412cb8d3ddefe5 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 24 Oct 2024 15:20:19 +0000 Subject: [PATCH 13/38] add session limiting, based on redis usage; refactor exceptions hierarchy;do validate minlength constraint limit --- .env | 17 +++++ composer.json | 1 + composer.lock | 7 +- .../templates/parts/forms/macros.html.twig | 3 + src/Exception/AntiCSRFException.php | 2 +- src/Exception/AuthenticationException.php | 2 +- src/Exception/AuthorizationException.php | 2 +- src/Exception/FirewallException.php | 7 -- src/Exception/RateLimitExceedException.php | 8 +++ src/Exception/UserNotAuthorizedException.php | 7 ++ src/Form/CrashReportSubmitForm.php | 6 ++ src/Form/Field.php | 21 +++++- src/Form/Field/RateLimiter.php | 41 +++++++++++ src/Form/FieldConstraint.php | 1 + src/RateLimiter/Constraint/FixedWindow.php | 69 +++++++++++++++++++ src/RateLimiter/ConstraintInterface.php | 11 +++ src/RateLimiter/RateLimiter.php | 28 ++++++++ src/Security/ClientIPAware.php | 62 +++++++++++++++++ src/Security/Firewall.php | 6 +- src/Storage/Redis.php | 28 ++++++++ 20 files changed, 312 insertions(+), 17 deletions(-) delete mode 100644 src/Exception/FirewallException.php create mode 100644 src/Exception/RateLimitExceedException.php create mode 100644 src/Exception/UserNotAuthorizedException.php create mode 100644 src/Form/Field/RateLimiter.php create mode 100644 src/RateLimiter/Constraint/FixedWindow.php create mode 100644 src/RateLimiter/ConstraintInterface.php create mode 100644 src/RateLimiter/RateLimiter.php create mode 100644 src/Security/ClientIPAware.php create mode 100644 src/Storage/Redis.php diff --git a/.env b/.env index 0fba4ad..49d11e8 100644 --- a/.env +++ b/.env @@ -1,9 +1,26 @@ # All the values in this file can be overridden in a file named .env.local # NB: make sure that one is never stored in git + DB_DSN=sqlite:/var/www/VeraCrypt-CrashCollector/var/data/crashcollector.db DB_USER= DB_PASSWORD= +REDIS_HOST=127.0.0.1 +REDIS_PORT=6379 +REDIS_PASSWORD= + +# When empty, the client's IP address will be taken from $_SERVER['REMOTE_ADDR']. +# Set it to a non empty string to have the client IP be extracted from a request HTTP header. +# Supported values: HTTP_CLIENT_IP, HTTP_FASTLY_CLIENT_IP, HTTP_TRUE_CLIENT_IP, HTTP_X_REAL_IP, HTTP_X_FORWARDED_FOR +# NB: HTTP_FASTLY_CLIENT_IP is not reliable by default, you have to set up dedicated vcl code for that, see https://www.fastly.com/documentation/reference/http/http-headers/Fastly-Client-IP/ +# NB: when setting it to a non empty value, TRUSTED_PROXIES has to be set as well (see below for details). +CLIENT_IP_HEADER= +# Csv list of IP addresses of proxies that you trust to set a truthful header identifying the client ip address. +# This means that the first proxy in the truthful chain _has to_ reset the designated http header if it receives it in +# its request. +# When a request comes in from an IP which is not in TRUSTED_PROXIES, $_SERVER['REMOTE_ADDR'] will be used as client IP +TRUSTED_PROXIES= + APP_DEBUG=false # NB: should always have a trailing slash diff --git a/composer.json b/composer.json index d443985..a2acccd 100644 --- a/composer.json +++ b/composer.json @@ -5,6 +5,7 @@ "require": { "php": "^8.1", "ext-pdo": "*", + "ext-redis": "*", "ext-sqlite3": "*", "psr/log": "^3.0.2", "symfony/console": "^6.4.12", diff --git a/composer.lock b/composer.lock index bc16ad9..ded6cd4 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": "ca3b1234b2816b7420d368f80e4b9d91", + "content-hash": "2750a17591ea2553a97475b0a08c4eaa", "packages": [ { "name": "psr/container", @@ -916,14 +916,15 @@ "packages-dev": [], "aliases": [], "minimum-stability": "stable", - "stability-flags": [], + "stability-flags": {}, "prefer-stable": false, "prefer-lowest": false, "platform": { "php": "^8.1", "ext-pdo": "*", + "ext-redis": "*", "ext-sqlite3": "*" }, - "platform-dev": [], + "platform-dev": {}, "plugin-api-version": "2.6.0" } diff --git a/resources/templates/parts/forms/macros.html.twig b/resources/templates/parts/forms/macros.html.twig index 005f048..910ade4 100644 --- a/resources/templates/parts/forms/macros.html.twig +++ b/resources/templates/parts/forms/macros.html.twig @@ -24,6 +24,9 @@
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} + {% elseif field.inputType == 'ratelimiter' %} + {# show nothing #} + {% elseif field.inputType == 'text' %}
diff --git a/src/Exception/AntiCSRFException.php b/src/Exception/AntiCSRFException.php index 217a572..17f3e67 100644 --- a/src/Exception/AntiCSRFException.php +++ b/src/Exception/AntiCSRFException.php @@ -2,6 +2,6 @@ namespace Veracrypt\CrashCollector\Exception; -abstract class AntiCSRFException extends \RuntimeException +abstract class AntiCSRFException extends AuthorizationException { } diff --git a/src/Exception/AuthenticationException.php b/src/Exception/AuthenticationException.php index 6feb768..0902453 100644 --- a/src/Exception/AuthenticationException.php +++ b/src/Exception/AuthenticationException.php @@ -2,6 +2,6 @@ namespace Veracrypt\CrashCollector\Exception; -class AuthenticationException extends FirewallException +abstract class AuthenticationException extends \RuntimeException { } diff --git a/src/Exception/AuthorizationException.php b/src/Exception/AuthorizationException.php index f371b86..1fd397b 100644 --- a/src/Exception/AuthorizationException.php +++ b/src/Exception/AuthorizationException.php @@ -2,6 +2,6 @@ namespace Veracrypt\CrashCollector\Exception; -class AuthorizationException extends FirewallException +class AuthorizationException extends \RuntimeException { } diff --git a/src/Exception/FirewallException.php b/src/Exception/FirewallException.php deleted file mode 100644 index 3562d00..0000000 --- a/src/Exception/FirewallException.php +++ /dev/null @@ -1,7 +0,0 @@ -fields['callStack'] = new Field\TextArea('Call stack', 'cs', [FC::Required => $this->requireAllFieldsByDefault]); + $this->fields['rateLimit'] = new Field\RateLimiter([ + new FixedWindow($actionUrl, 1, 30), // equivalent to once every 30 secs + new FixedWindow($actionUrl, 12, 3600), // equivalent to once every 5 minutes + new FixedWindow($actionUrl, 24, 86400), // equivalent to once every 30 minutes + ]); } } diff --git a/src/Form/Field.php b/src/Form/Field.php index 034db60..d0b1ca6 100644 --- a/src/Form/Field.php +++ b/src/Form/Field.php @@ -3,6 +3,8 @@ namespace Veracrypt\CrashCollector\Form; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; +use Veracrypt\CrashCollector\RateLimiter\ConstraintInterface; +use Veracrypt\CrashCollector\RateLimiter\RateLimiter; /** * @property-read ?string $value @@ -40,8 +42,15 @@ protected function validateConstraintsDefinitions(array $constraints): void case FC::Required: break; case FC::MaxLength: + case FC::MinLength: if ($targetValue < 0) { - throw new \DomainException("Unsupported field maxlength: $targetValue"); + throw new \DomainException("Unsupported field max(/min) length: $targetValue"); + } + break; + case FC::RateLimit: + if (!is_array($targetValue)) { + // the type and number of elements in $targetValue is checked by the RateLimiter itself, later on + throw new \DomainException("Unsupported configuration for rate-limit field: not an array"); } break; default: @@ -108,6 +117,16 @@ protected function validateConstraints(?string $value): bool return false; } break; + case FC::RateLimit: + $limiter = new RateLimiter($targetValue); + try { + $limiter->validateRequest((string)$value); + } catch (\RuntimeException $e) { + /// @todo should we tell apart rate limit hit vs. bad config or connection issues? + $this->errorMessage = $e->getMessage(); + return false; + } + break; // this is checked at constructor time //default: // throw new \DomainException("Unsupported field constraint: '$constraint"); diff --git a/src/Form/Field/RateLimiter.php b/src/Form/Field/RateLimiter.php new file mode 100644 index 0000000..eb2f02e --- /dev/null +++ b/src/Form/Field/RateLimiter.php @@ -0,0 +1,41 @@ +limiter = new Limiter($constraints); + } + + protected function validateValue(mixed $value): null|string + { + try { + $this->limiter->validateRequest(); + } catch (\RuntimeException $e) { + /// @todo should we tell apart rate limit hit vs. bad config or connection issues? + $this->errorMessage = $e->getMessage(); + } + return null; + } +} diff --git a/src/Form/FieldConstraint.php b/src/Form/FieldConstraint.php index cbdff8f..59a3b6e 100644 --- a/src/Form/FieldConstraint.php +++ b/src/Form/FieldConstraint.php @@ -8,4 +8,5 @@ final class FieldConstraint const Required = 1; const MaxLength = 2; const MinLength = 4; + const RateLimit = 8; } diff --git a/src/RateLimiter/Constraint/FixedWindow.php b/src/RateLimiter/Constraint/FixedWindow.php new file mode 100644 index 0000000..ab3c695 --- /dev/null +++ b/src/RateLimiter/Constraint/FixedWindow.php @@ -0,0 +1,69 @@ +connect(); + + $key = $this->getTokenName($extraIdentifier); + if (!self::$rh->exists($key)) { + if (!self::$rh->set($key, 1) || !self::$rh->expire($key, $this->intervalLength)) { + throw new \RuntimeException('Error saving RateLimiter data in Redis'); + } + } else { + $totalCalls = self::$rh->incr($key); + if ($totalCalls === false) { + throw new \RuntimeException('Error getting RateLimiter data from Redis'); + } + if ($totalCalls > $this->maxHitsPerInterval) { + throw new RateLimitExceedException("More than {$this->maxHitsPerInterval} requests were made in {$this->intervalLength} seconds"); + } + } + } + + /// @todo allow using other means than the client IP to group together requests, esp. when $extraIdentifier is not null + protected function getTokenName(?string $extraIdentifier = null): string + { + $clientIP = $this->getClientIP(); + $token = $this->prefix . '|' . $this->identifier . '|' . $this->intervalLength . '|' . $this->maxHitsPerInterval . '|' . $clientIP; + if ($extraIdentifier !== null) { + // Avoid extra-long strings - the worst case scenario for hash key collisions in this case is preventing + // someone else from submitting a form with same value, iff they share the same IP... + $token .= '|' . md5($extraIdentifier); + } + return $token; + } +} diff --git a/src/RateLimiter/ConstraintInterface.php b/src/RateLimiter/ConstraintInterface.php new file mode 100644 index 0000000..5a2da77 --- /dev/null +++ b/src/RateLimiter/ConstraintInterface.php @@ -0,0 +1,11 @@ +constraints as $constraint) { + $constraint->validateRequest($extraIdentifier); + } + } +} diff --git a/src/Security/ClientIPAware.php b/src/Security/ClientIPAware.php new file mode 100644 index 0000000..68983fd --- /dev/null +++ b/src/Security/ClientIPAware.php @@ -0,0 +1,62 @@ +getUser()->getRoles())) { - throw new AuthorizationException("Current user does not have required role " . $role->value); + throw new UserNotAuthorizedException("Current user does not have required role " . $role->value); } } } diff --git a/src/Storage/Redis.php b/src/Storage/Redis.php new file mode 100644 index 0000000..97731c7 --- /dev/null +++ b/src/Storage/Redis.php @@ -0,0 +1,28 @@ +connect($_ENV['REDIS_HOST'], $_ENV['REDIS_PORT']); + if (@$_ENV['REDIS_PASSWORD'] != '') { + self::$rh->auth($_ENV['REDIS_PASSWORD']); + } + } + } +} From 6b5dcb942e04224ef628a00f6c86680c29597283 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 24 Oct 2024 16:44:07 +0000 Subject: [PATCH 14/38] do not show rate limiter error messages if there are platform issues; hide rate limiting details in error messages;make key hash of rate limiter more robust in case of unwary devs --- src/Exception/RateLimitExceedException.php | 1 - src/Form/Field.php | 12 +++++++++--- src/Form/Field/RateLimiter.php | 11 ++++++++--- src/Form/Field/Redirect.php | 2 +- src/RateLimiter/Constraint/FixedWindow.php | 2 +- src/RateLimiter/ConstraintInterface.php | 7 ++++++- src/RateLimiter/RateLimiter.php | 8 +++++++- 7 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/Exception/RateLimitExceedException.php b/src/Exception/RateLimitExceedException.php index aecee6f..db027e2 100644 --- a/src/Exception/RateLimitExceedException.php +++ b/src/Exception/RateLimitExceedException.php @@ -4,5 +4,4 @@ class RateLimitExceedException extends AuthorizationException { - } diff --git a/src/Form/Field.php b/src/Form/Field.php index d0b1ca6..0d1e536 100644 --- a/src/Form/Field.php +++ b/src/Form/Field.php @@ -2,7 +2,9 @@ namespace Veracrypt\CrashCollector\Form; +use Veracrypt\CrashCollector\Exception\RateLimitExceedException; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; +use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\RateLimiter\ConstraintInterface; use Veracrypt\CrashCollector\RateLimiter\RateLimiter; @@ -121,9 +123,13 @@ protected function validateConstraints(?string $value): bool $limiter = new RateLimiter($targetValue); try { $limiter->validateRequest((string)$value); - } catch (\RuntimeException $e) { - /// @todo should we tell apart rate limit hit vs. bad config or connection issues? - $this->errorMessage = $e->getMessage(); + } catch (RateLimitExceedException $e) { + $this->errorMessage = "You have submitted the form too many times. Please wait for a while before re-submitting"; + + /// @todo improve this - add some info on the specific form (and the client IP?) + $logger = Logger::getInstance('audit'); + $logger->info("Form was denied submission - rate limit achieved for field constraint: " . $e->getMessage()); + return false; } break; diff --git a/src/Form/Field/RateLimiter.php b/src/Form/Field/RateLimiter.php index eb2f02e..278512e 100644 --- a/src/Form/Field/RateLimiter.php +++ b/src/Form/Field/RateLimiter.php @@ -2,7 +2,9 @@ namespace Veracrypt\CrashCollector\Form\Field; +use Veracrypt\CrashCollector\Exception\RateLimitExceedException; use Veracrypt\CrashCollector\Form\Field as BaseField; +use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\RateLimiter\ConstraintInterface; use Veracrypt\CrashCollector\RateLimiter\RateLimiter as Limiter; @@ -32,9 +34,12 @@ protected function validateValue(mixed $value): null|string { try { $this->limiter->validateRequest(); - } catch (\RuntimeException $e) { - /// @todo should we tell apart rate limit hit vs. bad config or connection issues? - $this->errorMessage = $e->getMessage(); + } catch (RateLimitExceedException $e) { + $this->errorMessage = "You have submitted the form too many times. Please wait for a while before re-submitting"; + + /// @todo improve this - add some info on the specific form (and the client IP?) + $logger = Logger::getInstance('audit'); + $logger->info("Form was denied submission - rate limit achieved for field: " . $e->getMessage()); } return null; } diff --git a/src/Form/Field/Redirect.php b/src/Form/Field/Redirect.php index 71fe238..5d9bad6 100644 --- a/src/Form/Field/Redirect.php +++ b/src/Form/Field/Redirect.php @@ -18,7 +18,7 @@ protected function validateValue(mixed $value): null|string $this->errorMessage = 'Tsk tsk tsk. Pen testing redirects?'; $logger = Logger::getInstance('audit'); - $logger->info("Hacking attempt? fom submitted with invalid redirect url '$value'"); + $logger->info("Hacking attempt? form submitted with invalid redirect url '$value'"); } // We reset the redirect to the previous (current) value - as otherwise the displayed form will keep showing // a non-acceptable value. Also, pen-testing tools might believe that they achieve some injection of sorts... diff --git a/src/RateLimiter/Constraint/FixedWindow.php b/src/RateLimiter/Constraint/FixedWindow.php index ab3c695..5b30f5c 100644 --- a/src/RateLimiter/Constraint/FixedWindow.php +++ b/src/RateLimiter/Constraint/FixedWindow.php @@ -58,7 +58,7 @@ public function validateRequest(?string $extraIdentifier = null): void protected function getTokenName(?string $extraIdentifier = null): string { $clientIP = $this->getClientIP(); - $token = $this->prefix . '|' . $this->identifier . '|' . $this->intervalLength . '|' . $this->maxHitsPerInterval . '|' . $clientIP; + $token = $this->prefix . '|' . str_replace('|', '||', $this->identifier) . '|' . $this->intervalLength . '|' . $this->maxHitsPerInterval . '|' . $clientIP; if ($extraIdentifier !== null) { // Avoid extra-long strings - the worst case scenario for hash key collisions in this case is preventing // someone else from submitting a form with same value, iff they share the same IP... diff --git a/src/RateLimiter/ConstraintInterface.php b/src/RateLimiter/ConstraintInterface.php index 5a2da77..e491cd7 100644 --- a/src/RateLimiter/ConstraintInterface.php +++ b/src/RateLimiter/ConstraintInterface.php @@ -2,10 +2,15 @@ namespace Veracrypt\CrashCollector\RateLimiter; +use Veracrypt\CrashCollector\Exception\AuthorizationException; +use Veracrypt\CrashCollector\Exception\RateLimitExceedException; + interface ConstraintInterface { /** - * @throws \RuntimeException + * @throws RateLimitExceedException in case of rate limit exceeded + * @throws AuthorizationException for any other auth-related issue + * @throws \RuntimeException for anything else */ public function validateRequest(?string $extraIdentifier = null): void; } diff --git a/src/RateLimiter/RateLimiter.php b/src/RateLimiter/RateLimiter.php index 37c7d7a..a7922f5 100644 --- a/src/RateLimiter/RateLimiter.php +++ b/src/RateLimiter/RateLimiter.php @@ -2,10 +2,14 @@ namespace Veracrypt\CrashCollector\RateLimiter; +use Veracrypt\CrashCollector\Exception\AuthorizationException; +use Veracrypt\CrashCollector\Exception\RateLimitExceedException; + class RateLimiter { /** * @param ConstraintInterface[] $constraints + * @throws \DomainException */ public function __construct( protected array $constraints = [] @@ -17,7 +21,9 @@ public function __construct( } /** - * @throws \RuntimeException + * @throws RateLimitExceedException in case of rate limit exceeded + * @throws AuthorizationException for any other auth-related issue + * @throws \RuntimeException for anything else */ public function validateRequest(?string $extraIdentifier = null): void { From 6c792aee62ee93c651a6a1ad7b95846018234555 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 24 Oct 2024 16:58:33 +0000 Subject: [PATCH 15/38] update readme --- README.md | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 477159a..b1c0957 100644 --- a/README.md +++ b/README.md @@ -2,9 +2,13 @@ ## Overview -**VeraCrypt Crash Collector** is a web application designed to gather and manage crash reports from the VeraCrypt desktop application running on Linux and macOS. This app ensures that users have control over their data, as crash reports are only sent if users explicitly allow it after VeraCrypt detects a crash has occurred. +**VeraCrypt Crash Collector** is a web application designed to gather and manage crash reports from the VeraCrypt desktop +application running on Linux and macOS. +This app ensures that users have control over their data, as crash reports are only sent if users explicitly allow it +after VeraCrypt detects a crash has occurred. -The collected crash reports provide vital information to improve the stability and performance of VeraCrypt by helping to identify and resolve issues. +The collected crash reports provide vital information to improve the stability and performance of VeraCrypt by helping +to identify and resolve issues. ## Crash Reporting Mechanism @@ -20,11 +24,13 @@ When a crash occurs, the following information is gathered by the crash reportin ### Important Note -No personal information is included in the crash reports. The call stack captured is purely technical and does not contain any user data. +No personal information is included in the crash reports. The call stack captured is purely technical and does not contain +any user data. ## Purpose -The goal of VeraCrypt Crash Collector is to streamline the crash report management process and provide a clear path to fixing any technical issues in VeraCrypt. It helps developers identify and resolve bugs by analyzing the crash data collected. +The goal of VeraCrypt Crash Collector is to streamline the crash report management process and provide a clear path to +fixing any technical issues in VeraCrypt. It helps developers identify and resolve bugs by analyzing the crash data collected. ## Contribution @@ -36,7 +42,9 @@ This project is licensed under the [Apache License 2.0](LICENSE). ## Requirements -- PHP 8.1 and up, with the SQLite extension +- PHP 8.1 and up, with the SQLite and PHPRedis extensions +- a webserver configured to run PHP +- a Redis server - Composer, to install the required dependencies ## Installation @@ -44,10 +52,20 @@ This project is licensed under the [Apache License 2.0](LICENSE). 1. run `composer install` at the root of the project 2. check the configuration in `.env`, and, if required, change any value by saving it in a file named `.env.local` 3. make sure that the `var/data` directory is writeable (this is where the app will create its sqlite db by default), - as well as `var/cache/twig` + as well as `var/logs` and `var/cache/twig` 4. create an administrator user: run the cli command `./bin/console user:create --is-superuser ` 5. set up the webserver: - configure the vhost root directory to be the `public` directory. No http access to any other folder please - make sure .php scripts are executed via the php interpreter - no rewrite rules are necessary +6. Navigate to `https://your-host/upload/` to upload crash reports; to `https://your-host/admin/` for browsing them + +## How it works + +Once uploaded, crash reports are stored in a SQLite database. They are not available for examination to the public, but +only to users of this web application. + +The web interface is kept extremely simple by design. Besides supporting the anonymous upload of the crash reports, it +allows application users to browse them and to change their own login password. The only way to manage the application's +users accounts (create, remove, update, enable/disable them) is via a command-line script. From 6104e7075c5bb13b3da61f1f9860fcdb0380f3f1 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 24 Oct 2024 17:13:13 +0000 Subject: [PATCH 16/38] a nitpick --- src/Storage/Redis.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storage/Redis.php b/src/Storage/Redis.php index 97731c7..b7efb91 100644 --- a/src/Storage/Redis.php +++ b/src/Storage/Redis.php @@ -19,7 +19,7 @@ protected function connect(): void { if (self::$rh === null) { self::$rh = new RedisServer(); - self::$rh->connect($_ENV['REDIS_HOST'], $_ENV['REDIS_PORT']); + self::$rh->connect($_ENV['REDIS_HOST'], (int)$_ENV['REDIS_PORT']); if (@$_ENV['REDIS_PASSWORD'] != '') { self::$rh->auth($_ENV['REDIS_PASSWORD']); } From 346cab0b243fdcc9c89a8d09cfa537b4d6e6dd0d Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 25 Oct 2024 13:12:59 +0000 Subject: [PATCH 17/38] reset rate-limit of login form on succesful login; refactor names and namespaces of various classes --- public/admin/index.php | 2 +- public/admin/login.php | 1 + public/admin/resetpassword.php | 2 +- src/Entity/User.php | 1 + src/Form/Field.php | 12 +++---- src/Form/Field/RateLimiter.php | 4 +-- src/Form/LoginForm.php | 22 ++++++++++-- src/RateLimiter/Constraint/FixedWindow.php | 34 +++++++++++-------- .../Constraint/RedisConstraint.php | 12 +++++++ src/RateLimiter/RateLimiter.php | 13 +++++-- ...Interface.php => RateLimiterInterface.php} | 4 ++- src/Repository/CrashReportRepository.php | 2 +- ...{Repository.php => DatabaseRepository.php} | 2 +- src/Repository/UserRepository.php | 2 +- src/Security/AnonymousUser.php | 2 -- src/Security/Firewall.php | 1 - src/Security/UserInterface.php | 2 -- src/{Entity => Security}/UserRole.php | 2 +- 18 files changed, 80 insertions(+), 40 deletions(-) create mode 100644 src/RateLimiter/Constraint/RedisConstraint.php rename src/RateLimiter/{ConstraintInterface.php => RateLimiterInterface.php} (82%) rename src/Repository/{Repository.php => DatabaseRepository.php} (98%) rename src/{Entity => Security}/UserRole.php (71%) diff --git a/public/admin/index.php b/public/admin/index.php index 9915166..dec19df 100644 --- a/public/admin/index.php +++ b/public/admin/index.php @@ -2,12 +2,12 @@ require_once(__DIR__ . '/../../autoload.php'); -use Veracrypt\CrashCollector\Entity\UserRole; use Veracrypt\CrashCollector\Exception\AuthorizationException; use Veracrypt\CrashCollector\Form\CrashReportSearchForm; use Veracrypt\CrashCollector\Repository\CrashReportRepository; use Veracrypt\CrashCollector\Router; use Veracrypt\CrashCollector\Security\Firewall; +use Veracrypt\CrashCollector\Security\UserRole; use Veracrypt\CrashCollector\Templating; /// @todo get from .env? diff --git a/public/admin/login.php b/public/admin/login.php index 959df85..a9ed2d9 100644 --- a/public/admin/login.php +++ b/public/admin/login.php @@ -24,6 +24,7 @@ try { $authenticator->authenticate(...$data); header('Location: ' . $redirectUrl, true, 303); + $form->onSuccessfulLogin(); exit(); } catch (AuthenticationException $e) { /// @todo should we reduce the level of info shown? Eg. not tell apart unknown user from bad password diff --git a/public/admin/resetpassword.php b/public/admin/resetpassword.php index 0db8cd7..6dc9b05 100644 --- a/public/admin/resetpassword.php +++ b/public/admin/resetpassword.php @@ -2,13 +2,13 @@ require_once(__DIR__ . '/../../autoload.php'); -use Veracrypt\CrashCollector\Entity\UserRole; use Veracrypt\CrashCollector\Exception\AuthorizationException; use Veracrypt\CrashCollector\Form\ResetPasswordForm; use Veracrypt\CrashCollector\Repository\UserRepository; use Veracrypt\CrashCollector\Router; use Veracrypt\CrashCollector\Security\Firewall; use Veracrypt\CrashCollector\Security\PasswordHasher; +use Veracrypt\CrashCollector\Security\UserRole; use Veracrypt\CrashCollector\Templating; $firewall = Firewall::getInstance(); diff --git a/src/Entity/User.php b/src/Entity/User.php index d282724..d56c54d 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -5,6 +5,7 @@ use DateTimeImmutable; use DateTimeInterface; use Veracrypt\CrashCollector\Security\UserInterface; +use Veracrypt\CrashCollector\Security\UserRole; /** * @property-read int $dateJoined diff --git a/src/Form/Field.php b/src/Form/Field.php index 0d1e536..fc57db1 100644 --- a/src/Form/Field.php +++ b/src/Form/Field.php @@ -5,13 +5,14 @@ use Veracrypt\CrashCollector\Exception\RateLimitExceedException; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; use Veracrypt\CrashCollector\Logger; -use Veracrypt\CrashCollector\RateLimiter\ConstraintInterface; -use Veracrypt\CrashCollector\RateLimiter\RateLimiter; +use Veracrypt\CrashCollector\RateLimiter\RateLimiterInterface; /** * @property-read ?string $value * @property-read ?string $errorMessage * @property-read bool isValid + * + * @todo introduce a FieldInterface */ abstract class Field { @@ -50,9 +51,9 @@ protected function validateConstraintsDefinitions(array $constraints): void } break; case FC::RateLimit: - if (!is_array($targetValue)) { + if (!($targetValue instanceof RateLimiterInterface)) { // the type and number of elements in $targetValue is checked by the RateLimiter itself, later on - throw new \DomainException("Unsupported configuration for rate-limit field: not an array"); + throw new \DomainException("Unsupported configuration for rate-limit field: not a rate limiter object"); } break; default: @@ -120,9 +121,8 @@ protected function validateConstraints(?string $value): bool } break; case FC::RateLimit: - $limiter = new RateLimiter($targetValue); try { - $limiter->validateRequest((string)$value); + $targetValue->validateRequest((string)$value); } catch (RateLimitExceedException $e) { $this->errorMessage = "You have submitted the form too many times. Please wait for a while before re-submitting"; diff --git a/src/Form/Field/RateLimiter.php b/src/Form/Field/RateLimiter.php index 278512e..285ddeb 100644 --- a/src/Form/Field/RateLimiter.php +++ b/src/Form/Field/RateLimiter.php @@ -5,7 +5,7 @@ use Veracrypt\CrashCollector\Exception\RateLimitExceedException; use Veracrypt\CrashCollector\Form\Field as BaseField; use Veracrypt\CrashCollector\Logger; -use Veracrypt\CrashCollector\RateLimiter\ConstraintInterface; +use Veracrypt\CrashCollector\RateLimiter\RateLimiterInterface; use Veracrypt\CrashCollector\RateLimiter\RateLimiter as Limiter; /** @@ -19,7 +19,7 @@ class RateLimiter extends BaseField protected Limiter $limiter; /** - * @var ConstraintInterface[] $constraints + * @var RateLimiterInterface[] $constraints */ public function __construct( array $constraints diff --git a/src/Form/LoginForm.php b/src/Form/LoginForm.php index d704b95..cb42840 100644 --- a/src/Form/LoginForm.php +++ b/src/Form/LoginForm.php @@ -4,21 +4,39 @@ use Veracrypt\CrashCollector\Form\FieldConstraint as FC; use Veracrypt\CrashCollector\Form\Form as BaseForm; +use Veracrypt\CrashCollector\RateLimiter\Constraint\FixedWindow; +use Veracrypt\CrashCollector\RateLimiter\RateLimiter; use Veracrypt\CrashCollector\Security\PasswordHasher; class LoginForm extends BaseForm { protected string $submitLabel = 'Login'; + protected RateLimiter $rateLimiter; public function __construct(string $actionUrl, string $redirect) { + $this->rateLimiter = new RateLimiter([ + new FixedWindow($actionUrl, 5, 10), // equivalent to once every 2 secs + new FixedWindow($actionUrl, 80, 3600), // equivalent to once every 45 secs + new FixedWindow($actionUrl, 288, 86400), // equivalent to once every 5 minutes + ]); + $this->fields = [ /// @todo get the field length from the Repo fields - 'username' => new Field\Text('Username', 'un', [FC::Required => true, FC::MaxLength => 180]), + 'username' => new Field\Text('Username', 'un', [ + FC::Required => true, + FC::MaxLength => 180, + FC::RateLimit => $this->rateLimiter, + ]), 'password' => new Field\Password('Password', 'pw', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'redirect' => new Field\Redirect('r', [FC::Required => true], $redirect) + 'redirect' => new Field\Redirect('r', [FC::Required => true], $redirect), ]; parent::__construct($actionUrl); } + + public function onSuccessfulLogin() + { + $this->rateLimiter->reset($this->getField('username')->getData()); + } } diff --git a/src/RateLimiter/Constraint/FixedWindow.php b/src/RateLimiter/Constraint/FixedWindow.php index 5b30f5c..ee20e63 100644 --- a/src/RateLimiter/Constraint/FixedWindow.php +++ b/src/RateLimiter/Constraint/FixedWindow.php @@ -4,16 +4,14 @@ use Veracrypt\CrashCollector\Exception\AuthorizationException; use Veracrypt\CrashCollector\Exception\RateLimitExceedException; -use Veracrypt\CrashCollector\RateLimiter\ConstraintInterface; +use Veracrypt\CrashCollector\RateLimiter\RateLimiterInterface; use Veracrypt\CrashCollector\Security\ClientIPAware; -use Veracrypt\CrashCollector\Storage\Redis; -class FixedWindow implements ConstraintInterface +class FixedWindow extends RedisConstraint implements RateLimiterInterface { - use Redis; use ClientIPAware; - protected string $prefix = 'RL_FW_'; + protected $postfix = 'FW'; /** * @param int $intervalLength in seconds @@ -38,7 +36,7 @@ public function validateRequest(?string $extraIdentifier = null): void { $this->connect(); - $key = $this->getTokenName($extraIdentifier); + $key = $this->getKey($extraIdentifier); if (!self::$rh->exists($key)) { if (!self::$rh->set($key, 1) || !self::$rh->expire($key, $this->intervalLength)) { throw new \RuntimeException('Error saving RateLimiter data in Redis'); @@ -54,16 +52,22 @@ public function validateRequest(?string $extraIdentifier = null): void } } + /** + * @throws \RedisException + */ + public function reset(?string $extraIdentifier = null): void + { + self::$rh->unlink($this->getKey()); + } + /// @todo allow using other means than the client IP to group together requests, esp. when $extraIdentifier is not null - protected function getTokenName(?string $extraIdentifier = null): string + protected function getKey(?string $extraIdentifier = null): string { - $clientIP = $this->getClientIP(); - $token = $this->prefix . '|' . str_replace('|', '||', $this->identifier) . '|' . $this->intervalLength . '|' . $this->maxHitsPerInterval . '|' . $clientIP; - if ($extraIdentifier !== null) { - // Avoid extra-long strings - the worst case scenario for hash key collisions in this case is preventing - // someone else from submitting a form with same value, iff they share the same IP... - $token .= '|' . md5($extraIdentifier); - } - return $token; + // the order of fields is chosen to allow wildcard purging of all constraints matching a given identifier, + // regardless of constraint type / config + return $this->prefix . '|' . str_replace('|', '||', $this->identifier) . '|' . + ($extraIdentifier !== null ? md5($extraIdentifier) : '') . '|' . + $this->getClientIP() . '|' . + $this->intervalLength . '|' . $this->maxHitsPerInterval . '|' . $this->postfix; } } diff --git a/src/RateLimiter/Constraint/RedisConstraint.php b/src/RateLimiter/Constraint/RedisConstraint.php new file mode 100644 index 0000000..cb91f5a --- /dev/null +++ b/src/RateLimiter/Constraint/RedisConstraint.php @@ -0,0 +1,12 @@ +validateRequest($extraIdentifier); } } + + public function reset(?string $extraIdentifier = null): void + { + foreach ($this->constraints as $constraint) { + $constraint->reset($extraIdentifier); + } + } } diff --git a/src/RateLimiter/ConstraintInterface.php b/src/RateLimiter/RateLimiterInterface.php similarity index 82% rename from src/RateLimiter/ConstraintInterface.php rename to src/RateLimiter/RateLimiterInterface.php index e491cd7..1da3e17 100644 --- a/src/RateLimiter/ConstraintInterface.php +++ b/src/RateLimiter/RateLimiterInterface.php @@ -5,7 +5,7 @@ use Veracrypt\CrashCollector\Exception\AuthorizationException; use Veracrypt\CrashCollector\Exception\RateLimitExceedException; -interface ConstraintInterface +interface RateLimiterInterface { /** * @throws RateLimitExceedException in case of rate limit exceeded @@ -13,4 +13,6 @@ interface ConstraintInterface * @throws \RuntimeException for anything else */ public function validateRequest(?string $extraIdentifier = null): void; + + public function reset(?string $extraIdentifier = null): void; } diff --git a/src/Repository/CrashReportRepository.php b/src/Repository/CrashReportRepository.php index a187989..ab19ad7 100644 --- a/src/Repository/CrashReportRepository.php +++ b/src/Repository/CrashReportRepository.php @@ -6,7 +6,7 @@ use Veracrypt\CrashCollector\Entity\CrashReport; use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; -class CrashReportRepository extends Repository +class CrashReportRepository extends DatabaseRepository { protected string $tableName = 'crash_report'; diff --git a/src/Repository/Repository.php b/src/Repository/DatabaseRepository.php similarity index 98% rename from src/Repository/Repository.php rename to src/Repository/DatabaseRepository.php index 8d38cf4..673fa6b 100644 --- a/src/Repository/Repository.php +++ b/src/Repository/DatabaseRepository.php @@ -4,7 +4,7 @@ use Veracrypt\CrashCollector\Storage\DatabaseTable; -abstract class Repository +abstract class DatabaseRepository { use DatabaseTable; diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index c84a36c..74994df 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -6,7 +6,7 @@ use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; -class UserRepository extends Repository +class UserRepository extends DatabaseRepository { protected string $tableName = 'auth_user'; protected Logger $logger; diff --git a/src/Security/AnonymousUser.php b/src/Security/AnonymousUser.php index 6181847..a39d7b6 100644 --- a/src/Security/AnonymousUser.php +++ b/src/Security/AnonymousUser.php @@ -2,8 +2,6 @@ namespace Veracrypt\CrashCollector\Security; -use Veracrypt\CrashCollector\Entity\UserRole; - class AnonymousUser implements UserInterface { public function getRoles(): array diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index 894d33f..95cb524 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -3,7 +3,6 @@ namespace Veracrypt\CrashCollector\Security; use Veracrypt\CrashCollector\Entity\User; -use Veracrypt\CrashCollector\Entity\UserRole; use Veracrypt\CrashCollector\Exception\UserNotAuthorizedException; use Veracrypt\CrashCollector\Exception\UserNotFoundException; use Veracrypt\CrashCollector\Form\LoginForm; diff --git a/src/Security/UserInterface.php b/src/Security/UserInterface.php index ed09c54..a82096e 100644 --- a/src/Security/UserInterface.php +++ b/src/Security/UserInterface.php @@ -2,8 +2,6 @@ namespace Veracrypt\CrashCollector\Security; -use Veracrypt\CrashCollector\Entity\UserRole; - interface UserInterface { /** diff --git a/src/Entity/UserRole.php b/src/Security/UserRole.php similarity index 71% rename from src/Entity/UserRole.php rename to src/Security/UserRole.php index 12c676c..f271de8 100644 --- a/src/Entity/UserRole.php +++ b/src/Security/UserRole.php @@ -1,6 +1,6 @@ Date: Sat, 26 Oct 2024 12:35:15 +0000 Subject: [PATCH 18/38] fix updating users passwords via cli --- bin/console | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/console b/bin/console index a5904bb..bd5e531 100755 --- a/bin/console +++ b/bin/console @@ -123,12 +123,15 @@ $application->register('user:update') ->addOption('is-superuser', null, InputOption::VALUE_REQUIRED, 'Use a 1/0 value to enable/disable') ->addOption('is-inactive', null, InputOption::VALUE_REQUIRED, 'Use a 1/0 value to enable/disable') ->setCode(function (InputInterface $input, OutputInterface $output): int { + $ph = new PasswordHasher(); $ur = new UserRepository(); $isSuperuser = $input->getOption('is-superuser') !== null ? (bool)$input->getOption('is-superuser') : null; $isActive = $input->getOption('is-inactive') !== null ? !$input->getOption('is-inactive') : null; + /// @todo move the hash creation to the Repository? + $passwordHash = $input->getOption('password') !== null ? $ph->hash($input->getOption('password')) : null; if (!$ur->updateUser( $input->getArgument('username'), - $input->getOption('password'), + $passwordHash, $input->getOption('email'), $input->getOption('first-name'), $input->getOption('last-name'), From 6cbb543c9bac7a7c878513ba7ec9dc9dbd0fc5ba Mon Sep 17 00:00:00 2001 From: gggeek Date: Mon, 28 Oct 2024 08:03:49 +0000 Subject: [PATCH 19/38] add user::isAuthenticated method --- public/admin/login.php | 1 - src/Entity/User.php | 5 +++++ src/Security/AnonymousUser.php | 5 +++++ src/Security/Firewall.php | 4 ++-- src/Security/UserInterface.php | 8 ++++++++ src/Security/UsernamePasswordAuthenticator.php | 7 ++++--- 6 files changed, 24 insertions(+), 6 deletions(-) diff --git a/public/admin/login.php b/public/admin/login.php index a9ed2d9..4e55058 100644 --- a/public/admin/login.php +++ b/public/admin/login.php @@ -5,7 +5,6 @@ use Veracrypt\CrashCollector\Exception\AuthenticationException; use Veracrypt\CrashCollector\Security\Firewall; use Veracrypt\CrashCollector\Form\LoginForm; -use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Router; use Veracrypt\CrashCollector\Security\UsernamePasswordAuthenticator; use Veracrypt\CrashCollector\Templating; diff --git a/src/Entity/User.php b/src/Entity/User.php index d56c54d..0b055a2 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -57,6 +57,11 @@ public function getUserIdentifier(): string return $this->username; } + public function isAuthenticated(): bool + { + return true; + } + public function isActive(): bool { return $this->isActive; diff --git a/src/Security/AnonymousUser.php b/src/Security/AnonymousUser.php index a39d7b6..2091baf 100644 --- a/src/Security/AnonymousUser.php +++ b/src/Security/AnonymousUser.php @@ -14,6 +14,11 @@ public function getUserIdentifier(): string return 'anonymous'; } + public function isAuthenticated(): bool + { + return false; + } + public function isActive(): bool { return false; diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index 95cb524..481e908 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -67,8 +67,8 @@ public function getUser(): UserInterface */ public function loginUser(UserInterface $user): void { - if ($user instanceof AnonymousUser) { - throw new \DomainException('Anonymous user can not be used for logging in'); + if (!$user->isAuthenticated()) { + throw new \DomainException('Non-authenticated user ' . $user->getUserIdentifier() . ' can not be used for logging in'); } if ($user === $this->user) { diff --git a/src/Security/UserInterface.php b/src/Security/UserInterface.php index a82096e..827a4fb 100644 --- a/src/Security/UserInterface.php +++ b/src/Security/UserInterface.php @@ -14,5 +14,13 @@ public function getRoles(): array; */ public function getUserIdentifier(): string; + /** + * Disabled users are not allowed to log in + */ public function isActive(): bool; + + /** + * Returns false for Anonymous users + */ + public function isAuthenticated(): bool; } diff --git a/src/Security/UsernamePasswordAuthenticator.php b/src/Security/UsernamePasswordAuthenticator.php index a8e5dba..3d8b329 100644 --- a/src/Security/UsernamePasswordAuthenticator.php +++ b/src/Security/UsernamePasswordAuthenticator.php @@ -2,6 +2,7 @@ namespace Veracrypt\CrashCollector\Security; +use Psr\Log\LoggerInterface; use Veracrypt\CrashCollector\Exception\AccountExpiredException; use Veracrypt\CrashCollector\Exception\AuthenticationException; use Veracrypt\CrashCollector\Exception\BadCredentialsException; @@ -11,11 +12,11 @@ class UsernamePasswordAuthenticator { /** @var UserProvider $userProvider */ - protected $userProvider; + protected UserProvider $userProvider; /** @var PasswordHasher $passwordHasher */ - protected $passwordHasher; + protected PasswordHasher $passwordHasher; /** @var Logger $logger */ - protected $logger; + protected LoggerInterface $logger; public function __construct() { From bfbfaa0d58c699b42f0925596dba872eb39bcf5f Mon Sep 17 00:00:00 2001 From: gggeek Date: Mon, 28 Oct 2024 08:17:37 +0000 Subject: [PATCH 20/38] remove existing anticsrf tokens from the session if switching users --- src/Security/AntiCSRF.php | 7 +++++++ src/Security/Firewall.php | 3 +++ src/Security/Session.php | 1 - 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Security/AntiCSRF.php b/src/Security/AntiCSRF.php index 9e2f989..9b1d726 100644 --- a/src/Security/AntiCSRF.php +++ b/src/Security/AntiCSRF.php @@ -110,4 +110,11 @@ public function validateToken(string $token, ?string $lockTo = ''): void throw new TokenMatchesWrongFormException('Anti-CSRF token used on the wrong form'); } } + + public function purgeTokens(): void + { + // remove all csrf tokens from the session + $session = Session::getInstance(); + $session->set($this->storageKey, []); + } } diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index 481e908..aa7b003 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -81,6 +81,9 @@ public function loginUser(UserInterface $user): void $previousUserIdentifier = $session->get($this->storageKey); if ($user->getUserIdentifier() !== $previousUserIdentifier) { $session->regenerate(); + + $antiCSRF = new AntiCSRF(); + $antiCSRF->purgeTokens(); } $session->set($this->storageKey, $user->getUserIdentifier()); /// @todo here we could add $session->commit() - or use autocommitting sessions diff --git a/src/Security/Session.php b/src/Security/Session.php index 02a071c..8748a10 100644 --- a/src/Security/Session.php +++ b/src/Security/Session.php @@ -42,7 +42,6 @@ protected function __construct() public function regenerate(): void { - /// @todo check: if there is already a session, should we remove existing csrf tokens from the storage? /// @todo Should we delete the previous session data, passing in $true, or keep the old session around? See example 2 at /// https://www.php.net/manual/en/function.session-regenerate-id.php#refsect1-function.session-regenerate-id-examples /// for the recommended way to generate new session ids while avoiding issues with unstable networks and From 7125af4941f8479738c9f4b3234d59efabb0337c Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 29 Oct 2024 19:31:07 +0000 Subject: [PATCH 21/38] fix bug when checking for multiple roles; do not always autostart session when creating the firewall --- src/Security/Firewall.php | 19 ++++++++++++++++--- src/Security/Session.php | 12 +++++++++++- src/Storage/Session.php | 1 + 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index aa7b003..440b6ae 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -26,7 +26,10 @@ protected function __construct() } } - protected function supportsRequest() + /** + * @todo implement some logic when/if we'll add support for non-session auth such as eg. basic auth, etc... + */ + protected function supportsRequest(): bool { return true; } @@ -34,6 +37,12 @@ protected function supportsRequest() protected function authenticate(): void { $session = Session::getInstance(); + // we avoid calling session_start (triggered by `get`) unless there is a session cookie present, in order to make it + // possible to have the fw run on pages which work both for non and authenticated users without creating sessions + // all the time + if (!$session->isStarted() && !$session->cookieIsPresent()) { + return; + } $username = $session->get($this->storageKey); if ($username !== null) { // we always refresh the user details from the repo, to check if his/her roles or active status have changed @@ -120,8 +129,12 @@ public function logoutUser(bool $force = false): void */ public function require(UserRole|array $roles): void { - foreach(array($roles) as $role) { - if (!in_array($role, $this->getUser()->getRoles())) { + $userRoles = $this->getUser()->getRoles(); + if (!is_array($roles)) { + $roles = array($roles); + } + foreach($roles as $role) { + if (!in_array($role, $userRoles)) { throw new UserNotAuthorizedException("Current user does not have required role " . $role->value); } } diff --git a/src/Security/Session.php b/src/Security/Session.php index 8748a10..0265f28 100644 --- a/src/Security/Session.php +++ b/src/Security/Session.php @@ -40,6 +40,16 @@ protected function __construct() ]; } + public function isStarted(): bool + { + return $this->sessionStarted; + } + + public function cookieIsPresent(): bool + { + return array_key_exists(session_name(), $_COOKIE); + } + public function regenerate(): void { /// @todo Should we delete the previous session data, passing in $true, or keep the old session around? See example 2 at @@ -53,7 +63,7 @@ public function destroySession(): void { if (\PHP_SESSION_ACTIVE === session_status()) { $_SESSION = []; - /// @todo check php source code: does it make sense to call session_write_close() here, or does session_destroy do that already? + // no need to call session_write_close() here, as or does session_destroy triggers that already session_destroy(); } diff --git a/src/Storage/Session.php b/src/Storage/Session.php index 5ca05c7..48064cf 100644 --- a/src/Storage/Session.php +++ b/src/Storage/Session.php @@ -26,6 +26,7 @@ public function set(string|int $key, mixed $value): void } /** + * NB: this will cause the session to start if it was not started already! * @todo we could cache the whole of $_SESSION in memory so that we can avoid further calls to session_start on later * calls to get (and either add a $forceRefresh argument, or a separate `refresh` method). This is esp. * useful when in autocommit mode From b6099756ddcd88dddec78556dcb74df49a2df668 Mon Sep 17 00:00:00 2001 From: gggeek Date: Tue, 29 Oct 2024 19:47:32 +0000 Subject: [PATCH 22/38] nitpicks: remove one unused use statement; change order of comparison clauses --- bin/console | 1 - src/Security/Firewall.php | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/bin/console b/bin/console index bd5e531..dca106f 100755 --- a/bin/console +++ b/bin/console @@ -14,7 +14,6 @@ use Symfony\Component\Console\Output\ConsoleOutputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Question\Question; use Symfony\Component\Console\Style\SymfonyStyle; -use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Repository\UserRepository; use Veracrypt\CrashCollector\Security\PasswordHasher; diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index 440b6ae..f607e14 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -40,7 +40,7 @@ protected function authenticate(): void // we avoid calling session_start (triggered by `get`) unless there is a session cookie present, in order to make it // possible to have the fw run on pages which work both for non and authenticated users without creating sessions // all the time - if (!$session->isStarted() && !$session->cookieIsPresent()) { + if (!$session->cookieIsPresent() && !$session->isStarted()) { return; } $username = $session->get($this->storageKey); From b189b8e90009f60db2db15422b9296498740cb9d Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 7 Nov 2024 13:21:00 +0000 Subject: [PATCH 23/38] implement forgot-password functionality for non-logged-in users; a little related refactoring --- .env | 6 ++ README.md | 2 +- bin/console | 9 +++ public/admin/forgotpassword.php | 58 ++++++++++++++++ public/admin/resetpassword.php | 2 +- public/admin/setnewpassword.php | 68 +++++++++++++++++++ .../templates/admin/forgotpassword.html.twig | 21 ++++++ resources/templates/admin/login.html.twig | 1 + .../templates/admin/setnewpassword.html.twig | 29 ++++++++ .../templates/emails/forgotpassword.txt.twig | 7 ++ src/Entity/ForgotPasswordToken.php | 7 ++ src/Entity/Token.php | 62 +++++++++++++++++ src/Entity/UserToken.php | 26 +++++++ src/Exception/ConstraintException.php | 7 ++ .../ConstraintUserInactiveException.php | 7 ++ .../ConstraintUserNotFoundException.php | 7 ++ .../FormFieldNotSubmittedException.php | 7 ++ src/Exception/TokenNotFoundException.php | 7 ++ src/Form/Field.php | 28 +++++--- .../Constraint/ActiveUserEmailConstraint.php | 26 +++++++ .../Constraint/ActiveUserTokenConstraint.php | 67 ++++++++++++++++++ .../Field/Constraint/ConstraintInterface.php | 13 ++++ .../Field/Constraint/UserConstraintTrait.php | 15 ++++ src/Form/Field/RateLimiter.php | 3 + src/Form/FieldConstraint.php | 1 + src/Form/ForgotPasswordEmailForm.php | 59 ++++++++++++++++ src/Form/ForgotPasswordForm.php | 58 ++++++++++++++++ src/Form/Form.php | 35 +++++++--- src/Form/LoginForm.php | 2 +- src/Form/PasswordUpdateBaseForm.php | 34 ++++++++++ src/Form/ResetPasswordForm.php | 27 ++++---- src/Form/SetNewPasswordForm.php | 66 ++++++++++++++++++ src/Mailer/Email.php | 55 +++++++++++++++ src/Mailer/Mailer.php | 25 +++++++ src/Repository/DatabaseRepository.php | 25 ++++++- .../ForgotPasswordTokenRepository.php | 27 ++++++++ src/Repository/TokenRepository.php | 67 ++++++++++++++++++ src/Repository/UserRepository.php | 15 ++++ src/Repository/UserTokenRepository.php | 33 +++++++++ src/Security/Firewall.php | 1 + src/Security/PasswordHasher.php | 5 ++ 41 files changed, 984 insertions(+), 36 deletions(-) create mode 100644 public/admin/forgotpassword.php create mode 100644 public/admin/setnewpassword.php create mode 100644 resources/templates/admin/forgotpassword.html.twig create mode 100644 resources/templates/admin/setnewpassword.html.twig create mode 100644 resources/templates/emails/forgotpassword.txt.twig create mode 100644 src/Entity/ForgotPasswordToken.php create mode 100644 src/Entity/Token.php create mode 100644 src/Entity/UserToken.php create mode 100644 src/Exception/ConstraintException.php create mode 100644 src/Exception/ConstraintUserInactiveException.php create mode 100644 src/Exception/ConstraintUserNotFoundException.php create mode 100644 src/Exception/FormFieldNotSubmittedException.php create mode 100644 src/Exception/TokenNotFoundException.php create mode 100644 src/Form/Field/Constraint/ActiveUserEmailConstraint.php create mode 100644 src/Form/Field/Constraint/ActiveUserTokenConstraint.php create mode 100644 src/Form/Field/Constraint/ConstraintInterface.php create mode 100644 src/Form/Field/Constraint/UserConstraintTrait.php create mode 100644 src/Form/ForgotPasswordEmailForm.php create mode 100644 src/Form/ForgotPasswordForm.php create mode 100644 src/Form/PasswordUpdateBaseForm.php create mode 100644 src/Form/SetNewPasswordForm.php create mode 100644 src/Mailer/Email.php create mode 100644 src/Mailer/Mailer.php create mode 100644 src/Repository/ForgotPasswordTokenRepository.php create mode 100644 src/Repository/TokenRepository.php create mode 100644 src/Repository/UserTokenRepository.php diff --git a/.env b/.env index 49d11e8..0e0f4b1 100644 --- a/.env +++ b/.env @@ -26,6 +26,12 @@ APP_DEBUG=false # NB: should always have a trailing slash ROOT_URL=/ +# Used for links when sending password-reset emails +WEBSITE=https://crashcollector.veracrypt.fr + +# Used when sending password-reset emails +MAIL_FROM=crashcollector@veracrypt.fr + LOG_DIR=/var/www/VeraCrypt-CrashCollector/var/logs # The audit log traces user events such as login, password changes, etc AUDIT_LOG_FILE=audit.log diff --git a/README.md b/README.md index b1c0957..9369b3d 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ This project is licensed under the [Apache License 2.0](LICENSE). ## Requirements -- PHP 8.1 and up, with the SQLite and PHPRedis extensions +- PHP 8.1 and up, with the SQLite and PHPRedis extensions (using SQLite Library 3.35.4 or later) - a webserver configured to run PHP - a Redis server - Composer, to install the required dependencies diff --git a/bin/console b/bin/console index dca106f..71f2613 100755 --- a/bin/console +++ b/bin/console @@ -14,6 +14,7 @@ use Symfony\Component\Console\Output\ConsoleOutputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Question\Question; use Symfony\Component\Console\Style\SymfonyStyle; +use Veracrypt\CrashCollector\Repository\ForgotPasswordTokenRepository; use Veracrypt\CrashCollector\Repository\UserRepository; use Veracrypt\CrashCollector\Security\PasswordHasher; @@ -144,4 +145,12 @@ $application->register('user:update') return Command::SUCCESS; }); +$application->register('forgotpasswordtoken:prune') + ->setCode(function (InputInterface $input, OutputInterface $output): int { + $repo = new ForgotPasswordTokenRepository(); + $repo->prune(); + $output->writeln("Expired forgotpassword tokens deleted"); + return Command::SUCCESS; + }); + $application->run(); diff --git a/public/admin/forgotpassword.php b/public/admin/forgotpassword.php new file mode 100644 index 0000000..e2c6fb1 --- /dev/null +++ b/public/admin/forgotpassword.php @@ -0,0 +1,58 @@ +getUser(); +if ($user->isAuthenticated()) { + header('Location: ' . $router->generate(__DIR__ . '/resetpassword.php'), true, 303); +} + +$form = new ForgotPasswordForm($router->generate(__FILE__)); + +if ($form->isSubmitted()) { + $form->handleRequest(); + if ($form->isValid()) { + + /** @var User $user */ + $user = $form->getUser(); + + $ph = new PasswordHasher(); + $fptRepo = new ForgotPasswordTokenRepository(); + $secret = $ph->generateRandomString($fptRepo->tokenLength); + $token = $fptRepo->createToken($user->username, $ph->hash($secret)); + /// @todo use mime multipart to add an html version besides plain text + $mailer = new Mailer(); + $email = new Email(); + $form2 = new ForgotPasswordEmailForm(__DIR__ . '/setnewpassword.php', $token->id, $secret); + $text = $tpl->render('emails/forgotpassword.txt.twig', [ + 'link' => $_ENV['WEBSITE'] . $router->generate(__DIR__ . '/setnewpassword.php', $form2->getQueryStringParts()) + ]); + $email->from($_ENV['MAIL_FROM'])->to($user->email)->subject("VeraCrypt Crash Collector password reset link")->text($text); + try { + $mailer->send($email); + } catch (\RuntimeException $e) { + $form->setError('There was an error sending the email, please retry later.'); + } + } +} + +echo $tpl->render('admin/forgotpassword.html.twig', [ + 'form' => $form, + 'urls' => $firewall->getAdminUrls(), +]); diff --git a/public/admin/resetpassword.php b/public/admin/resetpassword.php index 6dc9b05..c10b083 100644 --- a/public/admin/resetpassword.php +++ b/public/admin/resetpassword.php @@ -29,7 +29,7 @@ if ($form->isValid()) { $passwordHasher = new PasswordHasher(); $repository = new UserRepository(); - $repository->updateUser($user->getUserIdentifier(), $passwordHasher->hash($form->getField('newPassword')->getData())); + $repository->updateUser($user->getUserIdentifier(), $passwordHasher->hash($form->getFieldData('newPassword'))); } } diff --git a/public/admin/setnewpassword.php b/public/admin/setnewpassword.php new file mode 100644 index 0000000..3ed01dd --- /dev/null +++ b/public/admin/setnewpassword.php @@ -0,0 +1,68 @@ +getUser(); +if ($currentUser->isAuthenticated()) { + header('Location: ' . $router->generate(__DIR__ . '/resetpassword.php'), true, 303); +} + +// as per owasp recommendations - https://cheatsheetseries.owasp.org/cheatsheets/Forgot_Password_Cheat_Sheet.html#url-tokens +header('Referrer-Policy: no-referrer'); + +$errorMessage = null; +$tokenId = null; +$secret = null; + +$form1 = new ForgotPasswordEmailForm($router->generate(__FILE__)); +if ($form1->isSubmitted()) { + $form1->handleRequest(); + if ($form1->isValid()) { + $tokenId = $form1->getFieldData('token'); + $secret = $form1->getFieldData('secret'); + } else { + $errorMessage = $form1->errorMessage; + } +} + +$form2 = new SetNewPasswordForm($router->generate(__FILE__), $tokenId, $secret); +if ($errorMessage === null && $form2->isSubmitted()) { + $form2->handleRequest(); + if ($form2->isValid()) { + // update the user + $user = $form2->getUser(); + $passwordHasher = new PasswordHasher(); + $repository = new UserRepository(); + $repository->updateUser($user->username, $passwordHasher->hash($form2->getFieldData('newPassword'))); + // 'consume' (remove) the token + $form2->getTokenRepository()->delete($form2->getFieldData('token')); + } +} + +if (!$form1->isSubmitted() && !$form2->isSubmitted()) { + $errorMessage = 'Nothing to see here, move along.'; + + $logger = Logger::getInstance('audit'); + $logger->debug("A request for setnewpassword.php was received, with no form submitted. Hacking attempt?"); +} + +$tpl = new Templating(); +echo $tpl->render('admin/setnewpassword.html.twig', [ + //'user' => $tokenUser, + 'error' => $errorMessage, + 'form' => $form2, + 'urls' => $firewall->getAdminUrls(), +]); diff --git a/resources/templates/admin/forgotpassword.html.twig b/resources/templates/admin/forgotpassword.html.twig new file mode 100644 index 0000000..56b5408 --- /dev/null +++ b/resources/templates/admin/forgotpassword.html.twig @@ -0,0 +1,21 @@ +{% extends "base.html.twig" %} + +{% block content %} + {% import "parts/forms/macros.html.twig" as forms %} + {% if form.isSubmitted() and (form.isValid() or form.pretendIsValid()) %} +

Thanks. If the email provided matches an active user account, an email has been sent to it.

+ {% else %} + {# @todo make the form a bit less wide #} + + {% if form.errorMessage|default('') != '' %} +

{{ form.errorMessage }}

+ {% endif %} +
+ {{ forms.input(form.getField('email'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} +
+
+ {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }} +
+ + {% endif %} +{% endblock %} diff --git a/resources/templates/admin/login.html.twig b/resources/templates/admin/login.html.twig index 9e683db..49345e9 100644 --- a/resources/templates/admin/login.html.twig +++ b/resources/templates/admin/login.html.twig @@ -18,4 +18,5 @@ {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }}
+Password forgotten? {% endblock %} diff --git a/resources/templates/admin/setnewpassword.html.twig b/resources/templates/admin/setnewpassword.html.twig new file mode 100644 index 0000000..555c9ff --- /dev/null +++ b/resources/templates/admin/setnewpassword.html.twig @@ -0,0 +1,29 @@ +{% extends "base.html.twig" %} + +{% block content %} + {% import "parts/forms/macros.html.twig" as forms %} + {% if error != '' %} +

{{ error }}

+ {% elseif form.isSubmitted() and form.isValid() %} +

The password has been updated. Please log in.

+ {% else %} + {# @todo make the form a bit less wide #} +
+ {% if form.errorMessage|default('') != '' %} +

{{ form.errorMessage }}

+ {% endif %} +
+ {{ forms.input(form.getField('newPassword'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} +
+
+ {{ forms.input(form.getField('newPasswordConfirm'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}) }} +
+
+ {{ forms.input(form.getField('token')) }} + {{ forms.input(form.getField('secret')) }} + {#{ forms.input(form.getField('antiCSRF')) }#} + {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }} +
+
+ {% endif %} +{% endblock %} diff --git a/resources/templates/emails/forgotpassword.txt.twig b/resources/templates/emails/forgotpassword.txt.twig new file mode 100644 index 0000000..eadcbf1 --- /dev/null +++ b/resources/templates/emails/forgotpassword.txt.twig @@ -0,0 +1,7 @@ +Please click on the following link to set up a new password: + +{{ link }} + +Need further assistance? Contact us at ... + +The VeraCrypt Crash Collector Admin Team diff --git a/src/Entity/ForgotPasswordToken.php b/src/Entity/ForgotPasswordToken.php new file mode 100644 index 0000000..761871e --- /dev/null +++ b/src/Entity/ForgotPasswordToken.php @@ -0,0 +1,7 @@ +dateCreated = $dateCreated; + } else { + $this->dateCreated = $dateCreated->getTimestamp(); + } + if (null === $expirationDate || is_int($expirationDate)) { + $this->expirationDate = $expirationDate; + } else { + $this->expirationDate = $expirationDate->getTimestamp(); + } + } + + public function __get($name) + { + switch ($name) { + case 'dateCreated': + return $this->dateCreated; + case 'dateCreatedDT': + return new DateTimeImmutable("@{$this->dateCreated}"); + case 'expirationDate': + return $this->expirationDate; + case 'expirationDateDT': + return $this->expirationDate === null ? null : new DateTimeImmutable("@{$this->expirationDate}"); + default: + $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 1); + trigger_error('Undefined property via __get(): ' . $name . ' in ' . $trace[0]['file'] . ' on line ' . + $trace[0]['line'], E_USER_ERROR); + } + } + + public function __isset($name) + { + switch ($name) { + case 'dateCreated': + case 'dateCreatedDT': + return true; + case 'expirationDate': + case 'expirationDateDT': + return isset($this->expirationDate); + default: + return false; + } + } +} diff --git a/src/Entity/UserToken.php b/src/Entity/UserToken.php new file mode 100644 index 0000000..66ff342 --- /dev/null +++ b/src/Entity/UserToken.php @@ -0,0 +1,26 @@ +fetchUser($this->username); + } +} diff --git a/src/Exception/ConstraintException.php b/src/Exception/ConstraintException.php new file mode 100644 index 0000000..3a2d9c5 --- /dev/null +++ b/src/Exception/ConstraintException.php @@ -0,0 +1,7 @@ +errorMessage if a non-constraint is violated. */ protected function validateValue(mixed $value): null|string @@ -121,6 +123,7 @@ protected function validateConstraints(?string $value): bool } break; case FC::RateLimit: + /// @todo can this be implemented as a Custom Constraint? try { $targetValue->validateRequest((string)$value); } catch (RateLimitExceedException $e) { @@ -133,6 +136,15 @@ protected function validateConstraints(?string $value): bool return false; } break; + case FC::Custom: + /// @todo we should find a simple way to let the form users retrieve more info than just the exception message + try { + $targetValue->validateRequest((string)$value); + } catch (ConstraintException $e) { + $this->errorMessage = $e->getMessage(); + return false; + } + break; // this is checked at constructor time //default: // throw new \DomainException("Unsupported field constraint: '$constraint"); diff --git a/src/Form/Field/Constraint/ActiveUserEmailConstraint.php b/src/Form/Field/Constraint/ActiveUserEmailConstraint.php new file mode 100644 index 0000000..c0c643c --- /dev/null +++ b/src/Form/Field/Constraint/ActiveUserEmailConstraint.php @@ -0,0 +1,26 @@ +fetchUsersByEmail($value); + if (!$users || count($users) !== 1) { + throw new ConstraintUserNotFoundException('User matching email either not found or not unique'); + } + $user = $users[0]; + if (!$user->isActive()) { + throw new ConstraintUserInactiveException('User matching email is not active'); + } + $this->user = $user; + } +} diff --git a/src/Form/Field/Constraint/ActiveUserTokenConstraint.php b/src/Form/Field/Constraint/ActiveUserTokenConstraint.php new file mode 100644 index 0000000..8321b28 --- /dev/null +++ b/src/Form/Field/Constraint/ActiveUserTokenConstraint.php @@ -0,0 +1,67 @@ +repositoryClass(); + return $ph->verify($this->token->hash, $secret); + } + + /** + * @throws \RuntimeException or subclasses thereof + */ + public function validateRequest(?string $value = null): void + { + /** @var UserTokenRepository $repo */ + $repo = new $this->repositoryClass(); + + $tokenId = (int)$value; + if ($tokenId <= 0) { + throw new TokenNotFoundException('Token not found'); + } + $this->token = $repo->fetch($tokenId); + if ($this->token === null) { + throw new TokenNotFoundException('Token not found'); + } + $user = $this->token->getUser(); + if ($user === null) { + throw new ConstraintUserNotFoundException('User matching token not found'); + } + if (!$user->isActive()) { + throw new ConstraintUserInactiveException('User matching token is not active'); + } + $this->user = $user; + } + + public function getTokenRepository(): UserTokenRepository + { + return new $this->repositoryClass(); + } +} diff --git a/src/Form/Field/Constraint/ConstraintInterface.php b/src/Form/Field/Constraint/ConstraintInterface.php new file mode 100644 index 0000000..cfb2f3d --- /dev/null +++ b/src/Form/Field/Constraint/ConstraintInterface.php @@ -0,0 +1,13 @@ +user; + } +} diff --git a/src/Form/Field/RateLimiter.php b/src/Form/Field/RateLimiter.php index 285ddeb..d305c9d 100644 --- a/src/Form/Field/RateLimiter.php +++ b/src/Form/Field/RateLimiter.php @@ -30,6 +30,9 @@ public function __construct( $this->limiter = new Limiter($constraints); } + /** + * @return null we can not enforce this via the function declaration, but this function only ever returns null + */ protected function validateValue(mixed $value): null|string { try { diff --git a/src/Form/FieldConstraint.php b/src/Form/FieldConstraint.php index 59a3b6e..b50b6eb 100644 --- a/src/Form/FieldConstraint.php +++ b/src/Form/FieldConstraint.php @@ -9,4 +9,5 @@ final class FieldConstraint const MaxLength = 2; const MinLength = 4; const RateLimit = 8; + const Custom = 16; } diff --git a/src/Form/ForgotPasswordEmailForm.php b/src/Form/ForgotPasswordEmailForm.php new file mode 100644 index 0000000..a5a791e --- /dev/null +++ b/src/Form/ForgotPasswordEmailForm.php @@ -0,0 +1,59 @@ +userConstraint = new ActiveUserTokenConstraint(ForgotPasswordTokenRepository::class); + $this->fields = [ + /// @todo add an is-integer constraint? + 'token' => new Field\Hidden('tkn', [ + FC::Required => true, + FC::Custom => $this->userConstraint + ], $tokenId), + /// @todo get the field length from the TokenRepository + 'secret' => new Field\Hidden('sec', [ + FC::Required => true, + FC::MinLength => 128, + FC::MaxLength => 128, + ], $secret) + ]; + + parent::__construct($actionUrl); + } + + protected function validateSubmit(?array $request = null): void + { + if ($this->isValid) { + // use the same error message used for invalid token-ids + if (! $this->userConstraint->validateHash($this->getFieldData('secret'))) { + $this->setError("Token not found"); + } + } + } + + public function isSubmitted(?array $request = null): bool + { + if ($request === null) { + $request = $this->getRequest(); + } + return array_key_exists('tkn', $request); + } + + public function getUser(): null|User + { + return $this->userConstraint->getUser(); + } +} diff --git a/src/Form/ForgotPasswordForm.php b/src/Form/ForgotPasswordForm.php new file mode 100644 index 0000000..a50f670 --- /dev/null +++ b/src/Form/ForgotPasswordForm.php @@ -0,0 +1,58 @@ +userConstraint = new ActiveUserEmailConstraint(); + $this->fields = [ + /// @todo get the field length from the Repo field + 'email' => new Field\Email('Email', 'em', [ + FC::Required => true, + FC::MaxLength => 254, + FC::RateLimit => new RateLimiter([ + new FixedWindow($actionUrl, 10, 300), // equivalent to once every 30 secs + new FixedWindow($actionUrl, 12, 3600), // equivalent to once every 5 minutes + new FixedWindow($actionUrl, 120, 86400), // equivalent to once every 12 minutes + ]), + FC::Custom => $this->userConstraint + ]) + ]; + + parent::__construct($actionUrl); + } + + public function handleRequest(?array $request = null): void + { + parent::handleRequest($request); + + // Allow hiding all error messages from the end user, except empty and too-long email, to avoid the enumeration + // of existing users email (but keep $this->isValid false) + if (!$this->isValid && str_starts_with($this->fields['email']->errorMessage, 'User matching email ')) { + $this->pretendIsValid = true; + } + } + + public function getUser(): null|User + { + return $this->userConstraint->getUser(); + } + + public function pretendIsValid(): bool + { + return $this->pretendIsValid; + } +} diff --git a/src/Form/Form.php b/src/Form/Form.php index c21fba1..109fe11 100644 --- a/src/Form/Form.php +++ b/src/Form/Form.php @@ -2,11 +2,11 @@ namespace Veracrypt\CrashCollector\Form; +use Veracrypt\CrashCollector\Exception\FormFieldNotSubmittedException; use Veracrypt\CrashCollector\Form\Field\SubmitButton; /** * @property-read ?string $errorMessage - * @property-read string $actionUrl */ abstract class Form { @@ -20,17 +20,17 @@ abstract class Form protected int $submitOn = self::ON_POST; protected bool $isValid = false; protected ?string $errorMessage = null; - protected string $actionUrl; + protected string $submitInputName = 's'; + protected string|int $submitInputValue = 1; - public function __construct(string $actionUrl) + public function __construct(public readonly string $actionUrl) { - $this->actionUrl = $actionUrl; } /** * @throws \DomainException */ - public function getField($fieldName): Field + public function getField(string $fieldName): Field { if (array_key_exists($fieldName, $this->fields)) { return $this->fields[$fieldName]; @@ -48,15 +48,15 @@ public function getMethod(): string public function getSubmit(): Field { - return new SubmitButton($this->submitLabel, 's', [], 1); + return new SubmitButton($this->submitLabel, $this->submitInputName, [], $this->submitInputValue); } public function isSubmitted(?array $request = null): bool { - $submit = $this->getSubmit(); if ($request === null) { $request = $this->getRequest(); } + $submit = $this->getSubmit(); return isset($request[$submit->inputName]) && $request[$submit->inputName] == $submit->value; } @@ -96,6 +96,23 @@ protected function validateSubmit(?array $request = null): void { } + /** + * Make sure, before calling this, that the field was submitted. Typically, add a Required constraint to it. + * @throws \DomainException for invalid field names + * @throws FormFieldNotSubmittedException + */ + public function getFieldData(string $fieldName): mixed + { + $field = $this->getField($fieldName); + if ($field->value === null) { + throw new FormFieldNotSubmittedException("Form field '$fieldName' was not submitted"); + } + return $field->getData(); + } + + /** + * Returns values for all fields which were submitted. + */ public function getData(): array { $data = []; @@ -147,7 +164,7 @@ public function setError(?string $errorMessage) public function __get($name) { switch ($name) { - case 'actionUrl': + //case 'actionUrl': case 'errorMessage': return $this->$name; default: @@ -160,7 +177,7 @@ public function __get($name) public function __isset($name) { return match ($name) { - 'actionUrl' => true, + //'actionUrl' => true, 'errorMessage' => isset($this->$name), default => false }; diff --git a/src/Form/LoginForm.php b/src/Form/LoginForm.php index cb42840..67f27a5 100644 --- a/src/Form/LoginForm.php +++ b/src/Form/LoginForm.php @@ -37,6 +37,6 @@ public function __construct(string $actionUrl, string $redirect) public function onSuccessfulLogin() { - $this->rateLimiter->reset($this->getField('username')->getData()); + $this->rateLimiter->reset($this->getFieldData('username')); } } diff --git a/src/Form/PasswordUpdateBaseForm.php b/src/Form/PasswordUpdateBaseForm.php new file mode 100644 index 0000000..c9433b1 --- /dev/null +++ b/src/Form/PasswordUpdateBaseForm.php @@ -0,0 +1,34 @@ +fields = $this->getFieldsDefinitions($actionUrl); + parent::__construct($actionUrl); + } + + protected function getFieldsDefinitions(string $actionUrl): array + { + return [ + 'newPassword' => new Field\Password('New Password', 'np', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + 'newPasswordConfirm' => new Field\Password('Confirm new Password', 'npc', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), + ]; + } + + protected function validateSubmit(?array $request = null): void + { +/// @check: do we need the ref assignment? + /** @var Field $npcField */ + $npcField =& $this->fields['newPasswordConfirm']; + if ($this->fields['newPassword']->getData() !== $npcField->getData()) { + $npcField->setError('The password does not match'); + $this->isValid = false; + } + } +} diff --git a/src/Form/ResetPasswordForm.php b/src/Form/ResetPasswordForm.php index 8e76fe2..519e909 100644 --- a/src/Form/ResetPasswordForm.php +++ b/src/Form/ResetPasswordForm.php @@ -8,32 +8,29 @@ use Veracrypt\CrashCollector\Security\UserInterface; use Veracrypt\CrashCollector\Security\UsernamePasswordAuthenticator; -class ResetPasswordForm extends Form +class ResetPasswordForm extends PasswordUpdateBaseForm { protected UserInterface $currentUser; public function __construct(string $actionUrl, UserInterface $currentUser) { - $this->fields = [ - /// @todo add min pwd length constraints, maybe even a regex? - 'oldPassword' => new Field\Password('Current Password', 'cp', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'newPassword' => new Field\Password('New Password', 'np', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'newPasswordConfirm' => new Field\Password('Confirm new Password', 'npc', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH]), - 'antiCSRF' => new Field\AntiCSRF('ac', $actionUrl), - ]; $this->currentUser = $currentUser; - parent::__construct($actionUrl); } + protected function getFieldsDefinitions(string $actionUrl): array + { + return array_merge( + ['oldPassword' => new Field\Password('Current Password', 'cp', [FC::Required => true, FC::MaxLength => PasswordHasher::MAX_PASSWORD_LENGTH])], + parent::getFieldsDefinitions($actionUrl), + ['antiCSRF' => new Field\AntiCSRF('ac', $actionUrl)] + ); + } + protected function validateSubmit(?array $request = null): void { - /** @var Field $npcField */ - $npcField =& $this->fields['newPasswordConfirm']; - if ($this->fields['newPassword']->getData() !== $npcField->getData()) { - $npcField->setError('The password does not match'); - $this->isValid = false; - } else { + parent::validateSubmit($request); + if ($this->isValid) { $authenticator = new UsernamePasswordAuthenticator(); try { $authenticator->authenticate($this->currentUser->getUserIdentifier(), $this->fields['oldPassword']->getData()); diff --git a/src/Form/SetNewPasswordForm.php b/src/Form/SetNewPasswordForm.php new file mode 100644 index 0000000..cf21797 --- /dev/null +++ b/src/Form/SetNewPasswordForm.php @@ -0,0 +1,66 @@ +tokenId = $tokenId; + $this->secret = $secret; + parent::__construct($actionUrl); + } + + protected function getFieldsDefinitions(string $actionUrl, string $token = ''): array + { +/// @todo add anti-csrf and/or rate-limiting + $this->userConstraint = new ActiveUserTokenConstraint(ForgotPasswordTokenRepository::class); + return array_merge(parent::getFieldsDefinitions($actionUrl), [ + 'token' => new Field\Hidden('tkn', [ + FC::Required => true, + FC::Custom => $this->userConstraint + ], $this->tokenId), + /// @todo get the field length from the TokenRepository + 'secret' => new Field\Hidden('sec', [ + FC::Required => true, + FC::MinLength => 128, + FC::MaxLength => 128, + ], $this->secret) + ]); + } + + protected function validateSubmit(?array $request = null): void + { + parent::validateSubmit($request); + if ($this->isValid) { + if (!$this->userConstraint->validateHash($this->getFieldData('secret'))) { + // use the same error message used for invalid token-ids + $this->setError("Token not found"); + } + } + } + + + public function getUser(): null|User + { + return $this->userConstraint->getUser(); + } + + public function getTokenRepository(): UserTokenRepository + { + return $this->userConstraint->getTokenRepository(); + } +} diff --git a/src/Mailer/Email.php b/src/Mailer/Email.php new file mode 100644 index 0000000..0f36df6 --- /dev/null +++ b/src/Mailer/Email.php @@ -0,0 +1,55 @@ +from = $from; + return $this; + } + + public function getFrom(): string + { + return $this->from; + } + + public function to(string $to): Email + { + $this->to = $to; + return $this; + } + + public function getTo(): string + { + return $this->to; + } + + public function subject(string $subject): Email + { + $this->subject = $subject; + return $this; + } + + public function getSubject(): string + { + return $this->subject; + } + + public function text(string $text): Email + { + $this->text = $text; + return $this; + } + + public function getText(): string + { + return $this->text; + } +} diff --git a/src/Mailer/Mailer.php b/src/Mailer/Mailer.php new file mode 100644 index 0000000..d3fb3c0 --- /dev/null +++ b/src/Mailer/Mailer.php @@ -0,0 +1,25 @@ + $message->getFrom(), + ]; + // replace single \n chars with \r\n + $text = preg_replace('/(^|[^\\r])\\n/', "\\1\r\n", $message->getText()); + if (!mail($message->getTo(), $message->getSubject(), $text, $additionalHeaders)) { + throw new \RuntimeException("Mail delivery failed"); + } + } +} diff --git a/src/Repository/DatabaseRepository.php b/src/Repository/DatabaseRepository.php index 673fa6b..78e0f5c 100644 --- a/src/Repository/DatabaseRepository.php +++ b/src/Repository/DatabaseRepository.php @@ -4,6 +4,9 @@ use Veracrypt\CrashCollector\Storage\DatabaseTable; +/** + * @property Field[] $fields + */ abstract class DatabaseRepository { use DatabaseTable; @@ -48,13 +51,23 @@ protected function buildFetchEntityQuery(): string } /** + * @return null|array when there are autoincrement cols, and no value is passed in for those, their value is returned * @throws \PDOException */ - protected function storeEntity($value): void + protected function storeEntity($value): null|array { $query = 'insert into ' . $this->tableName . ' ('; $vq = ''; + $autoIncrementCols = []; foreach($this->fields as $colName => $field) { + foreach ($field->constraints as $constraintType => $constraintValue) { + if ($constraintType === FieldConstraint::Autoincrement && $constraintValue) { + $entityField = $field->entityField; + if ($entityField == '' || $value->$entityField === null) { + $autoIncrementCols[] = $colName; + } + } + } if ($field->entityField == '') { continue; } @@ -62,6 +75,10 @@ protected function storeEntity($value): void $vq .= ":$colName" . ', '; } $query = substr($query, 0, -2) . ') values (' . substr($vq, 0, -2) . ')'; + if ($autoIncrementCols) { + // 'returning' is supported by sqlite >= ..., mariadb >= 10.5, postgresql + $query .= ' returning ' . implode(', ', $autoIncrementCols); + } $stmt = self::$dbh->prepare($query); /// @todo test: can `bindvalue` or `execute` fail without throwing? @@ -78,5 +95,11 @@ protected function storeEntity($value): void $stmt->bindValue(":$colName", $val); } $stmt->execute(); + + if ($autoIncrementCols) { + return $stmt->fetch(\PDO::FETCH_ASSOC); + } + + return null; } } diff --git a/src/Repository/ForgotPasswordTokenRepository.php b/src/Repository/ForgotPasswordTokenRepository.php new file mode 100644 index 0000000..081bc1b --- /dev/null +++ b/src/Repository/ForgotPasswordTokenRepository.php @@ -0,0 +1,27 @@ +fields = $this->getFieldsDefinitions(); + + parent::__construct(); + } + + protected function getFieldsDefinitions() + { + return [ + 'id' => new Field('id', 'integer', [FC::NotNull => true, FC::PK => true, FC::Autoincrement => true]), + 'hash' => new Field('hash', 'varchar', [FC::Length => 255, FC::NotNull => true]), + 'date_created' => new Field('dateCreated', 'integer', [FC::NotNull => true]), + 'expiration_date' => new Field('expirationDate', 'integer', []), + // could add cols: usage_count, last_usage_datetime + ]; + } + + abstract protected function newTokenExpirationDate(): null|int; + + public function fetch(int $id): null|Token + { + $query = $this->buildFetchEntityQuery() . ' where id = :id'; + $stmt = self::$dbh->prepare($query); + $stmt->bindValue(':id', $id); + $stmt->execute(); + $result = $stmt->fetch(\PDO::FETCH_ASSOC); + return $result ? new $this->entityClass(...$result) : null; + } + + public function delete(int $id): bool + { + $query = 'delete from ' . $this->tableName . ' where id = :id'; + $stmt = self::$dbh->prepare($query); + $stmt->bindValue(':id', $id); + $stmt->execute(); + return (bool)$stmt->rowCount(); + } + + /** + * Removes all expired tokens + */ + public function prune(): bool + { + $query = 'delete from ' . $this->tableName . ' where expiration_date <= :now'; + $stmt = self::$dbh->prepare($query); + $stmt->bindValue(':now', time()); + $stmt->execute(); + return (bool)$stmt->rowCount(); + } +} diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 74994df..505f642 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -23,6 +23,7 @@ public function __construct() 'id' => new Field(null, 'integer', [FC::NotNull => true, FC::PK => true, FC::Autoincrement => true]), 'username' => new Field('username', 'varchar', [FC::Length => 180, FC::NotNull => true, FC::Unique => true]), 'password' => new Field('passwordHash', 'varchar', [FC::Length => 255, FC::NotNull => true]), + // q: should we make emails unique? 'email' => new Field('email', 'varchar', [FC::Length => 254, FC::NotNull => true]), 'first_name' => new Field('firstName', 'varchar', [FC::Length => 150, FC::NotNull => true]), 'last_name' => new Field('lastName', 'varchar', [FC::Length => 150, FC::NotNull => true]), @@ -64,6 +65,20 @@ public function fetchUser(string $username): User|null return $result ? new User(...$result) : null; } + /** + * @return User[] + * @throws \PDOException + */ + public function fetchUsersByEmail(string $email): array + { + $query = $this->buildFetchEntityQuery() . ' where email = :email'; + $stmt = self::$dbh->prepare($query); + $stmt->bindValue(':email', $email); + $stmt->execute(); + $results = $stmt->fetchAll(\PDO::FETCH_NUM); + return array_map(static fn($result) => new User(...$result), $results); + } + /** * NB: passing in an empty string for any value will trigger the data to be updated in the DB, unlike passing in a NULL. * This might be unexpected... diff --git a/src/Repository/UserTokenRepository.php b/src/Repository/UserTokenRepository.php new file mode 100644 index 0000000..d8bf76a --- /dev/null +++ b/src/Repository/UserTokenRepository.php @@ -0,0 +1,33 @@ + new Field('username', 'varchar', [FC::Length => 180, FC::NotNull => true]), + ]); + } + + public function createToken(string $userName, string $hash): UserToken + { + $args['id'] = null; + $args['hash'] = $hash; + $args['username'] = $userName; + $args['dateCreated'] = time(); + $args['expirationDate'] = $this->newTokenExpirationDate(); + $token = new $this->entityClass(...$args); + $autoincrements = $this->storeEntity($token); + // we have to create a new entity object in order to inject the id into it + $args['id'] = $autoincrements['id']; + return new $this->entityClass(...$args); + } +} diff --git a/src/Security/Firewall.php b/src/Security/Firewall.php index f607e14..d7a66fc 100644 --- a/src/Security/Firewall.php +++ b/src/Security/Firewall.php @@ -172,6 +172,7 @@ public function getAdminUrls(): array 'login' => $router->generate(__DIR__ . '/../../public/admin/login.php'), 'logout' => $router->generate(__DIR__ . '/../../public/admin/logout.php'), 'resetpassword' => $router->generate(__DIR__ . '/../../public/admin/resetpassword.php'), + 'forgotpassword' => $router->generate(__DIR__ . '/../../public/admin/forgotpassword.php'), ]; } } diff --git a/src/Security/PasswordHasher.php b/src/Security/PasswordHasher.php index 5452095..7c0c966 100644 --- a/src/Security/PasswordHasher.php +++ b/src/Security/PasswordHasher.php @@ -93,4 +93,9 @@ public function needsRehash(string $hashedPassword): bool { return password_needs_rehash($hashedPassword, $this->algorithm, $this->options); } + + public function generateRandomString(int $length): string + { + return substr(bin2hex(random_bytes($length)), 0, $length); + } } From e6f14288fcd8e06786e8142a51f8966e17554e07 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 7 Nov 2024 17:21:30 +0000 Subject: [PATCH 24/38] add rate-limiting to forgotpassword forms --- src/Form/ForgotPasswordEmailForm.php | 10 ++++++++-- src/Form/SetNewPasswordForm.php | 9 ++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/Form/ForgotPasswordEmailForm.php b/src/Form/ForgotPasswordEmailForm.php index a5a791e..5c322e2 100644 --- a/src/Form/ForgotPasswordEmailForm.php +++ b/src/Form/ForgotPasswordEmailForm.php @@ -6,6 +6,8 @@ use Veracrypt\CrashCollector\Form\Field\Constraint\ActiveUserTokenConstraint; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; use Veracrypt\CrashCollector\Form\Form as BaseForm; +use Veracrypt\CrashCollector\RateLimiter\Constraint\FixedWindow; +use Veracrypt\CrashCollector\RateLimiter\RateLimiter; use Veracrypt\CrashCollector\Repository\ForgotPasswordTokenRepository; class ForgotPasswordEmailForm extends BaseForm @@ -15,12 +17,16 @@ class ForgotPasswordEmailForm extends BaseForm public function __construct(string $actionUrl, ?int $tokenId = null, #[\SensitiveParameter] ?string $secret = null) { -/// @todo add anti-csrf and/or rate-limiting? $this->userConstraint = new ActiveUserTokenConstraint(ForgotPasswordTokenRepository::class); $this->fields = [ /// @todo add an is-integer constraint? 'token' => new Field\Hidden('tkn', [ FC::Required => true, + FC::RateLimit => new RateLimiter([ + new FixedWindow($actionUrl, 10, 300), // equivalent to once every 30 secs + new FixedWindow($actionUrl, 12, 3600), // equivalent to once every 5 minutes + new FixedWindow($actionUrl, 120, 86400), // equivalent to once every 12 minutes + ]), FC::Custom => $this->userConstraint ], $tokenId), /// @todo get the field length from the TokenRepository @@ -38,7 +44,7 @@ protected function validateSubmit(?array $request = null): void { if ($this->isValid) { // use the same error message used for invalid token-ids - if (! $this->userConstraint->validateHash($this->getFieldData('secret'))) { + if (!$this->userConstraint->validateHash($this->getFieldData('secret'))) { $this->setError("Token not found"); } } diff --git a/src/Form/SetNewPasswordForm.php b/src/Form/SetNewPasswordForm.php index cf21797..54ea425 100644 --- a/src/Form/SetNewPasswordForm.php +++ b/src/Form/SetNewPasswordForm.php @@ -5,6 +5,8 @@ use Veracrypt\CrashCollector\Entity\User; use Veracrypt\CrashCollector\Form\Field\Constraint\ActiveUserTokenConstraint; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; +use Veracrypt\CrashCollector\RateLimiter\Constraint\FixedWindow; +use Veracrypt\CrashCollector\RateLimiter\RateLimiter; use Veracrypt\CrashCollector\Repository\ForgotPasswordTokenRepository; use Veracrypt\CrashCollector\Repository\UserTokenRepository; @@ -26,11 +28,16 @@ public function __construct(string $actionUrl, ?int $tokenId, #[\SensitiveParame protected function getFieldsDefinitions(string $actionUrl, string $token = ''): array { -/// @todo add anti-csrf and/or rate-limiting + /// @todo should we move to starting a session before displaying this form, and add an anti-csrf token instead of rate-limiting? $this->userConstraint = new ActiveUserTokenConstraint(ForgotPasswordTokenRepository::class); return array_merge(parent::getFieldsDefinitions($actionUrl), [ 'token' => new Field\Hidden('tkn', [ FC::Required => true, + FC::RateLimit => new RateLimiter([ + new FixedWindow($actionUrl, 10, 300), // equivalent to once every 30 secs + new FixedWindow($actionUrl, 12, 3600), // equivalent to once every 5 minutes + new FixedWindow($actionUrl, 120, 86400), // equivalent to once every 12 minutes + ]), FC::Custom => $this->userConstraint ], $this->tokenId), /// @todo get the field length from the TokenRepository From bf25a20d1675f74dedfa17e8cc61d1ea88974b82 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 7 Nov 2024 17:27:15 +0000 Subject: [PATCH 25/38] fix fatal error in case token not in request --- src/Form/Field/Constraint/ActiveUserTokenConstraint.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Form/Field/Constraint/ActiveUserTokenConstraint.php b/src/Form/Field/Constraint/ActiveUserTokenConstraint.php index 8321b28..6cb85a2 100644 --- a/src/Form/Field/Constraint/ActiveUserTokenConstraint.php +++ b/src/Form/Field/Constraint/ActiveUserTokenConstraint.php @@ -46,10 +46,11 @@ public function validateRequest(?string $value = null): void if ($tokenId <= 0) { throw new TokenNotFoundException('Token not found'); } - $this->token = $repo->fetch($tokenId); - if ($this->token === null) { + $token = $repo->fetch($tokenId); + if ($token === null) { throw new TokenNotFoundException('Token not found'); } + $this->token = $token; $user = $this->token->getUser(); if ($user === null) { throw new ConstraintUserNotFoundException('User matching token not found'); @@ -57,6 +58,7 @@ public function validateRequest(?string $value = null): void if (!$user->isActive()) { throw new ConstraintUserInactiveException('User matching token is not active'); } + $this->user = $user; } From 8ebd302518c0f431f593318aea735789979d1243 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 7 Nov 2024 17:51:53 +0000 Subject: [PATCH 26/38] fix logic of setting errors to forms --- src/Form/Form.php | 2 +- src/Form/SetNewPasswordForm.php | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Form/Form.php b/src/Form/Form.php index 109fe11..924a35f 100644 --- a/src/Form/Form.php +++ b/src/Form/Form.php @@ -158,7 +158,7 @@ protected function getRequest() public function setError(?string $errorMessage) { $this->errorMessage = $errorMessage; - $this->isValid = ($errorMessage !== null && $errorMessage !== ''); + $this->isValid = ($errorMessage === null || $errorMessage === ''); } public function __get($name) diff --git a/src/Form/SetNewPasswordForm.php b/src/Form/SetNewPasswordForm.php index 54ea425..90e4f04 100644 --- a/src/Form/SetNewPasswordForm.php +++ b/src/Form/SetNewPasswordForm.php @@ -60,7 +60,6 @@ protected function validateSubmit(?array $request = null): void } } - public function getUser(): null|User { return $this->userConstraint->getUser(); From 32fdd05962eda9867091ef46dbdf9bad8f539080 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 7 Nov 2024 18:10:57 +0000 Subject: [PATCH 27/38] update readme with expanded installation instructions --- README.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 9369b3d..b9e4e2d 100644 --- a/README.md +++ b/README.md @@ -53,13 +53,21 @@ This project is licensed under the [Apache License 2.0](LICENSE). 2. check the configuration in `.env`, and, if required, change any value by saving it in a file named `.env.local` 3. make sure that the `var/data` directory is writeable (this is where the app will create its sqlite db by default), as well as `var/logs` and `var/cache/twig` -4. create an administrator user: run the cli command `./bin/console user:create --is-superuser ` -5. set up the webserver: +4. configure php: + + - for a production installation, it is recommended to follow the owasp guidelines available at + https://cheatsheetseries.owasp.org/cheatsheets/PHP_Configuration_Cheat_Sheet.html + - it is recommended to use Redis for php session storage +5. create an administrator user: run the cli command `php ./bin/console user:create --is-superuser ` +6. set up a cronjob (daily or weekly is fine) running the cli command `php ./bin/console forgotpasswordtoken:prune` +7. set up the webserver: - configure the vhost root directory to be the `public` directory. No http access to any other folder please - make sure .php scripts are executed via the php interpreter - no rewrite rules are necessary -6. Navigate to `https://your-host/upload/` to upload crash reports; to `https://your-host/admin/` for browsing them +8. navigate to `https://your-host/upload/` to upload crash reports; to `https://your-host/admin/` for browsing them +9. optionally, run the SQLite pragma `journal_mode=WAL` to have optimized performance and concurrency +10. optionally, set up cronjobs to run the SQLite pragmas `optimize` and `integrity_check` ## How it works From 9306d4ccb1c72beac08fc7f803cbe1019845e5f1 Mon Sep 17 00:00:00 2001 From: gggeek Date: Thu, 7 Nov 2024 19:35:24 +0000 Subject: [PATCH 28/38] add support for db indexes and foreign keys --- src/Repository/CrashReportRepository.php | 13 ++++++- src/Repository/DatabaseRepository.php | 4 +-- src/Repository/Field.php | 4 +-- src/Repository/TokenRepository.php | 16 +++++++++ src/Repository/UserRepository.php | 6 ++++ src/Repository/UserTokenRepository.php | 10 ++++++ src/Storage/Database.php | 8 +++++ .../Column.php} | 4 +-- src/Storage/Database/ForeignKey.php | 19 ++++++++++ src/Storage/Database/ForeignKeyAction.php | 12 +++++++ src/Storage/Database/Index.php | 15 ++++++++ .../{DatabaseTable.php => Database/Table.php} | 35 ++++++++++++++----- 12 files changed, 131 insertions(+), 15 deletions(-) rename src/Storage/{DatabaseColumn.php => Database/Column.php} (56%) create mode 100644 src/Storage/Database/ForeignKey.php create mode 100644 src/Storage/Database/ForeignKeyAction.php create mode 100644 src/Storage/Database/Index.php rename src/Storage/{DatabaseTable.php => Database/Table.php} (71%) diff --git a/src/Repository/CrashReportRepository.php b/src/Repository/CrashReportRepository.php index ab19ad7..4bc54fa 100644 --- a/src/Repository/CrashReportRepository.php +++ b/src/Repository/CrashReportRepository.php @@ -5,6 +5,7 @@ use DateTimeInterface; use Veracrypt\CrashCollector\Entity\CrashReport; use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; +use Veracrypt\CrashCollector\Storage\Database\Index; class CrashReportRepository extends DatabaseRepository { @@ -28,7 +29,17 @@ public function __construct() 'error_address' => new Field('errorAddress', 'varchar', [FC::Length => 255, FC::NotNull => true]), 'call_stack' => new Field('callStack', 'blob', [FC::NotNull => true]), ]; - + /// @todo should we just add a covering index which uses all columns? If so, figure out first the cardinality of each + $this->indexes = [ + 'idx_' . $this->tableName . '_dr' => new Index(['date_reported']), + 'idx_' . $this->tableName . '_pm' => new Index(['program_version']), + 'idx_' . $this->tableName . '_ov' => new Index(['os_version']), + 'idx_' . $this->tableName . '_ha' => new Index(['hw_architecture']), + 'idx_' . $this->tableName . '_es' => new Index(['executable_checksum']), + 'idx_' . $this->tableName . '_ec' => new Index(['error_category']), + 'idx_' . $this->tableName . '_ea' => new Index(['error_address']), + 'idx_' . $this->tableName . '_cs' => new Index(['call_stack']), + ]; parent::__construct(); } diff --git a/src/Repository/DatabaseRepository.php b/src/Repository/DatabaseRepository.php index 78e0f5c..0e40b59 100644 --- a/src/Repository/DatabaseRepository.php +++ b/src/Repository/DatabaseRepository.php @@ -2,14 +2,14 @@ namespace Veracrypt\CrashCollector\Repository; -use Veracrypt\CrashCollector\Storage\DatabaseTable; +use Veracrypt\CrashCollector\Storage\Database\Table; /** * @property Field[] $fields */ abstract class DatabaseRepository { - use DatabaseTable; + use Table; /** * @throws \DomainException diff --git a/src/Repository/Field.php b/src/Repository/Field.php index 4990805..f3fb726 100644 --- a/src/Repository/Field.php +++ b/src/Repository/Field.php @@ -2,11 +2,11 @@ namespace Veracrypt\CrashCollector\Repository; -use Veracrypt\CrashCollector\Storage\DatabaseColumn; +use Veracrypt\CrashCollector\Storage\Database\Column; class Field { - use DatabaseColumn; + use Column; /** * @param mixed $constraints keys must be FieldConstraint constants diff --git a/src/Repository/TokenRepository.php b/src/Repository/TokenRepository.php index 545eb9d..61c1803 100644 --- a/src/Repository/TokenRepository.php +++ b/src/Repository/TokenRepository.php @@ -4,6 +4,7 @@ use Veracrypt\CrashCollector\Entity\Token; use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; +use Veracrypt\CrashCollector\Storage\Database\Index; abstract class TokenRepository extends DatabaseRepository { @@ -17,6 +18,8 @@ abstract class TokenRepository extends DatabaseRepository protected function __construct(public readonly int $tokenLength) { $this->fields = $this->getFieldsDefinitions(); + $this->foreignKeys = $this->getForeignKeyDefinitions(); + $this->indexes = $this->getIndexesDefinitions(); parent::__construct(); } @@ -32,6 +35,19 @@ protected function getFieldsDefinitions() ]; } + protected function getForeignKeyDefinitions(): array + { + return []; + } + + protected function getIndexesDefinitions(): array + { + return [ + // used by the purge command + 'idx_' . $this->tableName . '_ed' => new Index(['expiration_date']), + ]; + } + abstract protected function newTokenExpirationDate(): null|int; public function fetch(int $id): null|Token diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 505f642..ef0b310 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -5,6 +5,7 @@ use Veracrypt\CrashCollector\Entity\User; use Veracrypt\CrashCollector\Logger; use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; +use Veracrypt\CrashCollector\Storage\Database\Index; class UserRepository extends DatabaseRepository { @@ -33,6 +34,11 @@ public function __construct() //'is_staff' => new Field('isStaff', 'bool', [FC::NotNull => true, FC::Default => 'false']), 'is_superuser' => new Field('isSuperuser', 'bool', [FC::NotNull => true, FC::Default => 'false']), ]; + $this->indexes = [ + // afaik, an unique index on username will have been created automatically to enforce FC::Unique + //'idx_' . $this->tableName . '_un' => new Index(['username'], true), + 'idx_' . $this->tableName . '_em' => new Index(['email']), + ]; $this->logger = Logger::getInstance('audit'); parent::__construct(); diff --git a/src/Repository/UserTokenRepository.php b/src/Repository/UserTokenRepository.php index d8bf76a..8432121 100644 --- a/src/Repository/UserTokenRepository.php +++ b/src/Repository/UserTokenRepository.php @@ -4,6 +4,8 @@ use Veracrypt\CrashCollector\Entity\UserToken; use Veracrypt\CrashCollector\Repository\FieldConstraint as FC; +use Veracrypt\CrashCollector\Storage\Database\ForeignKey; +use Veracrypt\CrashCollector\Storage\Database\ForeignKeyAction; /** * @method null|UserToken fetch(int $id) @@ -17,6 +19,14 @@ protected function getFieldsDefinitions(): array ]); } + protected function getForeignKeyDefinitions(): array + { + return [ + /// @todo make 'auth_user' a static var or class const of UserRepository, so that we can grab it from there + new ForeignKey(['username'], 'auth_user', ['username'], ForeignKeyAction::Cascade, ForeignKeyAction::Cascade), + ]; + } + public function createToken(string $userName, string $hash): UserToken { $args['id'] = null; diff --git a/src/Storage/Database.php b/src/Storage/Database.php index 9d89354..55d3be9 100644 --- a/src/Storage/Database.php +++ b/src/Storage/Database.php @@ -19,6 +19,14 @@ protected function connect(): void self::$dbh = new PDO($_ENV['DB_DSN'], $_ENV['DB_USER'], $_ENV['DB_PASSWORD']); self::$dbh->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC); + + $dbType = self::$dbh->getAttribute(PDO::ATTR_DRIVER_NAME); + switch ($dbType) { + case 'sqlite': + $query = "PRAGMA foreign_keys = ON"; + self::$dbh->query($query); + break; + } } } diff --git a/src/Storage/DatabaseColumn.php b/src/Storage/Database/Column.php similarity index 56% rename from src/Storage/DatabaseColumn.php rename to src/Storage/Database/Column.php index 758b7be..ba6feb0 100644 --- a/src/Storage/DatabaseColumn.php +++ b/src/Storage/Database/Column.php @@ -1,8 +1,8 @@ foreignKeys as $fk) { + $query .= 'FOREIGN KEY (' . implode(', ', $fk->columns). ') REFERENCES ' . $fk->parentTable. '(' . + implode(', ', $fk->parentColumns). ') ON DELETE ' . $fk->onDelete->value . ' ON UPDATE ' . + $fk->onUpdate->value . ', '; + } + $query = substr($query, 0, -2) . ')'; self::$dbh->exec($query); + + foreach ($this->indexes as $name => $idx) { + $query = 'CREATE ' . ($idx->unique ? 'UNIQUE ' : '') . 'INDEX ' . $name . ' ON ' . $this->tableName . '(' . + implode(', ', $idx->columns) . ')'; + self::$dbh->exec($query); + } } } From cd4dc8bf2899b7424268fac9864c4e033e2b1cae Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 8 Nov 2024 15:01:45 +0000 Subject: [PATCH 29/38] add support for pgsql and wip suupport for mariadb --- src/Repository/DatabaseRepository.php | 23 ++++++++++---------- src/Storage/Database.php | 31 ++++++++++++++++++++------- src/Storage/Database/Table.php | 25 +++++++++++++++++---- 3 files changed, 56 insertions(+), 23 deletions(-) diff --git a/src/Repository/DatabaseRepository.php b/src/Repository/DatabaseRepository.php index 0e40b59..41dc810 100644 --- a/src/Repository/DatabaseRepository.php +++ b/src/Repository/DatabaseRepository.php @@ -57,24 +57,25 @@ protected function buildFetchEntityQuery(): string protected function storeEntity($value): null|array { $query = 'insert into ' . $this->tableName . ' ('; - $vq = ''; + //$vq = ''; + $bindCols = []; $autoIncrementCols = []; foreach($this->fields as $colName => $field) { - foreach ($field->constraints as $constraintType => $constraintValue) { - if ($constraintType === FieldConstraint::Autoincrement && $constraintValue) { - $entityField = $field->entityField; - if ($entityField == '' || $value->$entityField === null) { - $autoIncrementCols[] = $colName; - } + if (isset($field->constraints[FieldConstraint::Autoincrement]) && $field->constraints[FieldConstraint::Autoincrement]) { + $entityField = $field->entityField; + if ($entityField == '' || $value->$entityField === null) { + $autoIncrementCols[] = $colName; + continue; } } if ($field->entityField == '') { continue; } - $query .= $colName . ', '; - $vq .= ":$colName" . ', '; + $bindCols[] = $colName; + //$query .= $colName . ', '; + //$vq .= ":$colName" . ', '; } - $query = substr($query, 0, -2) . ') values (' . substr($vq, 0, -2) . ')'; + $query .= implode(', ', $bindCols) . ') values (:' . implode(', :', $bindCols) . ')'; if ($autoIncrementCols) { // 'returning' is supported by sqlite >= ..., mariadb >= 10.5, postgresql $query .= ' returning ' . implode(', ', $autoIncrementCols); @@ -83,7 +84,7 @@ protected function storeEntity($value): null|array $stmt = self::$dbh->prepare($query); /// @todo test: can `bindvalue` or `execute` fail without throwing? foreach($this->fields as $colName => $field) { - if ($field->entityField == '') { + if (!in_array($colName, $bindCols)) { continue; } $entityField = $field->entityField; diff --git a/src/Storage/Database.php b/src/Storage/Database.php index 55d3be9..c21ab7b 100644 --- a/src/Storage/Database.php +++ b/src/Storage/Database.php @@ -20,7 +20,7 @@ protected function connect(): void self::$dbh->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC); - $dbType = self::$dbh->getAttribute(PDO::ATTR_DRIVER_NAME); + $dbType = self::$dbh->getAttribute(PDO::ATTR_DRIVER_NAME); switch ($dbType) { case 'sqlite': $query = "PRAGMA foreign_keys = ON"; @@ -52,17 +52,32 @@ protected function tableExists(string $tableName): bool * @return string[] value has to be the table name * @throws \DomainException in case of unsupported database type * @throws \PDOException + * @todo decide if we want to include or exclude views and be consistent about it */ protected function listTables(): array { $dbType = self::$dbh->getAttribute(PDO::ATTR_DRIVER_NAME); - switch ($dbType) { - case 'sqlite': - $query = "SELECT name FROM sqlite_schema WHERE type IN ('table','view') AND name NOT LIKE 'sqlite_%'"; - break; - default: - throw new \DomainException("Database type '$dbType' is not supported"); - } + // Queries taken from Doctrine DBAL + $query = match ($dbType) { + 'mysql' => "SHOW FULL TABLES WHERE Table_type = 'BASE TABLE'", + 'pgsql' => "SELECT quote_ident(table_name) AS table_name + FROM information_schema.tables + WHERE table_schema NOT LIKE 'pg\_%' + AND table_schema != 'information_schema' + AND table_name != 'geometry_columns' + AND table_name != 'spatial_ref_sys' + AND table_type != 'VIEW'", + 'sqlite' => "SELECT name FROM sqlite_master + WHERE type = 'table' + AND name != 'sqlite_sequence' + AND name != 'geometry_columns' + AND name != 'spatial_ref_sys' + UNION ALL + SELECT name + FROM sqlite_temp_master + WHERE type = 'table'", + default => throw new \DomainException("Database type '$dbType' is not supported"), + }; return self::$dbh->query($query)->fetchAll(PDO::FETCH_COLUMN, 0); } diff --git a/src/Storage/Database/Table.php b/src/Storage/Database/Table.php index 88961e0..d5aa8fa 100644 --- a/src/Storage/Database/Table.php +++ b/src/Storage/Database/Table.php @@ -67,11 +67,17 @@ protected function createTableIfNeeded(): bool */ protected function createTable(): void { + $dbType = self::$dbh->getAttribute(\PDO::ATTR_DRIVER_NAME); + $query = 'CREATE TABLE ' . $this->tableName . ' ('; foreach ($this->fields as $colName => $f) { - $query .= $colName . ' ' . $f->type . ' '; + $type = $f->type; + if (isset($f->constraints[FC::Autoincrement]) && $f->constraints[FC::Autoincrement] && $dbType == 'pgsql') { + $type = 'serial'; + } + $query .= $colName . ' ' . $type . ' '; $constraints = $f->constraints; - if (isset($constraints[FC::Length])) { + if ($type !== 'serial' && isset($constraints[FC::Length])) { $query .= '(' . $f->constraints[FC::Length] . ') '; unset($constraints[FC::Length]); } @@ -84,7 +90,18 @@ protected function createTable(): void break; case FC::Autoincrement: if ($cv) { - $query .= 'AUTOINCREMENT '; + switch ($dbType) { + case 'mysql': + $query .= 'AUTO_INCREMENT '; + break; + case 'pgsql': + break; + case 'sqlite': + $query .= 'AUTOINCREMENT '; + break; + default: + throw new \DomainException("Database type '$dbType' is not supported"); + } } break; case FC::NotNull: @@ -111,7 +128,7 @@ protected function createTable(): void /// @todo figure out how to enforce the fact that the referenced table has been already created foreach ($this->foreignKeys as $fk) { - $query .= 'FOREIGN KEY (' . implode(', ', $fk->columns). ') REFERENCES ' . $fk->parentTable. '(' . + $query .= 'FOREIGN KEY (' . implode(', ', $fk->columns). ') REFERENCES ' . $fk->parentTable . '(' . implode(', ', $fk->parentColumns). ') ON DELETE ' . $fk->onDelete->value . ' ON UPDATE ' . $fk->onUpdate->value . ', '; } From 988a802f152de664ecc88ce70da9946d8ecde274 Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 8 Nov 2024 15:27:28 +0000 Subject: [PATCH 30/38] update readme and remove dead code after cursory testing on mariadb --- README.md | 3 +++ src/Repository/DatabaseRepository.php | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index b9e4e2d..ed0bb63 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,9 @@ This project is licensed under the [Apache License 2.0](LICENSE). - a Redis server - Composer, to install the required dependencies +Note: PostgreSQL and MariaDB >= 10.5 should also work as an alternative to SQLite, but so far they have been tested less +extensively. + ## Installation 1. run `composer install` at the root of the project diff --git a/src/Repository/DatabaseRepository.php b/src/Repository/DatabaseRepository.php index 41dc810..9ef0d32 100644 --- a/src/Repository/DatabaseRepository.php +++ b/src/Repository/DatabaseRepository.php @@ -57,7 +57,6 @@ protected function buildFetchEntityQuery(): string protected function storeEntity($value): null|array { $query = 'insert into ' . $this->tableName . ' ('; - //$vq = ''; $bindCols = []; $autoIncrementCols = []; foreach($this->fields as $colName => $field) { @@ -72,8 +71,6 @@ protected function storeEntity($value): null|array continue; } $bindCols[] = $colName; - //$query .= $colName . ', '; - //$vq .= ":$colName" . ', '; } $query .= implode(', ', $bindCols) . ') values (:' . implode(', :', $bindCols) . ')'; if ($autoIncrementCols) { From f6d405a2e32ba9f648720602039d041630bee563 Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 13 Nov 2024 12:55:19 +0000 Subject: [PATCH 31/38] add forgotten exit calls after redirects are emitted (for logged-in users) --- public/admin/forgotpassword.php | 1 + public/admin/setnewpassword.php | 1 + 2 files changed, 2 insertions(+) diff --git a/public/admin/forgotpassword.php b/public/admin/forgotpassword.php index e2c6fb1..30b952c 100644 --- a/public/admin/forgotpassword.php +++ b/public/admin/forgotpassword.php @@ -21,6 +21,7 @@ $user = $firewall->getUser(); if ($user->isAuthenticated()) { header('Location: ' . $router->generate(__DIR__ . '/resetpassword.php'), true, 303); + exit(); } $form = new ForgotPasswordForm($router->generate(__FILE__)); diff --git a/public/admin/setnewpassword.php b/public/admin/setnewpassword.php index 3ed01dd..a603bd8 100644 --- a/public/admin/setnewpassword.php +++ b/public/admin/setnewpassword.php @@ -18,6 +18,7 @@ $currentUser = $firewall->getUser(); if ($currentUser->isAuthenticated()) { header('Location: ' . $router->generate(__DIR__ . '/resetpassword.php'), true, 303); + exit(); } // as per owasp recommendations - https://cheatsheetseries.owasp.org/cheatsheets/Forgot_Password_Cheat_Sheet.html#url-tokens From fc9f8408c6523d9cd144246faa0789af540368fb Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 13 Nov 2024 13:10:44 +0000 Subject: [PATCH 32/38] allow disabling the forgotpassword feature --- .env | 3 +++ public/admin/forgotpassword.php | 6 ++++++ public/admin/setnewpassword.php | 6 ++++++ resources/templates/admin/login.html.twig | 2 ++ src/EnvVarProcessor.php | 14 ++++++++++++++ src/Security/Firewall.php | 9 +++++++-- 6 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 src/EnvVarProcessor.php diff --git a/.env b/.env index 0e0f4b1..26c29b5 100644 --- a/.env +++ b/.env @@ -32,6 +32,9 @@ WEBSITE=https://crashcollector.veracrypt.fr # Used when sending password-reset emails MAIL_FROM=crashcollector@veracrypt.fr +# Enable/disable the feature to allow users self-service password reset via being sent an email, aka. 'forgot password' +ENABLE_FORGOTPASSWORD=true + LOG_DIR=/var/www/VeraCrypt-CrashCollector/var/logs # The audit log traces user events such as login, password changes, etc AUDIT_LOG_FILE=audit.log diff --git a/public/admin/forgotpassword.php b/public/admin/forgotpassword.php index 30b952c..0a4bb6c 100644 --- a/public/admin/forgotpassword.php +++ b/public/admin/forgotpassword.php @@ -2,6 +2,7 @@ require_once(__DIR__ . '/../../autoload.php'); +use Veracrypt\CrashCollector\EnvVarProcessor; use Veracrypt\CrashCollector\Entity\User; use Veracrypt\CrashCollector\Form\ForgotPasswordForm; use Veracrypt\CrashCollector\Form\ForgotPasswordEmailForm; @@ -17,6 +18,11 @@ $router = new Router(); $tpl = new Templating(); +if (!EnvVarProcessor::bool($_ENV['ENABLE_FORGOTPASSWORD'])) { + header('Location: ' . $router->generate(__DIR__ . '/index.php'), true, 303); + exit(); +} + // if non-anon user, redirect to resetpassword instead of using this form $user = $firewall->getUser(); if ($user->isAuthenticated()) { diff --git a/public/admin/setnewpassword.php b/public/admin/setnewpassword.php index a603bd8..06ba562 100644 --- a/public/admin/setnewpassword.php +++ b/public/admin/setnewpassword.php @@ -2,6 +2,7 @@ require_once(__DIR__ . '/../../autoload.php'); +use Veracrypt\CrashCollector\EnvVarProcessor; use Veracrypt\CrashCollector\Form\ForgotPasswordEmailForm; use Veracrypt\CrashCollector\Form\SetNewPasswordForm; use Veracrypt\CrashCollector\Logger; @@ -14,6 +15,11 @@ $firewall = Firewall::getInstance(); $router = new Router(); +if (!EnvVarProcessor::bool($_ENV['ENABLE_FORGOTPASSWORD'])) { + header('Location: ' . $router->generate(__DIR__ . '/index.php'), true, 303); + exit(); +} + // if non-anon user, redirect to resetpassword instead of using this form $currentUser = $firewall->getUser(); if ($currentUser->isAuthenticated()) { diff --git a/resources/templates/admin/login.html.twig b/resources/templates/admin/login.html.twig index 49345e9..753477b 100644 --- a/resources/templates/admin/login.html.twig +++ b/resources/templates/admin/login.html.twig @@ -18,5 +18,7 @@ {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control'}) }}
+{% if urls.forgotpassword is defined %} Password forgotten? +{% endif %} {% endblock %} diff --git a/src/EnvVarProcessor.php b/src/EnvVarProcessor.php new file mode 100644 index 0000000..6754467 --- /dev/null +++ b/src/EnvVarProcessor.php @@ -0,0 +1,14 @@ + $router->generate(__DIR__ . '/../../public'), 'home' => $router->generate(__DIR__ . '/../../public/admin/index.php'), 'login' => $router->generate(__DIR__ . '/../../public/admin/login.php'), 'logout' => $router->generate(__DIR__ . '/../../public/admin/logout.php'), 'resetpassword' => $router->generate(__DIR__ . '/../../public/admin/resetpassword.php'), - 'forgotpassword' => $router->generate(__DIR__ . '/../../public/admin/forgotpassword.php'), + ]; + if (EnvVarProcessor::bool($_ENV['ENABLE_FORGOTPASSWORD'])) { + $urls['forgotpassword'] = $router->generate(__DIR__ . '/../../public/admin/forgotpassword.php'); + } + return $urls; } } From dc6c99088439eb97bbc2aa3cdbc95243aaad6eb9 Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 15 Nov 2024 14:06:09 +0000 Subject: [PATCH 33/38] modify upload API; allow users to see and delete uploaded data for 1h after upload --- .env | 2 + README.md | 28 +++++++- bin/console | 7 +- public/admin/setnewpassword.php | 1 - public/report/confirm.php | 61 ++++++++++++++++ public/report/upload.php | 69 ++++++++++++++++++ public/upload/index.php | 32 --------- .../templates/parts/forms/macros.html.twig | 12 ++-- resources/templates/report/confirm.html.twig | 35 +++++++++ .../upload.html.twig} | 4 +- src/Entity/CrashReport.php | 1 + src/Entity/ManageReportToken.php | 7 ++ src/Entity/ReportToken.php | 26 +++++++ .../ConstraintReportNotFoundException.php | 7 ++ src/Form/CrashReportBaseForm.php | 26 ++++--- src/Form/CrashReportConfirmForm.php | 68 ++++++++++++++++++ src/Form/CrashReportRemoveForm.php | 41 +++++++++++ src/Form/CrashReportSearchForm.php | 14 ++-- src/Form/CrashReportSubmitForm.php | 21 +++--- src/Form/Field.php | 3 +- .../Constraint/ReportTokenConstraint.php | 71 +++++++++++++++++++ src/Form/Field/DateTime.php | 4 +- src/Form/Field/Email.php | 4 +- src/Form/Field/Password.php | 4 +- src/Form/Field/RateLimiter.php | 4 +- src/Form/Field/Text.php | 4 +- src/Form/Field/TextArea.php | 4 +- src/Form/ForgotPasswordEmailForm.php | 8 +-- src/Form/Form.php | 14 ++++ src/Repository/CrashReportRepository.php | 39 ++++++++-- .../ManageReportTokenRepository.php | 27 +++++++ src/Repository/ReportTokenRepository.php | 43 +++++++++++ 32 files changed, 601 insertions(+), 90 deletions(-) create mode 100644 public/report/confirm.php create mode 100644 public/report/upload.php delete mode 100644 public/upload/index.php create mode 100644 resources/templates/report/confirm.html.twig rename resources/templates/{upload/index.html.twig => report/upload.html.twig} (83%) create mode 100644 src/Entity/ManageReportToken.php create mode 100644 src/Entity/ReportToken.php create mode 100644 src/Exception/ConstraintReportNotFoundException.php create mode 100644 src/Form/CrashReportConfirmForm.php create mode 100644 src/Form/CrashReportRemoveForm.php create mode 100644 src/Form/Field/Constraint/ReportTokenConstraint.php create mode 100644 src/Repository/ManageReportTokenRepository.php create mode 100644 src/Repository/ReportTokenRepository.php diff --git a/.env b/.env index 26c29b5..c847b14 100644 --- a/.env +++ b/.env @@ -34,6 +34,8 @@ MAIL_FROM=crashcollector@veracrypt.fr # Enable/disable the feature to allow users self-service password reset via being sent an email, aka. 'forgot password' ENABLE_FORGOTPASSWORD=true +# Enable/disable the feature to allow uploading crash reports via a browser-based form instead of using API as VeraCrypt does +ENABLE_BROWSER_UPLOAD=false LOG_DIR=/var/www/VeraCrypt-CrashCollector/var/logs # The audit log traces user events such as login, password changes, etc diff --git a/README.md b/README.md index ed0bb63..cb34a10 100644 --- a/README.md +++ b/README.md @@ -62,21 +62,43 @@ extensively. https://cheatsheetseries.owasp.org/cheatsheets/PHP_Configuration_Cheat_Sheet.html - it is recommended to use Redis for php session storage 5. create an administrator user: run the cli command `php ./bin/console user:create --is-superuser ` -6. set up a cronjob (daily or weekly is fine) running the cli command `php ./bin/console forgotpasswordtoken:prune` +6. set up a cronjob (daily or weekly is fine) running the cli command `php ./bin/console token:prune` 7. set up the webserver: - configure the vhost root directory to be the `public` directory. No http access to any other folder please - make sure .php scripts are executed via the php interpreter + - the file to serve when a directory index is requested is `index.php` - no rewrite rules are necessary -8. navigate to `https://your-host/upload/` to upload crash reports; to `https://your-host/admin/` for browsing them +8. navigate to `https://your-host/report/upload.php` to upload crash reports; to `https://your-host/admin/` for browsing them 9. optionally, run the SQLite pragma `journal_mode=WAL` to have optimized performance and concurrency 10. optionally, set up cronjobs to run the SQLite pragmas `optimize` and `integrity_check` ## How it works Once uploaded, crash reports are stored in a SQLite database. They are not available for examination to the public, but -only to users of this web application. +only to registered users of this web application. For a short time after the upload, the submitter of a crash report can +see the uploaded data and is given a chance to remove it if desired. The web interface is kept extremely simple by design. Besides supporting the anonymous upload of the crash reports, it allows application users to browse them and to change their own login password. The only way to manage the application's users accounts (create, remove, update, enable/disable them) is via a command-line script. + +### The upload API + +The interaction between VeraCrypt and the Crash Collector is the following: + +1. VeraCrypt sends a POST request to the url `/report/upload.php` using `application/x-www-form-urlencoded` encoding. + In case of success, a 303 redirection response is returned. + In case of errors with the POST request data, a 400 response is returned, with `plain/text` content tipe, and + error messages displayed one per line. + In case of unexpected / server errors, a 40x or 50x response can also be returned. +2. In case of a successful upload, VeraCrypt should start a browser session and send the user to the redirection target + URL given at step 1. + +Rate limiting is implemented, to avoid spamming of the upload page. + +### Troubleshooting and Debugging + +The name of the fields to submit at step 1 above can be seen by setting `ENABLE_BROWSER_UPLOAD=true` in config. file +`.env.local`, and pointing a browser at the `/report/upload.php` URL. +That results in the display of a crash-report upload form which can be filled in manually. diff --git a/bin/console b/bin/console index 71f2613..51412cb 100755 --- a/bin/console +++ b/bin/console @@ -15,6 +15,7 @@ use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Question\Question; use Symfony\Component\Console\Style\SymfonyStyle; use Veracrypt\CrashCollector\Repository\ForgotPasswordTokenRepository; +use Veracrypt\CrashCollector\Repository\ManageReportTokenRepository; use Veracrypt\CrashCollector\Repository\UserRepository; use Veracrypt\CrashCollector\Security\PasswordHasher; @@ -145,11 +146,13 @@ $application->register('user:update') return Command::SUCCESS; }); -$application->register('forgotpasswordtoken:prune') +$application->register('tokens:prune') ->setCode(function (InputInterface $input, OutputInterface $output): int { $repo = new ForgotPasswordTokenRepository(); $repo->prune(); - $output->writeln("Expired forgotpassword tokens deleted"); + $repo = new ManageReportTokenRepository(); + $repo->prune(); + $output->writeln("Expired tokens deleted: forgotpassword and managereport"); return Command::SUCCESS; }); diff --git a/public/admin/setnewpassword.php b/public/admin/setnewpassword.php index 06ba562..fa87a49 100644 --- a/public/admin/setnewpassword.php +++ b/public/admin/setnewpassword.php @@ -68,7 +68,6 @@ $tpl = new Templating(); echo $tpl->render('admin/setnewpassword.html.twig', [ - //'user' => $tokenUser, 'error' => $errorMessage, 'form' => $form2, 'urls' => $firewall->getAdminUrls(), diff --git a/public/report/confirm.php b/public/report/confirm.php new file mode 100644 index 0000000..33c1239 --- /dev/null +++ b/public/report/confirm.php @@ -0,0 +1,61 @@ +generate(__FILE__)); +if ($form1->isSubmitted()) { + $form1->handleRequest(); + if ($form1->isValid()) { + $report = $form1->getReport(); + $tokenId = $form1->getFieldData('token'); + $secret = $form1->getFieldData('secret'); + } else { + $errorMessage = $form1->errorMessage; + } +} + +$form2 = new CrashReportRemoveForm($router->generate(__FILE__), $tokenId, $secret, $report); +if ($errorMessage === null && $form2->isSubmitted()) { + $form2->handleRequest(); + if ($form2->isValid()) { + // delete the report + $report = $form2->getReport(); + $repository = new CrashReportRepository(); + $repository->deleteReport($report->id); + // 'consume' (remove) the token + $form2->getTokenRepository()->delete($form2->getFieldData('token')); + } +} + +if (!$form1->isSubmitted() && !$form2->isSubmitted()) { + $errorMessage = 'Nothing to see here, move along.'; + + $logger = Logger::getInstance('audit'); + $logger->debug("A request for /report/confirm.php was received, with no form submitted. Hacking attempt?"); +} + +$tpl = new Templating(); +echo $tpl->render('report/confirm.html.twig', [ + 'error' => $errorMessage, + 'report' => $report, + 'form' => $form2, + 'urls' => [ + 'root' => $router->generate(__DIR__ . '/..'), + ], +]); diff --git a/public/report/upload.php b/public/report/upload.php new file mode 100644 index 0000000..6619576 --- /dev/null +++ b/public/report/upload.php @@ -0,0 +1,69 @@ +generate(__FILE__)); +$confirmUrl = null; + +if ($form->isSubmitted()) { + $form->handleRequest(); + if ($form->isValid()) { + /// @todo catch db runtime errors and show a nice error msg such as 'try later' - possibly distinguishing + /// data-related errors and using an appropriate error message + $crr = new CrashReportRepository(); + $report = $crr->createReport(...$form->getData()); + + $mrr = new ManageReportTokenRepository(); + $ph = new PasswordHasher(); + $secret = $ph->generateRandomString($mrr->tokenLength); + $token = $mrr->createToken($report->id, $ph->hash($secret)); + + $confirmUrl = $router->generate(__DIR__ . '/confirm.php', ['tkn' => $token->id, 'sec' => $secret]); + header('Location: ' . $confirmUrl, true, 303); + exit(); + } else { + http_response_code(400); + header('Content-Type: text/plain'); + + $errors = $form->getFieldsErrors(); + array_walk($errors, function(&$value, $key) use ($form) { + $value = $form->getField($key)->label . ': ' . $value; + }); + if ($form->errorMessage != '') { + array_unshift($errors, $form->errorMessage); + } + echo implode("\n", $errors); + + exit(); + } +} + +if (!EnvVarProcessor::bool($_ENV['ENABLE_BROWSER_UPLOAD'])) { + http_response_code(404); + exit(); +} + +// uncomment these lines to allow to pre-fill form fields using a GET request, but only act on POST +//if ($form->isSubmitted($_GET)) { +// $form->handleRequest($_GET); +//} + +$tpl = new Templating(); +echo $tpl->render('report/upload.html.twig', [ + 'form' => $form, + 'urls' => [ + 'root' => $router->generate(__DIR__ . '/..'), + 'confirm' => $confirmUrl + ], +]); diff --git a/public/upload/index.php b/public/upload/index.php deleted file mode 100644 index 74d0d68..0000000 --- a/public/upload/index.php +++ /dev/null @@ -1,32 +0,0 @@ -generate(__FILE__)); - -// allow to pre-fill form fields using GET request, but only act on POST -/// @todo should we change this behaviour to pre-fill the form on POST requests which just miss the Submit button? -/// It might make sense, as the call-stack field is not a good candidate for being serialized in the URL... -if ($form->isSubmitted()) { - $form->handleRequest(); - if ($form->isValid()) { - /// @todo catch db runtime errors and show a nice error msg such as 'try later' - possibly distinguishing - /// data-related errors and using an appropriate error message - $crr = new CrashReportRepository(); - $crr->createReport(...$form->getData()); - } -} elseif ($form->isSubmitted($_GET)) { - $form->handleRequest($_GET); -} - -$tpl = new Templating(); -echo $tpl->render('upload/index.html.twig', [ - 'form' => $form, - 'urls' => ['root' => $router->generate(__DIR__ . '/..')], -]); diff --git a/resources/templates/parts/forms/macros.html.twig b/resources/templates/parts/forms/macros.html.twig index 910ade4..ea41cc8 100644 --- a/resources/templates/parts/forms/macros.html.twig +++ b/resources/templates/parts/forms/macros.html.twig @@ -1,16 +1,16 @@ {% macro input(field, css_classes=[], input_attributes=[]) %} - {# @todo allow better styling of the error message: align it below the input field #} + {# @todo allow better styling of the error message: align it below the input field instead of the label #} {% if field.inputType == 'anticsrf' %} {% elseif field.inputType == 'datetime-local' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'email' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'hidden' %} @@ -21,7 +21,7 @@ {% elseif field.inputType == 'password' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'ratelimiter' %} @@ -29,12 +29,12 @@ {% elseif field.inputType == 'text' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% elseif field.inputType == 'textarea' %} -
+
{% if field.errorMessage != '' %}

{{ field.errorMessage }}

{% endif %} {% else %} diff --git a/resources/templates/report/confirm.html.twig b/resources/templates/report/confirm.html.twig new file mode 100644 index 0000000..c25c456 --- /dev/null +++ b/resources/templates/report/confirm.html.twig @@ -0,0 +1,35 @@ +{% extends "base.html.twig" %} + +{% block content %} + {% import "parts/forms/macros.html.twig" as forms %} + {% if error != '' %} +

{{ error }}

+ {% elseif form.isSubmitted() and form.isValid() %} +

The crash report has been deleted

+ {% else %} +
+

This is the Crash Report that you have just uploaded:

+

NB please use the Report Id for all future communications

+ {% if form.errorMessage|default('') != '' %} +

{{ form.errorMessage }}

+ {% endif %} +
+ {{ forms.input(form.getField('id'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }} +
+
+ {{ forms.input(form.getField('reported'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }} +
+ {{ include('parts/forms/cr_common_fields.html.twig') }} +
+ {{ forms.input(form.getField('callStack'), {'label': 'col-sm-2 col-form-label', 'div': 'col-sm-10', 'input': 'form-control'}, {'spellcheck': 'false'}) }} +
+
+ {{ forms.input(form.getField('token')) }} + {{ forms.input(form.getField('secret')) }} +
If you are not happy with it, you can delete it now. Otherwise, there is nothing else that you need to do.
+ {{ forms.input(form.getSubmit(), {'input': 'btn btn-primary form-control', 'div': 'col-sm-4'}) }} +
+
+ {% endif %} + +{% endblock %} diff --git a/resources/templates/upload/index.html.twig b/resources/templates/report/upload.html.twig similarity index 83% rename from resources/templates/upload/index.html.twig rename to resources/templates/report/upload.html.twig index bc6495a..75eeb3d 100644 --- a/resources/templates/upload/index.html.twig +++ b/resources/templates/report/upload.html.twig @@ -3,7 +3,9 @@ {% block content %} {% import "parts/forms/macros.html.twig" as forms %} {% if form.isSubmitted() and form.isValid() %} -

Thank you for taking the time to submit the information and sorry for your hassles.

+

Thank you for taking the time to submit the information and sorry for your hassles.
+ You can see, and remove, your submission following this link, within the next hour. +

{% else %}
{% if form.errorMessage|default('') != '' %} diff --git a/src/Entity/CrashReport.php b/src/Entity/CrashReport.php index 9fdb5f6..5e3ce48 100644 --- a/src/Entity/CrashReport.php +++ b/src/Entity/CrashReport.php @@ -15,6 +15,7 @@ class CrashReport private int $dateReported; public function __construct( + public readonly ?int $id, int|DateTimeInterface $dateReported, public readonly string $programVersion, public readonly string $osVersion, diff --git a/src/Entity/ManageReportToken.php b/src/Entity/ManageReportToken.php new file mode 100644 index 0000000..657b7bf --- /dev/null +++ b/src/Entity/ManageReportToken.php @@ -0,0 +1,7 @@ +fetchReport($this->reportId); + } +} diff --git a/src/Exception/ConstraintReportNotFoundException.php b/src/Exception/ConstraintReportNotFoundException.php new file mode 100644 index 0000000..27adb29 --- /dev/null +++ b/src/Exception/ConstraintReportNotFoundException.php @@ -0,0 +1,7 @@ +fields = [ + parent::__construct($actionUrl); + $this->fields = $this->getFieldsDefinitions($actionUrl, $report); + } + + protected function getFieldsDefinitions(string $actionUrl, ?CrashReport $report = null): array + { + return [ /// @todo get the field lengths from the Repo fields - 'programVersion' => new Field\Text('Program version', 'pv', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'osVersion' => new Field\Text('OS version', 'ov', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'hwArchitecture' => new Field\Text('Architecture', 'ha', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'executableChecksum' => new Field\Text('Executable checksum', 'ck', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'errorCategory' => new Field\Text('Error category', 'ec', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), - 'errorAddress' => new Field\Text('Error address', 'ea', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255]), + 'programVersion' => new Field\Text('Program version', 'pv', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255], $report?->programVersion , $this->isReadOnly), + 'osVersion' => new Field\Text('OS version', 'ov', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255], $report?->osVersion, $this->isReadOnly), + 'hwArchitecture' => new Field\Text('Architecture', 'ha', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255], $report?->hwArchitecture, $this->isReadOnly), + 'executableChecksum' => new Field\Text('Executable checksum', 'ck', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255], $report?->executableChecksum, $this->isReadOnly), + 'errorCategory' => new Field\Text('Error category', 'ec', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255], $report?->errorCategory, $this->isReadOnly), + 'errorAddress' => new Field\Text('Error address', 'ea', [FC::Required => $this->requireAllFieldsByDefault, FC::MaxLength => 255], $report?->errorAddress, $this->isReadOnly), ]; - - parent::__construct($actionUrl); } } diff --git a/src/Form/CrashReportConfirmForm.php b/src/Form/CrashReportConfirmForm.php new file mode 100644 index 0000000..f5ce242 --- /dev/null +++ b/src/Form/CrashReportConfirmForm.php @@ -0,0 +1,68 @@ +fields = $this->getFieldsDefinitions($actionUrl, $report, $tokenId, $secret); + } + + protected function getFieldsDefinitions(string $actionUrl, ?CrashReport $report = null, ?int $tokenId = null, #[\SensitiveParameter] ?string $secret = null): array + { + $this->reportConstraint = new ReportTokenConstraint(ManageReportTokenRepository::class); + return [ + 'token' => new Field\Hidden('tkn', [ + /// @todo add an is-integer constraint? + FC::Required => true, + FC::RateLimit => new RateLimiter([ + new FixedWindow($actionUrl, 10, 300), // equivalent to once every 30 secs + new FixedWindow($actionUrl, 12, 3600), // equivalent to once every 5 minutes + new FixedWindow($actionUrl, 120, 86400), // equivalent to once every 12 minutes + ]), + FC::Custom => $this->reportConstraint + ], $tokenId), + /// @todo get the field length from the TokenRepository + 'secret' => new Field\Hidden('sec', [ + FC::Required => true, + FC::MinLength => 64, + FC::MaxLength => 64, + ], $secret), + ]; + } + + protected function validateSubmit(?array $request = null): void + { + // use the same error message used for invalid token-ids + if (!$this->reportConstraint->validateHash($this->getFieldData('secret'))) { + $this->setError("Token not found"); + } + } + + public function isSubmitted(?array $request = null): bool + { + if ($request === null) { + $request = $this->getRequest(); + } + return array_key_exists('tkn', $request); + } + + public function getReport(): null|CrashReport + { + return $this->reportConstraint->getReport(); + } +} diff --git a/src/Form/CrashReportRemoveForm.php b/src/Form/CrashReportRemoveForm.php new file mode 100644 index 0000000..c56986d --- /dev/null +++ b/src/Form/CrashReportRemoveForm.php @@ -0,0 +1,41 @@ + new Field\Text('Report Id', 'id', [FC::Required => $this->requireAllFieldsByDefault], $report?->id, $this->isReadOnly), + 'reported' => new Field\Text('Date', 'dt', [FC::Required => $this->requireAllFieldsByDefault], $report ? date('Y-m-d H:i:s', $report->dateReported) : null, $this->isReadOnly), + ], + CrashReportBaseForm::getFieldsDefinitions($actionUrl, $report), + [ + 'callStack' => new Field\TextArea('Call stack', 'cs', [FC::Required => $this->requireAllFieldsByDefault], $report?->callStack, $this->isReadOnly), + ], + parent::getFieldsDefinitions($actionUrl, $report, $tokenId, $secret), + ); + } + + public function isSubmitted(?array $request = null): bool + { + return CrashReportBaseForm::isSubmitted($request); + } + + public function getTokenRepository(): ReportTokenRepository + { + return $this->reportConstraint->getTokenRepository(); + } +} diff --git a/src/Form/CrashReportSearchForm.php b/src/Form/CrashReportSearchForm.php index a2ef0e6..b3ce35b 100644 --- a/src/Form/CrashReportSearchForm.php +++ b/src/Form/CrashReportSearchForm.php @@ -2,6 +2,8 @@ namespace Veracrypt\CrashCollector\Form; +use Veracrypt\CrashCollector\Entity\CrashReport; + /** * @todo we could implement custom validation checks in handleRequest */ @@ -10,10 +12,14 @@ class CrashReportSearchForm extends CrashReportBaseForm protected string $submitLabel = 'Search'; protected int $submitOn = self::ON_GET; - public function __construct(string $actionUrl) + protected function getFieldsDefinitions(string $actionUrl, ?CrashReport $report = null): array { - parent::__construct($actionUrl); - $this->fields['minDate'] = new Field\DateTime('After', 'da'); - $this->fields['maxDate'] = new Field\DateTime('Before', 'db'); + return array_merge( + parent::getFieldsDefinitions($actionUrl, $report), + [ + 'minDate' => new Field\DateTime('After', 'da'), + 'maxDate' => new Field\DateTime('Before', 'db'), + ] + ); } } diff --git a/src/Form/CrashReportSubmitForm.php b/src/Form/CrashReportSubmitForm.php index 8e70908..c2341b8 100644 --- a/src/Form/CrashReportSubmitForm.php +++ b/src/Form/CrashReportSubmitForm.php @@ -2,6 +2,7 @@ namespace Veracrypt\CrashCollector\Form; +use Veracrypt\CrashCollector\Entity\CrashReport; use Veracrypt\CrashCollector\Form\FieldConstraint as FC; use Veracrypt\CrashCollector\RateLimiter\Constraint\FixedWindow; @@ -9,14 +10,18 @@ class CrashReportSubmitForm extends CrashReportBaseForm { protected bool $requireAllFieldsByDefault = true; - public function __construct(string $actionUrl) + protected function getFieldsDefinitions(string $actionUrl, ?CrashReport $report = null): array { - parent::__construct($actionUrl); - $this->fields['callStack'] = new Field\TextArea('Call stack', 'cs', [FC::Required => $this->requireAllFieldsByDefault]); - $this->fields['rateLimit'] = new Field\RateLimiter([ - new FixedWindow($actionUrl, 1, 30), // equivalent to once every 30 secs - new FixedWindow($actionUrl, 12, 3600), // equivalent to once every 5 minutes - new FixedWindow($actionUrl, 24, 86400), // equivalent to once every 30 minutes - ]); + return array_merge( + parent::getFieldsDefinitions($actionUrl, $report), + [ + 'callStack' => new Field\TextArea('Call stack', 'cs', [FC::Required => $this->requireAllFieldsByDefault], null, $this->isReadOnly), + 'rateLimit' => new Field\RateLimiter([ + new FixedWindow($actionUrl, 1, 30), // equivalent to once every 30 secs + new FixedWindow($actionUrl, 12, 3600), // equivalent to once every 5 minutes + new FixedWindow($actionUrl, 24, 86400), // equivalent to once every 30 minutes + ]), + ] + ); } } diff --git a/src/Form/Field.php b/src/Form/Field.php index dd59a42..5f33ce0 100644 --- a/src/Form/Field.php +++ b/src/Form/Field.php @@ -26,7 +26,8 @@ protected function __construct( public readonly string $inputName, public readonly array $constraints = [], protected mixed $value = null, - public readonly bool $isVisible = true + public readonly bool $isVisible = true, + public readonly bool $isReadonly = false ) { $this->validateConstraintsDefinitions($constraints); diff --git a/src/Form/Field/Constraint/ReportTokenConstraint.php b/src/Form/Field/Constraint/ReportTokenConstraint.php new file mode 100644 index 0000000..04d208b --- /dev/null +++ b/src/Form/Field/Constraint/ReportTokenConstraint.php @@ -0,0 +1,71 @@ +repositoryClass(); + return $ph->verify($this->token->hash, $secret); + } + + /** + * @throws \RuntimeException or subclasses thereof + */ + public function validateRequest(?string $value = null): void + { + /** @var ReportTokenRepository $repo */ + $repo = new $this->repositoryClass(); + + $tokenId = (int)$value; + if ($tokenId <= 0) { + throw new TokenNotFoundException('Token not found'); + } + $token = $repo->fetch($tokenId); + if ($token === null) { + throw new TokenNotFoundException('Token not found'); + } + $this->token = $token; + $report = $this->token->getReport(); + if ($report === null) { + throw new ConstraintReportNotFoundException('Report matching token not found'); + } + + $this->report = $report; + } + + public function getTokenRepository(): ReportTokenRepository + { + return new $this->repositoryClass(); + } + + public function getReport(): null|CrashReport + { + return $this->report; + } +} diff --git a/src/Form/Field/DateTime.php b/src/Form/Field/DateTime.php index 9c34d84..5928e61 100644 --- a/src/Form/Field/DateTime.php +++ b/src/Form/Field/DateTime.php @@ -9,9 +9,9 @@ */ class DateTime extends Basefield { - public function __construct(string $label, string $inputName, array $constraints = [], mixed $value = null) + public function __construct(string $label, string $inputName, array $constraints = [], mixed $value = null, bool $isReadOnly = false) { - parent::__construct('datetime-local', $label, $inputName, $constraints, $value); + parent::__construct('datetime-local', $label, $inputName, $constraints, $value, true, $isReadOnly); } /** diff --git a/src/Form/Field/Email.php b/src/Form/Field/Email.php index ce0c430..9ab810e 100644 --- a/src/Form/Field/Email.php +++ b/src/Form/Field/Email.php @@ -6,9 +6,9 @@ class Email extends Basefield { - public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null) + public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null, bool $isReadOnly = false) { - parent::__construct('email', $label, $inputName, $constraints, $value); + parent::__construct('email', $label, $inputName, $constraints, $value, true, $isReadOnly); } protected function validateValue(mixed $value): null|string diff --git a/src/Form/Field/Password.php b/src/Form/Field/Password.php index 2c55b75..f1d4888 100644 --- a/src/Form/Field/Password.php +++ b/src/Form/Field/Password.php @@ -9,8 +9,8 @@ */ class Password extends Basefield { - public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null) + public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null, bool $isReadOnly = false) { - parent::__construct('password', $label, $inputName, $constraints, $value); + parent::__construct('password', $label, $inputName, $constraints, $value, true, $isReadOnly); } } diff --git a/src/Form/Field/RateLimiter.php b/src/Form/Field/RateLimiter.php index d305c9d..6ac73b9 100644 --- a/src/Form/Field/RateLimiter.php +++ b/src/Form/Field/RateLimiter.php @@ -21,9 +21,7 @@ class RateLimiter extends BaseField /** * @var RateLimiterInterface[] $constraints */ - public function __construct( - array $constraints - ) + public function __construct(array $constraints) { parent::__construct('ratelimiter', '', '', [], null, false); diff --git a/src/Form/Field/Text.php b/src/Form/Field/Text.php index 640aa90..edc6b69 100644 --- a/src/Form/Field/Text.php +++ b/src/Form/Field/Text.php @@ -6,8 +6,8 @@ class Text extends Basefield { - public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null) + public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null, bool $isReadOnly = false) { - parent::__construct('text', $label, $inputName, $constraints, $value); + parent::__construct('text', $label, $inputName, $constraints, $value, true, $isReadOnly); } } diff --git a/src/Form/Field/TextArea.php b/src/Form/Field/TextArea.php index 8972063..7cb1a3d 100644 --- a/src/Form/Field/TextArea.php +++ b/src/Form/Field/TextArea.php @@ -6,8 +6,8 @@ class TextArea extends Basefield { - public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null) + public function __construct(string $label, string $inputName, array $constraints = [], ?string $value = null, bool $isReadOnly = false) { - parent::__construct('textarea', $label, $inputName, $constraints, $value); + parent::__construct('textarea', $label, $inputName, $constraints, $value, true, $isReadOnly); } } diff --git a/src/Form/ForgotPasswordEmailForm.php b/src/Form/ForgotPasswordEmailForm.php index 5c322e2..22fb592 100644 --- a/src/Form/ForgotPasswordEmailForm.php +++ b/src/Form/ForgotPasswordEmailForm.php @@ -42,11 +42,9 @@ public function __construct(string $actionUrl, ?int $tokenId = null, #[\Sensitiv protected function validateSubmit(?array $request = null): void { - if ($this->isValid) { - // use the same error message used for invalid token-ids - if (!$this->userConstraint->validateHash($this->getFieldData('secret'))) { - $this->setError("Token not found"); - } + // use the same error message used for invalid token-ids + if (!$this->userConstraint->validateHash($this->getFieldData('secret'))) { + $this->setError("Token not found"); } } diff --git a/src/Form/Form.php b/src/Form/Form.php index 924a35f..5a74a3d 100644 --- a/src/Form/Form.php +++ b/src/Form/Form.php @@ -124,6 +124,20 @@ public function getData(): array return $data; } + /** + * @return string[] key: field name + */ + public function getFieldsErrors($onlyVisibleFields = true): array + { + $errors = []; + foreach($this->fields as $name => $field) { + if (($field->errorMessage !== '' && $field->errorMessage !== null) && ($field->isVisible || !$onlyVisibleFields)) { + $errors[$name] = $field->errorMessage; + } + } + return $errors; + } + public function getQueryStringParts(bool $includeSubmit = false) { if ($this->submitOn == self::ON_POST) { diff --git a/src/Repository/CrashReportRepository.php b/src/Repository/CrashReportRepository.php index 4bc54fa..df6d877 100644 --- a/src/Repository/CrashReportRepository.php +++ b/src/Repository/CrashReportRepository.php @@ -19,7 +19,7 @@ public function __construct() { /// @todo add a 'hash' column as PK instead of the ID? If so, it could/should probably include the source IP too... $this->fields = [ - 'id' => new Field(null, 'integer', [FC::NotNull => true, FC::PK => true, FC::Autoincrement => true]), + 'id' => new Field('id', 'integer', [FC::NotNull => true, FC::PK => true, FC::Autoincrement => true]), 'date_reported' => new Field('dateReported', 'integer', [FC::NotNull => true]), 'program_version' => new Field('programVersion', 'varchar', [FC::Length => 255, FC::NotNull => true]), 'os_version' => new Field('osVersion', 'varchar', [FC::Length => 255, FC::NotNull => true]), @@ -51,10 +51,41 @@ public function createReport(string $programVersion, string $osVersion, string $ string $errorCategory, string $errorAddress, string $callStack): CrashReport { $dateReported = time(); - $cr = new CrashReport($dateReported, $programVersion, $osVersion, $hwArchitecture, $executableChecksum, $errorCategory, + $cr = new CrashReport(null, $dateReported, $programVersion, $osVersion, $hwArchitecture, $executableChecksum, $errorCategory, $errorAddress, $callStack); - $this->storeEntity($cr); - return $cr; + $autoincrements = $this->storeEntity($cr); + // we have to create a new entity object in order to inject the id into it + return new CrashReport($autoincrements['id'], $dateReported, $programVersion, $osVersion, $hwArchitecture, + $executableChecksum, $errorCategory, $errorAddress, $callStack); + } + + /** + * @throws \PDOException + */ + public function fetchReport(int $id): CrashReport|null + { + $query = $this->buildFetchEntityQuery() . ' where id = :id'; + $stmt = self::$dbh->prepare($query); + $stmt->bindValue(':id', $id); + $stmt->execute(); + $result = $stmt->fetch(\PDO::FETCH_ASSOC); + return $result ? new CrashReport(...$result) : null; + } + + /** + * @throws \PDOException + */ + public function deleteReport(int $id): bool + { + $query = 'delete from ' . $this->tableName . ' where id = :id'; + $stmt = self::$dbh->prepare($query); + $stmt->bindValue(':id', $id); + $stmt->execute(); + $deleted = (bool)$stmt->rowCount(); + //if ($deleted) { + // $this->logger->debug("Report '$id' was deleted"); + //} + return $deleted; } /** diff --git a/src/Repository/ManageReportTokenRepository.php b/src/Repository/ManageReportTokenRepository.php new file mode 100644 index 0000000..21401f6 --- /dev/null +++ b/src/Repository/ManageReportTokenRepository.php @@ -0,0 +1,27 @@ + new Field('reportId', 'integer', [FC::NotNull => true]), + ]); + } + + protected function getForeignKeyDefinitions(): array + { + return [ + /// @todo make 'crash_report' a static var or class const of CrashReportRepository, so that we can grab it from there + new ForeignKey(['report_id'], 'crash_report', ['id'], ForeignKeyAction::Cascade, ForeignKeyAction::Cascade), + ]; + } + + public function createToken(string $reportId, string $hash): ReportToken + { + $args['id'] = null; + $args['hash'] = $hash; + $args['reportId'] = $reportId; + $args['dateCreated'] = time(); + $args['expirationDate'] = $this->newTokenExpirationDate(); + $token = new $this->entityClass(...$args); + $autoincrements = $this->storeEntity($token); + // we have to create a new entity object in order to inject the id into it + $args['id'] = $autoincrements['id']; + return new $this->entityClass(...$args); + } +} From c863de3e5715855e0241ec3581bada0ed050a9e6 Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 15 Nov 2024 14:06:37 +0000 Subject: [PATCH 34/38] disable emulated prepared statements to fix running on mariadb --- src/Storage/Database.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Storage/Database.php b/src/Storage/Database.php index c21ab7b..7a72c54 100644 --- a/src/Storage/Database.php +++ b/src/Storage/Database.php @@ -19,6 +19,9 @@ protected function connect(): void self::$dbh = new PDO($_ENV['DB_DSN'], $_ENV['DB_USER'], $_ENV['DB_PASSWORD']); self::$dbh->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC); + // this is necessary for ex. for queries using bound params for offset, limit on mariadb... + self::$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); + $dbType = self::$dbh->getAttribute(PDO::ATTR_DRIVER_NAME); switch ($dbType) { From 8fccabb14718cfad99f6aec21d8d7a8ffe453617 Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 15 Nov 2024 14:29:24 +0000 Subject: [PATCH 35/38] update docs --- CONTRIBUTING.md | 284 +++++++++++++++++++++++++----------------------- README.md | 2 + 2 files changed, 150 insertions(+), 136 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c8b4a71..1e2dd71 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,136 +1,148 @@ -# Contributing to VeraCrypt Crash Collector - -Thank you for considering contributing to VeraCrypt Crash Collector! Your contributions help improve the project, and we appreciate your effort. The following guidelines will assist you through the contribution process. - -## Getting Started - -### 1. Fork the Repository - -- Navigate to the [VeraCrypt-CrashCollector](https://github.com/veracrypt/VeraCrypt-CrashCollector) repository and click "Fork." -- Clone your fork locally: - ```bash - git clone https://github.com/your-username/VeraCrypt-CrashCollector.git - ``` -- Set up the upstream remote to keep your fork up-to-date with the original repository: - ```bash - git remote add upstream https://github.com/veracrypt/VeraCrypt-CrashCollector.git - ``` - -### 2. Set Up Your Development Environment - -Ensure you have the required tools installed to run a PHP web application. - -- **PHP**: Make sure you have PHP installed on your system. -- **Web Server**: Use a local web server like Apache or Nginx, or use the built-in PHP development server: - ```bash - php -S localhost:8000 - ``` - -### 3. Create a New Branch - -Before you start working, create a new branch for your changes: -```bash -git checkout -b feature/your-feature-name -``` - -Use a clear, descriptive name for your branch, such as `fix/issue-123` or `feature/new-feature`. - -### 4. Make Your Changes - -Make your changes in the new branch. Be sure to: - -- Follow the **coding standards** and existing conventions. -- Write **clear, concise comments** where necessary. -- Add or update **tests** if you are adding new functionality. -- Regularly run the project to ensure everything is working. - -### 5. Test Your Changes - -Run the application locally to ensure your changes work using your preferred PHP development setup. - -### 6. Commit Your Changes - -After making sure everything is working, commit your changes with a meaningful message: -```bash -git commit -m "Fix issue with crash report handling in macOS" -``` - -Try to keep your commits small and focused on a specific change. - -### 7. Push to Your Fork - -Push your changes to your fork on GitHub: -```bash -git push origin feature/your-feature-name -``` - -### 8. Create a Pull Request (PR) - -Once your changes are pushed, open a Pull Request (PR) in the original repository: - -1. Go to the [Pull Requests](https://github.com/veracrypt/VeraCrypt-CrashCollector/pulls) section. -2. Click "New Pull Request." -3. Choose your branch and provide a descriptive title and detailed description of your changes. - -Make sure to link to any relevant issues using `Fixes #issue_number` in the description. This will automatically close the linked issue when the PR is merged. - -## Code Reviews - -All PRs are subject to review by maintainers or other contributors. Please: - -- Be open to feedback. -- Address requested changes promptly. -- Participate in discussions if necessary. - -Reviewing ensures code quality, consistency, and alignment with project goals. Don't hesitate to ask for clarification if you're unsure about any feedback. - -## Contribution Guidelines - -### Bug Reports - -If you encounter a bug, please submit an issue to help us investigate: - -- **Title**: A concise description of the issue. -- **Steps to Reproduce**: A detailed list of steps to reproduce the bug. -- **Expected Behavior**: What should have happened. -- **Actual Behavior**: What actually happened, including error messages if applicable. -- **Versions**: The VeraCrypt version and the OS version (Linux/macOS) you are using. -- **Logs or Crash Reports**: Attach relevant logs or crash reports, if available. - -### Feature Requests - -We welcome new feature suggestions! If you have an idea, submit an issue labeled "feature request" with the following details: - -- **Use Case**: Why this feature is needed. -- **Proposed Solution**: A description of how it might work. -- **Alternatives Considered**: Other possible approaches (if applicable). - -### Coding Standards - -- Follow the **existing code style** and patterns. -- Always include **descriptive comments** in your code. -- Write **unit tests** for new features or bug fixes when applicable. -- Ensure your changes do not break existing functionality. - -### Commit Guidelines - -- Keep commits small and focused. -- Use descriptive commit messages, following this format: - - **fix**: for bug fixes. - - **feat**: for new features. - - **docs**: for documentation changes. - - **refactor**: for code improvements. - - **test**: for test changes or additions. - -Example commit message: -``` -feat: add crash report parsing for Linux -``` - -## License - -By contributing to VeraCrypt Crash Collector, you agree that your contributions will be licensed under the [Apache License 2.0](LICENSE). - ---- - -Thank you for contributing! We look forward to collaborating with you. +# Contributing to VeraCrypt Crash Collector + +Thank you for considering contributing to VeraCrypt Crash Collector! Your contributions help improve the project, and we +appreciate your effort. The following guidelines will assist you through the contribution process. + +## Getting Started + +### 1. Fork the Repository + +- Navigate to the [VeraCrypt-CrashCollector](https://github.com/veracrypt/VeraCrypt-CrashCollector) repository and click "Fork." +- Clone your fork locally: + ```bash + git clone https://github.com/your-username/VeraCrypt-CrashCollector.git + ``` +- Set up the upstream remote to keep your fork up-to-date with the original repository: + ```bash + git remote add upstream https://github.com/veracrypt/VeraCrypt-CrashCollector.git + ``` + +### 2. Set Up Your Development Environment + +Ensure you have the required tools installed to run a PHP web application. + +- **PHP**: Make sure you have PHP installed on your system. +- **Redis**: Make sure you have a Redis server installed on your system or reachable from it +- **Web Server**: Use a local web server like Apache or Nginx, or use the built-in PHP development server: + ```bash + php -S localhost:8000 + ``` + +### 3. Create a New Branch + +Before you start working, create a new branch for your changes: +```bash +git checkout -b feature/your-feature-name +``` + +Use a clear, descriptive name for your branch, such as `fix/issue-123` or `feature/new-feature`. + +### 4. Make Your Changes + +Make your changes in the new branch. Be sure to: + +- Follow the **coding standards** and existing conventions. +- Write **clear, concise comments** where necessary. +- Add or update **tests** if you are adding new functionality. +- Regularly run the project to ensure everything is working. + +### 5. Test Your Changes + +Run the application locally to ensure your changes work using your preferred PHP development setup. + +### 6. Commit Your Changes + +After making sure everything is working, commit your changes with a meaningful message: +```bash +git commit -m "Fix issue with crash report handling in macOS" +``` + +Try to keep your commits small and focused on a specific change. + +### 7. Push to Your Fork + +Push your changes to your fork on GitHub: +```bash +git push origin feature/your-feature-name +``` + +### 8. Create a Pull Request (PR) + +Once your changes are pushed, open a Pull Request (PR) in the original repository: + +1. Go to the [Pull Requests](https://github.com/veracrypt/VeraCrypt-CrashCollector/pulls) section. +2. Click "New Pull Request." +3. Choose your branch and provide a descriptive title and detailed description of your changes. + +Make sure to link to any relevant issues using `Fixes #issue_number` in the description. This will automatically close the linked issue when the PR is merged. + +## Code Reviews + +All PRs are subject to review by maintainers or other contributors. Please: + +- Be open to feedback. +- Address requested changes promptly. +- Participate in discussions if necessary. + +Reviewing ensures code quality, consistency, and alignment with project goals. Don't hesitate to ask for clarification if you're unsure about any feedback. + +## Contribution Guidelines + +### Bug Reports + +If you encounter a bug, please submit an issue to help us investigate: + +- **Title**: A concise description of the issue. +- **Steps to Reproduce**: A detailed list of steps to reproduce the bug. +- **Expected Behavior**: What should have happened. +- **Actual Behavior**: What actually happened, including error messages if applicable. +- **Versions**: The VeraCrypt version and the OS version (Linux/macOS) you are using. +- **Logs or Crash Reports**: Attach relevant logs or crash reports, if available. + +### Feature Requests + +We welcome new feature suggestions! If you have an idea, submit an issue labeled "feature request" with the following details: + +- **Use Case**: Why this feature is needed. +- **Proposed Solution**: A description of how it might work. +- **Alternatives Considered**: Other possible approaches (if applicable). + +### Design Guidelines + +- Reduce external dependencies as much as possible. Ideally, this package should not depend on any external library + or service +- Security is paramount +- Use strict typing whenever possible +- + +### Coding Standards + +- Follow the **existing code style** and patterns. + Code formatting rules are specified in the `.editorconfig` file. + HTML styling is based on Bootstrap, version 5.3. +- Always include **descriptive comments** in your code. +- Write either **unit tests** or **functional tests** for new features or bug fixes when applicable. +- Ensure your changes do not break existing functionality. + +### Commit Guidelines + +- Keep commits small and focused. +- Use descriptive commit messages, following this format: + - **fix**: for bug fixes. + - **feat**: for new features. + - **docs**: for documentation changes. + - **refactor**: for code improvements. + - **test**: for test changes or additions. + +Example commit message: +``` +feat: add crash report parsing for Linux +``` + +## License + +By contributing to VeraCrypt Crash Collector, you agree that your contributions will be licensed under the [Apache License 2.0](LICENSE). + +--- + +Thank you for contributing! We look forward to collaborating with you. diff --git a/README.md b/README.md index cb34a10..97ae3fb 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,8 @@ after VeraCrypt detects a crash has occurred. The collected crash reports provide vital information to improve the stability and performance of VeraCrypt by helping to identify and resolve issues. +Similar projects are f.e. https://github.com/tdf/crash-srv. + ## Crash Reporting Mechanism When a crash occurs, the following information is gathered by the crash reporting system: From d341cf765d1b6d5e752c24fde991cb44768bb2b0 Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 15 Nov 2024 16:25:30 +0000 Subject: [PATCH 36/38] feat: allow using urls which omit the trailing .php --- .env | 8 ++++++++ README.md | 30 +++++++++++++++++++++++++----- src/Router.php | 4 ++-- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/.env b/.env index c847b14..20998a9 100644 --- a/.env +++ b/.env @@ -32,6 +32,14 @@ WEBSITE=https://crashcollector.veracrypt.fr # Used when sending password-reset emails MAIL_FROM=crashcollector@veracrypt.fr +# Set to true to make the app generate urls such as `/admin/` instead of `/admin/index.php`. +# NB: this requires matching webserver configuration, such as `index index.php` for Nginx +URLS_STRIP_INDEX_DOT_PHP=false +# Set to true to make the app generate urls such as `/report/upload` instead of `/report/upload.php`. +# NB: this requires matching webserver configuration, see f.e. +# https://serverfault.com/questions/761627/nginx-rewrite-to-remove-php-from-files-has-no-effect-but-to-redirect-to-homepag +URLS_STRIP_PHP_EXTENSION=false + # Enable/disable the feature to allow users self-service password reset via being sent an email, aka. 'forgot password' ENABLE_FORGOTPASSWORD=true # Enable/disable the feature to allow uploading crash reports via a browser-based form instead of using API as VeraCrypt does diff --git a/README.md b/README.md index 97ae3fb..56d8544 100644 --- a/README.md +++ b/README.md @@ -62,18 +62,38 @@ extensively. - for a production installation, it is recommended to follow the owasp guidelines available at https://cheatsheetseries.owasp.org/cheatsheets/PHP_Configuration_Cheat_Sheet.html - - it is recommended to use Redis for php session storage + - it is recommended to use Redis for php session storage instead of the default file-based storage 5. create an administrator user: run the cli command `php ./bin/console user:create --is-superuser ` 6. set up a cronjob (daily or weekly is fine) running the cli command `php ./bin/console token:prune` 7. set up the webserver: - configure the vhost root directory to be the `public` directory. No http access to any other folder please - make sure .php scripts are executed via the php interpreter - - the file to serve when a directory index is requested is `index.php` + - the file to serve when a directory index is requested should be `index.php` - no rewrite rules are necessary -8. navigate to `https://your-host/report/upload.php` to upload crash reports; to `https://your-host/admin/` for browsing them -9. optionally, run the SQLite pragma `journal_mode=WAL` to have optimized performance and concurrency -10. optionally, set up cronjobs to run the SQLite pragmas `optimize` and `integrity_check` +8. navigate to `https://your-host/report/upload.php` to upload crash reports; to `https://your-host/admin/index.php` for browsing them + +### Advanced configuration + +* Removing `.php` from the URLs used by the application + + In order to have the application use "php-less" URLs, you have to 1. set up the webserver so that it will try to + pass requests for URLs not ending in `.php` to the php interpreter, and 2. configure the application accordingly. + + Point 1 can be done, for Nginx, following f.e. the instructions at + https://serverfault.com/questions/761627/nginx-rewrite-to-remove-php-from-files-has-no-effect-but-to-redirect-to-homepag + + For point 2, add `URLS_STRIP_INDEX_DOT_PHP=true` and `URLS_STRIP_PHP_EXTENSION=true` to file `.env.local` + +* Optimizing SQLite performance and scalability + + Optionally, run the SQLite pragma `journal_mode=WAL` to have optimized performance and concurrency + + Optionally, set up cronjobs to run the SQLite pragmas `optimize` and `integrity_check` + +* Using Redis for PHP session storage + + Google is your friend - there are countless guides for this. ## How it works diff --git a/src/Router.php b/src/Router.php index 62a51b6..d75e0e9 100644 --- a/src/Router.php +++ b/src/Router.php @@ -11,11 +11,11 @@ class Router public function __construct() { - /// @todo allow setting $stripPhpExtension and $stripIndexDotPhp from $_ENV - $this->rootUrl = $_ENV['ROOT_URL']; // nb: realpath trims the trailing slash $this->rootDir = realpath(__DIR__ . '/../public/'); + $this->stripPhpExtension = EnvVarProcessor::bool($_ENV['URLS_STRIP_PHP_EXTENSION']); + $this->stripIndexDotPhp = EnvVarProcessor::bool($_ENV['URLS_STRIP_INDEX_DOT_PHP']); } /** From 3b683c4e0602974640187876cfacba06ad04c6d3 Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 15 Nov 2024 16:26:29 +0000 Subject: [PATCH 37/38] docs: add a comment about a possible php warning --- src/Storage/Session.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Storage/Session.php b/src/Storage/Session.php index 48064cf..4279923 100644 --- a/src/Storage/Session.php +++ b/src/Storage/Session.php @@ -72,6 +72,7 @@ protected function startSession(): void /// @todo once we have improved `regenerate` so that it keeps around the old session and adds specific /// data to it, check here for its presence + // NB: this sometimes generates a php warning `ps_files_cleanup_dir: opendir(/var/lib/php/sessions) failed: Permission denied` if (!session_start($this->sessionOptions)) { throw new \RuntimeException('Failed to start the session'); } From 6c7fa5d4aa8c75f029439d9d51128842708c299f Mon Sep 17 00:00:00 2001 From: gggeek Date: Fri, 15 Nov 2024 16:32:12 +0000 Subject: [PATCH 38/38] feat: allow a different html page title on different pages --- resources/templates/admin/forgotpassword.html.twig | 2 ++ resources/templates/admin/index.html.twig | 2 ++ resources/templates/admin/resetpassword.html.twig | 2 ++ resources/templates/admin/setnewpassword.html.twig | 2 ++ resources/templates/base.html.twig | 2 +- resources/templates/report/confirm.html.twig | 2 ++ resources/templates/report/upload.html.twig | 2 ++ 7 files changed, 13 insertions(+), 1 deletion(-) diff --git a/resources/templates/admin/forgotpassword.html.twig b/resources/templates/admin/forgotpassword.html.twig index 56b5408..1b69be9 100644 --- a/resources/templates/admin/forgotpassword.html.twig +++ b/resources/templates/admin/forgotpassword.html.twig @@ -1,5 +1,7 @@ {% extends "base.html.twig" %} +{% block pagetitle %}VeraCrypt Crash Collector | Forgot Password{% endblock %} + {% block content %} {% import "parts/forms/macros.html.twig" as forms %} {% if form.isSubmitted() and (form.isValid() or form.pretendIsValid()) %} diff --git a/resources/templates/admin/index.html.twig b/resources/templates/admin/index.html.twig index ba71a9f..23fc312 100644 --- a/resources/templates/admin/index.html.twig +++ b/resources/templates/admin/index.html.twig @@ -1,5 +1,7 @@ {% extends "base.html.twig" %} +{% block pagetitle %}VeraCrypt Crash Collector | Search{% endblock %} + {% block content %} {% import "parts/forms/macros.html.twig" as forms %}
diff --git a/resources/templates/admin/resetpassword.html.twig b/resources/templates/admin/resetpassword.html.twig index 76d99d6..737ad89 100644 --- a/resources/templates/admin/resetpassword.html.twig +++ b/resources/templates/admin/resetpassword.html.twig @@ -1,5 +1,7 @@ {% extends "base.html.twig" %} +{% block pagetitle %}VeraCrypt Crash Collector | Reset Password{% endblock %} + {% block content %} {% import "parts/forms/macros.html.twig" as forms %} {% if form.isSubmitted() and form.isValid() %} diff --git a/resources/templates/admin/setnewpassword.html.twig b/resources/templates/admin/setnewpassword.html.twig index 555c9ff..8a464a6 100644 --- a/resources/templates/admin/setnewpassword.html.twig +++ b/resources/templates/admin/setnewpassword.html.twig @@ -1,5 +1,7 @@ {% extends "base.html.twig" %} +{% block pagetitle %}VeraCrypt Crash Collector | Set New Password{% endblock %} + {% block content %} {% import "parts/forms/macros.html.twig" as forms %} {% if error != '' %} diff --git a/resources/templates/base.html.twig b/resources/templates/base.html.twig index a8e53f1..78d9cc0 100644 --- a/resources/templates/base.html.twig +++ b/resources/templates/base.html.twig @@ -4,7 +4,7 @@ {% block head %} - VeraCrypt Crash Collector + {% block pagetitle %}VeraCrypt Crash Collector{% endblock %} {% endblock %} diff --git a/resources/templates/report/confirm.html.twig b/resources/templates/report/confirm.html.twig index c25c456..2d3d0b0 100644 --- a/resources/templates/report/confirm.html.twig +++ b/resources/templates/report/confirm.html.twig @@ -1,5 +1,7 @@ {% extends "base.html.twig" %} +{% block pagetitle %}VeraCrypt Crash Collector | Confirm Report{% endblock %} + {% block content %} {% import "parts/forms/macros.html.twig" as forms %} {% if error != '' %} diff --git a/resources/templates/report/upload.html.twig b/resources/templates/report/upload.html.twig index 75eeb3d..4d7948e 100644 --- a/resources/templates/report/upload.html.twig +++ b/resources/templates/report/upload.html.twig @@ -1,5 +1,7 @@ {% extends "base.html.twig" %} +{% block pagetitle %}VeraCrypt Crash Collector | Upload Report{% endblock %} + {% block content %} {% import "parts/forms/macros.html.twig" as forms %} {% if form.isSubmitted() and form.isValid() %}