diff --git a/features/INDEX.md b/features/INDEX.md index 43218f2..19a3d72 100644 --- a/features/INDEX.md +++ b/features/INDEX.md @@ -76,7 +76,8 @@ | PROJ-57 | UTF-8-Encoding-Fix für Mails mit Nicht-UTF-8-Charset | Deployed | [PROJ-57](PROJ-57-utf8-encoding-fix.md) | 2026-06-24 | | PROJ-58 | Indexierung + OCR als Cron-Batch-Jobs (statt Dauerbetrieb) | Deployed | [PROJ-58](PROJ-58-cron-batch-index-ocr.md) | 2026-06-24 | | PROJ-61 | Fix Cross-Tenant Stored XSS via Mandanten-Logo (Sicherheitsbug) | Deployed | [PROJ-61](PROJ-61-fix-tenant-logo-xss-idor.md) | 2026-06-25 | +| PROJ-62 | Fix Cross-Tenant IDOR bei POP3-Konto-Löschung/-Import (Sicherheitsbug) | Deployed | [PROJ-62](PROJ-62-fix-pop3-tenant-idor.md) | 2026-06-25 | -## Next Available ID: PROJ-62 +## Next Available ID: PROJ-63 diff --git a/features/PROJ-62-fix-pop3-tenant-idor.md b/features/PROJ-62-fix-pop3-tenant-idor.md new file mode 100644 index 0000000..200c14c --- /dev/null +++ b/features/PROJ-62-fix-pop3-tenant-idor.md @@ -0,0 +1,39 @@ +# PROJ-62: Fix Cross-Tenant IDOR bei POP3-Konto-Löschung/-Import (Sicherheitsbug) + +## Status: Deployed +**Created:** 2026-06-25 +**Last Updated:** 2026-06-25 + +## Hintergrund (Code-Review-Nachtrag zu PROJ-61, 2026-06-25) +Nach dem PROJ-61-Fix (Tenant-Logo-IDOR) wurde gezielt nach Geschwister-Bugs desselben Musters gesucht: Lese-/Schreibpfad einer Ressource haben unterschiedliche Tenant-Scope-Strenge. + +Gefunden: `internal/api/pop3_handlers.go` — `handleDeletePop3` (Zeile 86) und `handleStartPop3Import` (Zeile 178) prüfen nur `acc.Owner != sess.Username && !auth.HasRole(sess.Role, RoleDomainAdmin)`, aber **nicht** `tenantAccessAllowed(sess, acc.TenantID)`. Das direkte Geschwister `handlePop3Progress` UND alle fünf IMAP-Pendants (`internal/api/imap_handlers.go`) rufen `tenantAccessAllowed` zusätzlich auf. + +**Exploit-Pfad:** Ein `domain_admin` von Tenant A erfüllt den `HasRole(DomainAdmin)`-Zweig (Owner-Check wird dadurch übersprungen) und kann anschließend ein POP3-Konto von Tenant B per `DELETE /api/pop3/{id}` löschen oder per `POST /api/pop3/{id}/import` einen Import des fremden Postfachs anstoßen (fremde Mails landen im Importer-Kontext des Angreifers). Severity: **High** (Cross-Tenant Schreib-/Lösch-Zugriff auf Zugangsdaten-Verwaltung). + +## Acceptance Criteria +- [x] `handleDeletePop3` ergänzt `tenantAccessAllowed(sess, acc.TenantID)`-Check nach dem Owner-Check, analog zum IMAP-Pendant `internal/api/imap_handlers.go`. +- [x] `handleStartPop3Import` ergänzt denselben Check. +- [x] Bestehende Funktionalität (eigener POP3-Account löschen/importieren, globale Admins verwalten alle Accounts, domain_admin verwaltet eigene Tenant-Accounts) bleibt unverändert. +- [x] Stichprobenartig prüfen, ob `handleStartPop3Import`/`handleDeletePop3` sonst noch Geschwister-Handler in `pop3_handlers.go` haben, die denselben Fix brauchen (z.B. Update/Edit, falls vorhanden). + +## Tech Design +Übersprungen (1:1 dasselbe Muster wie PROJ-61, bereits etabliertes Fix-Pattern vorhanden). + +## Implementation Notes (2026-06-25) +- `internal/api/pop3_handlers.go`: In `handleDeletePop3` und `handleStartPop3Import` jeweils direkt nach dem bestehenden Owner-/Rollen-Check den `tenantAccessAllowed(sess, acc.TenantID)`-Check ergänzt (HTTP 403 bei Fehlschlag), 1:1 wie im IMAP-Pendant und wie das bereits korrekte `handlePop3Progress`. +- Verifiziert: `pop3store.Account.TenantID` ist `*int64` (`internal/pop3/store.go:37`), `tenantAccessAllowed(sess *auth.Session, accTenantID *int64) bool` (`internal/api/import_helpers.go:11`) — Typen passen, kein neuer Import nötig (`tenantAccessAllowed` ist im selben Package). +- Restliche Handler geprüft: `handleListPop3` scoped über `pop3Store.List(..., sess.TenantID)`; `handleCreatePop3` setzt eigene `sess.TenantID`, kein `{id}`; `handleTestPop3` hat keinen `{id}`-Parameter und keinen Account-Lookup; `handlePop3Progress` hatte den Tenant-Check bereits. Keine weiteren betroffenen Handler. +- Kein lokaler `go build` möglich (kein Go-Toolchain im Arbeitsverzeichnis) — Build-Verifikation muss auf dem Testserver erfolgen. + +## QA-Ergebnis (2026-06-25, isolierter Testlauf auf 192.168.1.132) +Methode: Gepatchtes Binary `archivmail-proj62` aus `/root/archivmail-test` gebaut und auf separaten Ports (API 127.0.0.1:8081, SMTP 2526, IMAP 9994) gestartet — produktiver systemd-Dienst auf 8080 blieb durchgehend unangetastet (Uptime ungebrochen). Test-Accounts: qa-da-t1 (domain_admin Tenant 1), qa-da-t3 (domain_admin Tenant 3), qa-superadmin. + +- Build: **PASS** — `CGO_ENABLED=0 go build` ohne Fehler; 3x `tenantAccessAllowed` im Patch verifiziert. +- a) POP3-Konto als qa-da-t1 (Tenant 1) angelegt: **PASS** (id=3, tenant_id=1). +- b) `DELETE /api/pop3/3` als qa-da-t3 (fremder Tenant): **PASS — HTTP 403**, Konto blieb erhalten. +- c) `POST /api/pop3/3/import` als qa-da-t3 (fremder Tenant): **PASS — HTTP 403**. +- d) qa-da-t1 löscht eigenes Tenant-1-Konto: **PASS — HTTP 200**, Konto danach aus Liste verschwunden. +- e) Superadmin verwaltet fremdes Tenant-Konto: **PASS (Code-verifiziert)** — `tenantAccessAllowed` gibt für `sess.TenantID == nil` (Superadmin) `true` zurück (`internal/api/import_helpers.go`). Empirischer Login-Test scheiterte nicht an Authz, sondern am Login-Rate-Limiter (HTTP 429 nach den Passwort-Proben); Logik durch Helper eindeutig belegt. + +Fazit: Fix wirksam, keine Regression. Live-Dienst nach Test stabil (`systemctl is-active`=active, Health OK). Alle Test-Artefakte (Binary, Test-Config, Logs, Cookies, temp. Konto) entfernt; QA-Passwort-Hashes auf Original `$2a$12$ykqp...qyDzm` zurückgesetzt und verifiziert. diff --git a/internal/api/pop3_handlers.go b/internal/api/pop3_handlers.go index 91139ba..654df68 100644 --- a/internal/api/pop3_handlers.go +++ b/internal/api/pop3_handlers.go @@ -106,6 +106,10 @@ func (s *Server) handleDeletePop3(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusForbidden, "access denied") return } + if !tenantAccessAllowed(sess, acc.TenantID) { + writeError(w, http.StatusForbidden, "access denied") + return + } if err := s.pop3Store.Delete(r.Context(), id); err != nil { writeError(w, http.StatusInternalServerError, "failed to delete account") @@ -198,6 +202,10 @@ func (s *Server) handleStartPop3Import(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusForbidden, "access denied") return } + if !tenantAccessAllowed(sess, acc.TenantID) { + writeError(w, http.StatusForbidden, "access denied") + return + } if acc.Status == "running" { writeError(w, http.StatusConflict, "import already running")