security: comprehensive security audit with 13 findings
Document findings from full codebase security review covering: - 3 high-severity: unused InputSanitizer, password in query param, silent encryption fallback to plaintext - 5 medium-severity: missing authorization enforcement, rate limit bypass, ZIP path traversal, temp file permissions, sync API bypass - 5 low-severity: CSRF scope, error message leaks, missing CSP headers, CSV injection potential, date formatting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
shahondin1624
@@ -0,0 +1,126 @@
|
||||
# Security Review — Mitgliederverwaltung
|
||||
|
||||
**Date:** 2026-04-10
|
||||
**Reviewer:** Automated (Claude)
|
||||
**Scope:** Full codebase (`lib/`, `src/`, configuration)
|
||||
|
||||
---
|
||||
|
||||
## Critical Findings
|
||||
|
||||
### 1. InputSanitizer exists but is never called from controllers
|
||||
**Severity:** HIGH
|
||||
**Location:** `lib/Middleware/InputSanitizer.php`, all controllers
|
||||
**Description:** The `InputSanitizer` class provides `sanitizeString()`, `sanitizeArray()`, `sanitizeDate()`, etc., but no controller or service actually calls these methods. All user input from `getRequestData()` (JSON body) and `$this->request->getParam()` (query params) flows directly into services and database queries without sanitization.
|
||||
**Risk:** While the ORM uses parameterized queries (preventing SQL injection), unsanitized strings are stored in the database. These could contain HTML/script payloads that, while currently escaped by Vue on render, could be exploited if data is ever rendered in a non-Vue context (e.g., emails, PDF exports, CalDAV sync).
|
||||
**Recommendation:** Wire `InputSanitizer` into the `ApiControllerTrait.getRequestData()` method or apply it in service-layer `create`/`update` methods for all string fields.
|
||||
|
||||
### 2. DSGVO export uses query parameter for password instead of request body
|
||||
**Severity:** HIGH
|
||||
**Location:** `lib/Controller/DsgvoController.php:64`
|
||||
**Description:** The DSGVO export endpoint reads the encryption password via `$this->request->getParam('password')` which can come from query string. Passwords in query strings are logged in server access logs, browser history, and proxy logs.
|
||||
**Recommendation:** Read password exclusively from the POST body via `getRequestData()['password']`.
|
||||
|
||||
### 3. Encryption fallback returns plaintext silently
|
||||
**Severity:** HIGH
|
||||
**Location:** `lib/Service/EncryptionService.php:60-62`
|
||||
**Description:** If encryption fails (e.g., corrupted ICrypto configuration), `encrypt()` returns the plaintext value as a fallback. This means sensitive data (IBAN, health info) could be stored unencrypted in the database without any indication to the user or admin.
|
||||
**Recommendation:** Throw an exception instead of silently returning plaintext. Data loss prevention is secondary to data exposure prevention for IBAN/medical data.
|
||||
|
||||
---
|
||||
|
||||
## Medium Findings
|
||||
|
||||
### 4. No authorization checks on most controller endpoints
|
||||
**Severity:** MEDIUM
|
||||
**Location:** Most controllers (MemberController, FamilyController, LagerController, InjuryController, etc.)
|
||||
**Description:** While `PermissionService` exists and provides `canRead()`, `canWrite()`, `isAdmin()` etc., only a few controllers (DsgvoController, MilestoneController, some ExportController methods) actually check permissions. Most CRUD endpoints are accessible to any authenticated Nextcloud user with access to the app.
|
||||
**Risk:** Any app user can create, modify, or delete any member/family/fee/injury record. The PermissionService defines granular levels (none, read, write, admin, stufe-scoped) but they are not enforced.
|
||||
**Recommendation:** Add permission checks to all controller endpoints via the middleware or at the start of each handleAction callback.
|
||||
|
||||
### 5. Rate limiter uses URI-based pattern matching with md5 per-URI keys
|
||||
**Severity:** MEDIUM
|
||||
**Location:** `lib/Middleware/RateLimitMiddleware.php:80`
|
||||
**Description:** The rate limit cache key includes `md5($uri)`, which means each unique URL (including query parameters) gets its own rate limit bucket. An attacker can bypass the rate limit by appending varying dummy query parameters (e.g., `?_=1`, `?_=2`) to each request.
|
||||
**Recommendation:** Normalize the URI by stripping query parameters before hashing, or use route-based keys instead.
|
||||
|
||||
### 6. ZIP path traversal in BundleImportService
|
||||
**Severity:** MEDIUM
|
||||
**Location:** `lib/Service/BundleImportService.php:325`
|
||||
**Description:** When extracting ZIP files, `$zip->getNameIndex($i)` returns the filename as stored in the ZIP, which could contain path traversal sequences (e.g., `../../etc/passwd`). While `basename()` is used to strip directories (line 335), the `basename()` call happens after the hidden file checks but the content is read from the original index, not the basename. The actual risk is low because the content is only used as CSV data, not written to disk.
|
||||
**Recommendation:** Add explicit path traversal validation: reject entries containing `..` or absolute paths.
|
||||
|
||||
### 7. Temp file handling in EncryptedExportService and BundleImportService
|
||||
**Severity:** MEDIUM
|
||||
**Location:** `lib/Service/EncryptedExportService.php:114-164`, `lib/Service/BundleImportService.php:309-351`
|
||||
**Description:** Both services create temporary files with `tempnam()`. While `finally` blocks clean up the files, a race condition exists: between `file_put_contents` and `unlink`, another process could read sensitive exported data from the temp directory.
|
||||
**Recommendation:** Set restrictive permissions on temp files (`chmod 0600`) immediately after creation.
|
||||
|
||||
### 8. CalendarSync/ContactsSync use direct DB queries instead of Nextcloud APIs
|
||||
**Severity:** MEDIUM
|
||||
**Location:** `lib/Service/CalendarSyncService.php`, `lib/Service/ContactsSyncService.php`
|
||||
**Description:** These services directly insert into `mv_calendar_events` and `mv_contacts` tables and build VCalendar/VCard objects, but don't use the Nextcloud CalDAV/CardDAV APIs. This bypasses Nextcloud's own access control and sharing mechanisms.
|
||||
**Recommendation:** Consider using the `OCP\Calendar\IManager` and `OCP\Contacts\IManager` interfaces for proper integration with Nextcloud's permission model.
|
||||
|
||||
---
|
||||
|
||||
## Low Findings
|
||||
|
||||
### 9. CSRF protection disabled on read endpoints
|
||||
**Severity:** LOW
|
||||
**Location:** All controllers using `#[NoCSRFRequired]` on GET endpoints
|
||||
**Description:** `#[NoCSRFRequired]` is appropriately used on GET (read-only) endpoints. However, some export/download endpoints that trigger actions or side effects also have this attribute (e.g., DSGVO export, report PDF downloads). This is standard Nextcloud practice for download endpoints but worth noting.
|
||||
**Recommendation:** Verify that all `#[NoCSRFRequired]` endpoints are truly side-effect-free or require explicit user action.
|
||||
|
||||
### 10. Error messages leak internal details
|
||||
**Severity:** LOW
|
||||
**Location:** Various controllers and services
|
||||
**Description:** Some error responses include exception messages that could reveal internal implementation details (e.g., `ImportController` returns `'Bundle-Import fehlgeschlagen: ' . $e->getMessage()`). While the `handleAction` trait method uses generic "Internal server error" messages, some controllers still have manual catch blocks that expose details.
|
||||
**Recommendation:** Ensure all 500 responses use generic messages; log details server-side only.
|
||||
|
||||
### 11. No Content-Security-Policy headers on download responses
|
||||
**Severity:** LOW
|
||||
**Location:** `lib/Controller/ExportController.php`, `lib/Controller/ReportController.php`
|
||||
**Description:** File download responses (CSV, PDF, ZIP) don't set `Content-Disposition: attachment` explicitly in all cases, which could allow browser rendering of potentially malicious content (CSV injection).
|
||||
**Recommendation:** Always set `Content-Disposition: attachment; filename="..."` and `X-Content-Type-Options: nosniff` on download responses.
|
||||
|
||||
### 12. CSV injection potential in exports
|
||||
**Severity:** LOW
|
||||
**Location:** `lib/Service/CsvExportService.php`, `lib/Service/EntityExportService.php`
|
||||
**Description:** Exported CSV data includes user-controlled content (names, addresses, notes). If a field starts with `=`, `+`, `-`, or `@`, spreadsheet applications like Excel may interpret it as a formula, potentially executing commands (DDE/macro injection).
|
||||
**Recommendation:** Prefix fields starting with formula characters with a single quote (`'`) or tab character in CSV output.
|
||||
|
||||
### 13. Frontend date formatting inconsistency
|
||||
**Severity:** LOW
|
||||
**Location:** `src/views/Import.vue`, `src/views/MemberList.vue`
|
||||
**Description:** Some views format dates client-side with `toLocaleDateString('de-DE')` while the backend returns ISO format. This is a display-only concern, but inconsistent date handling can lead to misinterpretation in edge cases.
|
||||
|
||||
---
|
||||
|
||||
## Positive Security Practices
|
||||
|
||||
The following security measures are already well-implemented:
|
||||
|
||||
- **Parameterized queries:** All database queries use `createNamedParameter()` — no string concatenation in SQL.
|
||||
- **Field-level encryption:** IBAN, account holder, and allergy data are encrypted at rest via Nextcloud's `ICrypto`.
|
||||
- **Audit logging:** All CRUD operations are logged with user ID, timestamp, and field-level diffs.
|
||||
- **Encrypted field masking:** Audit logs and exports mask encrypted fields as `[verschluesselt]` unless explicitly authorized.
|
||||
- **Rate limiting:** Present on sensitive endpoints (search, export, import, reports).
|
||||
- **Soft-delete with data purging:** Member deletion removes sensitive data but preserves the record.
|
||||
- **DSGVO compliance:** Export and hard-delete endpoints exist for data subject rights.
|
||||
- **Password strength validation:** Encrypted export requires minimum 8-character password.
|
||||
- **LIKE escape:** The query builder properly escapes `%` and `_` in LIKE patterns.
|
||||
- **AST depth limiting:** Query builder limits nesting depth to 10 levels to prevent DoS.
|
||||
- **ZIP bomb protection:** Bundle import uses streaming extraction and checks file types.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Severity | Count | Findings |
|
||||
|----------|-------|----------|
|
||||
| Critical/High | 3 | Unused sanitizer, password in query param, silent encryption fallback |
|
||||
| Medium | 5 | Missing auth checks, rate limit bypass, ZIP traversal, temp files, sync bypasses |
|
||||
| Low | 5 | CSRF scope, error leaks, CSP headers, CSV injection, date formatting |
|
||||
|
||||
**Priority recommendation:** Address findings #1 (InputSanitizer integration), #2 (password in body), and #4 (authorization enforcement) first, as they have the highest practical risk.
|
||||
Reference in New Issue
Block a user