From 363874767b0eebacd8aff96a314c46972c607916 Mon Sep 17 00:00:00 2001
From: sysops <sysops@LenovoT14Gen5>
Date: Thu, 25 Jun 2026 00:31:41 +0200
Subject: [PATCH] fix(PROJ-61): Cross-Tenant Stored XSS via Mandanten-Logo
 behoben (Sicherheitsbug)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

GET /api/tenants/{id}/logo prüfte nur die Authentifizierung, aber keinen
Tenant-Scope — jeder eingeloggte Nutzer konnte das Logo jedes beliebigen
Tenants lesen. Kombiniert mit dem bisher erlaubten SVG-Upload (kann
eingebettetes JavaScript enthalten) ergab das einen Cross-Tenant Stored-XSS:
ein domain_admin konnte ein bösartiges SVG als eigenes Logo hochladen und
Opfer aus beliebigen anderen Tenants per direktem Link darauf locken.

Fix: tenantAccessAllowed()-Scope-Check beim Logo-Lesepfad (analog PROJ-55),
SVG aus erlaubten Upload-Typen entfernt, X-Content-Type-Options: nosniff
als Defense-in-Depth ergänzt.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---
 features/INDEX.md                            |  3 +-
 features/PROJ-61-fix-tenant-logo-xss-idor.md | 53 ++++++++++++++++++++
 internal/api/ldap_tenants.go                 |  2 +-
 internal/api/tenant_logo_handlers.go         | 24 ++++++---
 4 files changed, 73 insertions(+), 9 deletions(-)
 create mode 100644 features/PROJ-61-fix-tenant-logo-xss-idor.md

diff --git a/features/INDEX.md b/features/INDEX.md
index 526b8f0..43218f2 100644
--- a/features/INDEX.md
+++ b/features/INDEX.md
@@ -75,7 +75,8 @@
 | PROJ-56 | Last-Entzerrung für Hintergrundjobs (OCR-Zeitfenster, IMAP-Sync-Jitter) | Deployed | [PROJ-56](PROJ-56-last-entzerrung-hintergrundjobs.md) | 2026-06-22 |
 | 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 |
 
 <!-- Add features above this line -->
 
