fix(PROJ-62): Cross-Tenant IDOR bei POP3-Konto-Löschung/-Import behoben (Sicherheitsbug)
handleDeletePop3 und handleStartPop3Import prüften nur Owner/Rollen-Level, nicht den Tenant-Scope (anders als das korrekte IMAP-Pendant). Ein domain_admin konnte dadurch POP3-Konten eines fremden Tenants löschen oder deren Import anstoßen. Fix: tenantAccessAllowed(sess, acc.TenantID) ergänzt, analog zum IMAP-Handler. Gefunden bei gezielter Nachsuche nach Geschwister-Bugs zu PROJ-61. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
363874767b
commit
7c028601cf
+2
-1
@@ -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-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-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-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 |
|
||||||
|
|
||||||
<!-- Add features above this line -->
|
<!-- Add features above this line -->
|
||||||
|
|
||||||
## Next Available ID: PROJ-62
|
## Next Available ID: PROJ-63
|
||||||
|
|||||||
@@ -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.
|
||||||
@@ -106,6 +106,10 @@ func (s *Server) handleDeletePop3(w http.ResponseWriter, r *http.Request) {
|
|||||||
writeError(w, http.StatusForbidden, "access denied")
|
writeError(w, http.StatusForbidden, "access denied")
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if !tenantAccessAllowed(sess, acc.TenantID) {
|
||||||
|
writeError(w, http.StatusForbidden, "access denied")
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
if err := s.pop3Store.Delete(r.Context(), id); err != nil {
|
if err := s.pop3Store.Delete(r.Context(), id); err != nil {
|
||||||
writeError(w, http.StatusInternalServerError, "failed to delete account")
|
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")
|
writeError(w, http.StatusForbidden, "access denied")
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if !tenantAccessAllowed(sess, acc.TenantID) {
|
||||||
|
writeError(w, http.StatusForbidden, "access denied")
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
if acc.Status == "running" {
|
if acc.Status == "running" {
|
||||||
writeError(w, http.StatusConflict, "import already running")
|
writeError(w, http.StatusConflict, "import already running")
|
||||||
|
|||||||
Reference in New Issue
Block a user