-## Next Available ID: PROJ-59
+## Next Available ID: PROJ-62
diff --git a/features/PROJ-61-fix-tenant-logo-xss-idor.md b/features/PROJ-61-fix-tenant-logo-xss-idor.md
new file mode 100644
index 0000000..e19ae15
--- /dev/null
+++ b/features/PROJ-61-fix-tenant-logo-xss-idor.md
@@ -0,0 +1,53 @@
+# PROJ-61: Fix Cross-Tenant Stored XSS via Mandanten-Logo (Sicherheitsbug)
+
+## Status: Deployed
+**Created:** 2026-06-25
+**Last Updated:** 2026-06-25
+
+## Hintergrund (Security-Audit, 2026-06-25)
+Nachtrag zum letzten Audit (docs/security-audit-2026-03-18.md) über alle Commits seit März (PROJ-25–60). Zwei kombinierte Lücken in den Tenant-Logo-Handlern (eingeführt mit Multi-Tenancy-Ausbau):
+
+1. `POST /api/tenant/logo` (eigener Tenant, `domain_admin` reicht) akzeptiert `image/svg+xml` ohne Sanitisierung. SVG kann `<script>`/`onload` enthalten.
+2. `GET /api/tenants/{id}/logo` prüft nur `s.auth` (jeder eingeloggte Nutzer, egal welcher Tenant/Rolle) — kein Tenant-Scope-Check, anders als die Schreib-/Lösch-Pendants (`requireRole`/`authAdmin` prüfen nur Rollenlevel, keinen Tenant-Bezug).
+
+**Exploit-Pfad:** `domain_admin` von Tenant A lädt präpariertes SVG als eigenes Logo hoch → lockt ein Opfer aus einem BELIEBIGEN anderen Tenant per direktem Link auf `/api/tenants/A/logo` → SVG wird same-origin mit `image/svg+xml` ausgeliefert und im Browser des Opfers ausgeführt → Skript kann mit dem httpOnly-JWT-Cookie des Opfers API-Calls im Namen des Opfers auslösen.
+
+## Acceptance Criteria
+- [x] `GET /api/tenants/{id}/logo` erzwingt Tenant-Scope: nur `superadmin`/`admin` (global) ODER der Nutzer gehört zum angefragten Tenant — analog zum projektüblichen `tenantAccessAllowed()`-Muster (siehe PROJ-55).
+- [x] SVG (`image/svg+xml`) wird beim Logo-Upload nicht mehr akzeptiert (einfachste, robusteste Lösung).
+- [x] Bestehende, bereits hochgeladene Logos werden beim Ausliefern zusätzlich defensiv abgesichert: `X-Content-Type-Options: nosniff` Header bei Logo-Responses ergänzt (Defense-in-Depth, auch für nicht-SVG-Typen).
+- [x] Bestehende Funktionalität (PNG/JPEG/GIF/WebP-Logos hoch-/runterladen, eigene + fremde Tenant-Logos für globale Admins) bleibt unverändert nutzbar.
+- [x] Kein Datenverlust: bereits gespeicherte SVG-Logos werden nicht automatisch gelöscht, nur zukünftige Uploads blockiert (siehe Restrisiko unten).
+
+## Tech Design
+Übersprungen (klar umrissener Sicherheitsfix mit bestehenden Mustern, kein architektonischer Schnitt).
+
+## Implementation Notes (2026-06-25)
+Geändert: `internal/api/tenant_logo_handlers.go`, `internal/api/ldap_tenants.go`.
+
+1. **IDOR-Fix** (`handleGetTenantLogo`): Nach dem Parsen der `id` wird `sessionFromCtx(r.Context())` geholt und `tenantAccessAllowed(sess, &id)` geprüft. Globale Sessions (`sess.TenantID == nil`, also superadmin/admin) dürfen jedes Tenant-Logo lesen; alle anderen nur das eigene (`*sess.TenantID == id`), sonst HTTP 403. Route in `ldap_tenants.go` bleibt `s.auth(...)` (setzt Session via authMiddleware), Scope jetzt im Handler erzwungen — Kommentar angepasst. Kein neuer Import nötig (`sessionFromCtx`/`tenantAccessAllowed` sind im selben Package).
+
+2. **XSS-Fix** (`saveTenantLogo`): `"image/svg+xml"` aus der `allowed`-Map entfernt; Fehlertext und erlaubte Typen aktualisiert (png, jpeg, gif, webp). PNG/JPEG/GIF/WebP unverändert.
+
+3. **Defense-in-Depth**: `w.Header().Set("X-Content-Type-Options", "nosniff")` in `handleGetTenantLogo` und `handleGetOwnTenantLogo` vor dem Body-Write ergänzt.
+
+Frontend-Prüfung: `getTenantLogoUrl(tenantId)` wird ausschließlich in `src/hooks/useTenantLogos.ts` (Superadmin/Admin-Tenant-Verwaltungsdialog) genutzt; das eigene Logo läuft über `/api/tenant/logo`. Kein Frontend-Pfad liest fremde Tenant-Logos als nicht-globaler Nutzer → IDOR-Fix bricht nichts.
+
+## Restrisiko / Offene Fragen
+- **Bestands-SVGs:** Bereits gespeicherte SVG-Logos werden weiter mit `image/svg+xml` ausgeliefert. `nosniff` verhindert MIME-Sniffing, aber NICHT die Ausführung eines explizit als `image/svg+xml` ausgelieferten SVG bei direkter Navigation. Restrisiko ist durch den IDOR-Fix stark reduziert (nur noch eigener Tenant + globale Admins). Falls Bestands-SVGs existieren, sollte ein einmaliges Cleanup/Re-Upload erwogen werden (separate Aufgabe).
+- Lokal kein `go build` möglich — Compile auf 192.168.1.131 (devops-deploy) verifizieren.
+
+## QA Test Results (192.168.1.132, 2026-06-25)
+- Build: Exit 0. `go vet`: keine neuen Befunde (einziger Treffer vorbestehend, PROJ-61-fremd: `api_test.go:36 os.Discard`).
+- a) superadmin liest beliebiges Tenant-Logo → 200/404, nicht 403. PASS
+- b) domain_admin (Tenant 3) liest fremdes Tenant-Logo (1, 2) → **403**. Eigenes Tenant (3) → 404 (kein Logo). PASS — IDOR behoben.
+- c) domain_admin liest eigenes Logo via `/api/tenant/logo` → normal. PASS
+- d) SVG-Upload → **400** "unsupported image type". PASS — XSS-Vektor blockiert.
+- e) PNG-Upload → 200. PASS — bestehende Funktionalität erhalten.
+- f) `X-Content-Type-Options: nosniff` im Response-Header bestätigt. PASS
+- Unauthentifizierter Zugriff → 401. Globale Admins weiterhin uneingeschränkt.
+- Keine Regressionen. Cleanup auf 132 durchgeführt, Health-Check OK.
+
+## Deployment
+- Test (192.168.1.132): QA bestanden, System nach Test zurückgesetzt. 2026-06-25.
+- Produktion (192.168.1.131): siehe unten.
diff --git a/internal/api/ldap_tenants.go b/internal/api/ldap_tenants.go
index 5e6affa..fc3934a 100644
--- a/internal/api/ldap_tenants.go
+++ b/internal/api/ldap_tenants.go
@@ -38,7 +38,7 @@ func (s *Server) SetTenants(store *tenantstore.Store) {
 	s.mux.HandleFunc("DELETE /api/tenants/{id}/domains/{did}", s.authMiddleware(s.requireRole(userstore.RoleAdmin, s.handleRemoveTenantDomain)))
 	s.mux.HandleFunc("GET /api/tenants/{id}/users", s.authMiddleware(s.requireRole(userstore.RoleAdmin, s.handleListTenantUsers)))
 
-	// Logo routes: any auth can read; admin can write
+	// Logo routes: read is tenant-scoped inside handleGetTenantLogo (PROJ-61); admin can write
 	s.mux.HandleFunc("GET /api/tenants/{id}/logo", s.auth(s.handleGetTenantLogo))
 	s.mux.HandleFunc("POST /api/tenants/{id}/logo", s.authMiddleware(s.requireRole(userstore.RoleAdmin, s.handleUploadTenantLogo)))
 	s.mux.HandleFunc("DELETE /api/tenants/{id}/logo", s.authMiddleware(s.requireRole(userstore.RoleAdmin, s.handleDeleteTenantLogo)))
diff --git a/internal/api/tenant_logo_handlers.go b/internal/api/tenant_logo_handlers.go
index 7a8f8e5..e723919 100644
--- a/internal/api/tenant_logo_handlers.go
+++ b/internal/api/tenant_logo_handlers.go
@@ -21,6 +21,13 @@ func (s *Server) handleGetTenantLogo(w http.ResponseWriter, r *http.Request) {
 		writeError(w, http.StatusBadRequest, "invalid tenant id")
 		return
 	}
+	// Tenant-scope enforcement (IDOR fix, PROJ-61): non-global sessions may only
+	// read their own tenant's logo. Global admins (sess.TenantID == nil) see all.
+	sess := sessionFromCtx(r.Context())
+	if !tenantAccessAllowed(sess, &id) {
+		writeError(w, http.StatusForbidden, "access denied")
+		return
+	}
 	data, contentType, err := s.tenantStore.GetLogo(r.Context(), id)
 	if err != nil {
 		writeError(w, http.StatusInternalServerError, "failed to load logo")
@@ -34,6 +41,7 @@ func (s *Server) handleGetTenantLogo(w http.ResponseWriter, r *http.Request) {
 		contentType = "image/png"
 	}
 	w.Header().Set("Content-Type", contentType)
+	w.Header().Set("X-Content-Type-Options", "nosniff")
 	w.Header().Set("Cache-Control", "public, max-age=86400")
 	w.WriteHeader(http.StatusOK)
 	_, _ = w.Write(data)
@@ -103,6 +111,7 @@ func (s *Server) handleGetOwnTenantLogo(w http.ResponseWriter, r *http.Request)
 		contentType = "image/png"
 	}
 	w.Header().Set("Content-Type", contentType)
+	w.Header().Set("X-Content-Type-Options", "nosniff")
 	w.Header().Set("Cache-Control", "public, max-age=86400")
 	w.WriteHeader(http.StatusOK)
 	_, _ = w.Write(data)
@@ -163,16 +172,17 @@ func (s *Server) saveTenantLogo(w http.ResponseWriter, r *http.Request, tenantID
 	if contentType == "" {
 		contentType = "image/png"
 	}
+	// SVG intentionally NOT allowed (PROJ-61): SVG can carry embedded
+	// JavaScript and would be served same-origin as image/svg+xml → stored XSS.
 	allowed := map[string]bool{
-		"image/png":     true,
-		"image/jpeg":    true,
-		"image/jpg":     true,
-		"image/gif":     true,
-		"image/webp":    true,
-		"image/svg+xml": true,
+		"image/png":  true,
+		"image/jpeg": true,
+		"image/jpg":  true,
+		"image/gif":  true,
+		"image/webp": true,
 	}
 	if !allowed[contentType] {
-		writeError(w, http.StatusBadRequest, "unsupported image type (allowed: png, jpeg, gif, webp, svg)")
+		writeError(w, http.StatusBadRequest, "unsupported image type (allowed: png, jpeg, gif, webp)")
 		return
 	}