sysops
2de340573b
F-01: err.Error() wird nicht mehr an HTTP-Clients gesendet.
Stattdessen generische Fehlermeldungen + Server-Log.
Betrifft: handleCreateUser, handleUpdateUser, handleDeleteUser,
handleSyncNow, handleSecurityConfig, handleUpload.
F-02: Login-Audit-Log enthält keinen rohen err.Error() mehr.
Neue classifyLoginError() Funktion: invalid_password / ldap_error /
account_disabled / unknown — schützt vor LDAP-Info-Leak via Audit-API.
W-03: remoteIP() trimmt jetzt Leerzeichen aus X-Forwarded-For.
Vollständige Lösung erfordert Proxy-Konfiguration (W-03 bleibt WARN).
W-04: Attachment-Dateiname wird durch sanitizeFilename() bereinigt.
Nur [a-zA-Z0-9._- ] erlaubt — verhindert Header-Injection.
PROJ-24: TOTP 2FA vollständig implementiert:
- internal/auth/totp.go: GenerateSecret, ValidateTOTP, QRCodeSVG
- internal/api/totp_handlers.go: Setup, Login-Step2, Admin-Reset
- internal/userstore: SetTOTPSecret, EnableTOTP, DisableTOTP, ResetTOTP
- Login-Flow: totp_pending JWT → /api/auth/totp → vollwertiger JWT
- AES-256-GCM verschlüsseltes Secret in users.totp_secret
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
191 lines
12 KiB
Markdown
191 lines
12 KiB
Markdown
## Security-Audit Bericht -- archivmail (2026-03-18)
|
|
|
|
Auditor: Security Engineer (Code-Review)
|
|
Scope: Juengste Commits -- HKDF, Rollenhierarchie, Tenant-LDAP, Mail-ID-Validation, SMTP, Export
|
|
|
|
---
|
|
|
|
### Zusammenfassung
|
|
|
|
PASS: 27 | FAIL: 2 | WARN: 5
|
|
|
|
---
|
|
|
|
### Ergebnisse
|
|
|
|
| # | Bereich | Pruefpunkt | Ergebnis | Details |
|
|
|---|---------|-----------|----------|---------|
|
|
| 1 | SEC-08 HKDF | Zwei verschiedene Salts fuer JWT und AES | PASS | `archivmail-jwt-v1` und `archivmail-aes-v1` korrekt in `cmd/archivmail/main.go:82,87` |
|
|
| 2 | SEC-08 HKDF | jwtSecret wird fuer auth.New() verwendet | PASS | `main.go:165`: `auth.New(users, ldapSt, jwtSecret)` |
|
|
| 3 | SEC-08 HKDF | aesKey wird fuer ldapcfg, imap, pop3 verwendet | PASS | `main.go:157,243,261`: alle drei Stores erhalten `aesKey` |
|
|
| 4 | SEC-08 HKDF | apiCfg.Secret = jwtSecret (nicht cfg.API.Secret) | PASS | `main.go:170`: `Secret: jwtSecret` -- korrekt abgeleitet |
|
|
| 5 | SEC-08 HKDF | Keine direkte Nutzung von cfg.API.Secret als Schluessel | PASS | Grep zeigt nur `main.go:80` (masterKey Zuweisung) und Kommentar -- kein Durchreichen |
|
|
| 6 | SEC-01 Rollen | roleLevel() Hierarchie korrekt | PASS | `server.go:46-55`: superadmin=5, admin=4, domain_admin=3, auditor=2, user=1. Konsistent mit `auth.go:284-293` |
|
|
| 7 | SEC-01 Rollen | handleCreateUser prueft roleLevel | PASS | `server.go:387`: `roleLevel(req.Role) >= roleLevel(sess.Role)` -- domain_admin (3) kann keinen superadmin (5) anlegen |
|
|
| 8 | SEC-01 Rollen | handleUpdateUser prueft roleLevel fuer Ziel UND neue Rolle | PASS | `server.go:464,468`: Beide Checks vorhanden (target.Role und req.Role) |
|
|
| 9 | SEC-01 Rollen | handleDeleteUser prueft roleLevel | PASS | `server.go:525`: `roleLevel(target.Role) >= roleLevel(sess.Role)` |
|
|
| 10 | SEC-01 Rollen | domain_admin kann sich nicht zu superadmin befoerdern | PASS | handleUpdateUser Zeile 468: Selbst-Update wird durch `roleLevel(target.Role) >= roleLevel(sess.Role)` in Zeile 464 blockiert (eigene Rolle = domain_admin (3) >= domain_admin (3) = true -> 403) |
|
|
| 11 | BUG-1 Fix | handleSaveTenantLDAP Allowlist | PASS | `ldap_tenants.go:452`: Allowlist `{"user": true, "auditor": true}` korrekt |
|
|
| 12 | BUG-1 Fix | handleSaveTenantLDAP prueft group_mappings | PASS | `ldap_tenants.go:457-461`: Jedes GroupMapping wird gegen Allowlist geprueft |
|
|
| 13 | BUG-1 Fix | handleAdminSaveTenantLDAP Allowlist | PASS | `ldap_tenants.go:593`: Allowlist `{"user": true, "auditor": true, "domain_admin": true}` -- korrekt, superadmin ausgeschlossen |
|
|
| 14 | BUG-1 Fix | handleAdminSaveTenantLDAP prueft group_mappings | PASS | `ldap_tenants.go:598-602`: Jedes GroupMapping wird geprueft |
|
|
| 15 | BUG-1 Fix | Leerer default_role erlaubt | PASS | `ldap_tenants.go:453`: `cfg.DefaultRole != ""` -- leerer String wird nicht geprueft, also erlaubt |
|
|
| 16 | Login LDAP | Reihenfolge: lokal -> tenant LDAP -> global LDAP | PASS | `auth.go:66-155`: (1) local VerifyPassword (2) tenant LDAP ab Zeile 74 (3) global LDAP ab Zeile 119 |
|
|
| 17 | Login LDAP | Globales LDAP kann Tenant-LDAP nicht ueberschreiben | PASS | Tenant-LDAP (Step 2) wird vor globalem LDAP (Step 3) geprueft. Bei Erfolg in Step 2 wird sofort returned |
|
|
| 18 | SEC-22 | handleGetMail: isValidMailID vor Store-Zugriff | PASS | `server.go:908-912` |
|
|
| 19 | SEC-22 | handleGetAttachment: isValidMailID vor Store-Zugriff | PASS | `server.go:997-1001` |
|
|
| 20 | SEC-22 | handleGetRaw: isValidMailID vor Store-Zugriff | PASS | `server.go:1060-1064` |
|
|
| 21 | SEC-22 | handleExportPDF: isValidMailID vor Store-Zugriff | PASS | `export.go:331-334` |
|
|
| 22 | SEC-22 | handleExportZIP: isValidMailID fuer alle IDs | PASS | `export.go:413-418` |
|
|
| 23 | SEC-22 | Regex als Paket-Variable kompiliert | PASS | `server.go:37`: `var mailIDRegex = regexp.MustCompile(...)` -- einmalig kompiliert |
|
|
| 24 | SEC-26 | isAllowed: leere AllowedIPs -> false | PASS | `smtpd.go:317-318`: `len(d.cfg.AllowedIPs) == 0` returns false (fail-closed) |
|
|
| 25 | SEC-26 | Kommentar in config.go vorhanden | PASS | `config.go:59-61`: Erklaert `allowed_ips: ["0.0.0.0/0", "::/0"]` |
|
|
| 26 | SEC-05 | handleExportPDF: Tenant-Isolation | PASS | `export.go:351-357`: Prueft `sess.TenantID != nil` und vergleicht mit Mail-Tenant |
|
|
| 27 | SEC-05 | handleExportZIP: Tenant-Isolation | PASS | `export.go:423-439`: Laedt alle IDs via GetAllIDsByTenant und prueft jede angeforderte ID |
|
|
| 28 | SEC-05 | superadmin (TenantID==nil) sieht alle Mails | PASS | Alle Tenant-Checks starten mit `if sess.TenantID != nil` -- superadmin hat nil -> kein Check -> Vollzugriff |
|
|
| 29 | Tenant-LDAP Auth | /api/tenant/ldap ohne Session -> 401 | PASS | `ldap_tenants.go:396`: `s.authAdmin(...)` = `authMiddleware(tenantMiddleware(requireRole(RoleDomainAdmin, ...)))` -- authMiddleware blockt ohne Token |
|
|
| 30 | Tenant-LDAP Auth | user-Rolle kann /api/tenant/ldap nicht aufrufen | PASS | requireRole(RoleDomainAdmin, ...) -> HasRole("user", "domain_admin") = false (1 < 3) -> HTTP 403 |
|
|
| 31 | Tenant-LDAP Auth | GET /api/tenant/ldap gibt bind_password NICHT im Klartext zurueck | PASS | `handleGetTenantLDAP` ruft `tenantLdapStore.Get()` auf -> `tenant_store.go:77-78`: Password wird zu "..." maskiert |
|
|
| 32 | SEC-01 Hierarchie | Hierarchie-Diskrepanz Spec vs. Code | WARN | Audit-Aufgabe nennt `superadmin=5 > domain_admin=4 > admin=3`, Code hat `admin=4 > domain_admin=3`. Code-Hierarchie ist logisch korrekt (globaler admin > tenant domain_admin). Spec-Text im Audit-Auftrag weicht ab. |
|
|
| 33 | Allgemein | SQL Injection | PASS | Keine String-Konkatenation in SQL-Queries gefunden. Alle Stores verwenden pgx parameterized queries |
|
|
| 34 | Allgemein | Fehlerausgaben mit err.Error() | FAIL | Siehe Finding F-01 |
|
|
| 35 | Allgemein | Audit-Log fuer sicherheitsrelevante Aktionen | WARN | Siehe Finding W-01 |
|
|
|
|
---
|
|
|
|
### Kritische Findings (FAIL)
|
|
|
|
#### F-01: Interne Fehlerdetails an Client (Severity: MEDIUM, Priority: P2)
|
|
|
|
**Datei:** `internal/api/server.go`
|
|
**Zeilen:** 407, 480, 535, 1541, 1917
|
|
|
|
Mehrere Handler geben `err.Error()` direkt an den HTTP-Client weiter:
|
|
|
|
- Zeile 407 (`handleCreateUser`): `writeError(w, http.StatusBadRequest, err.Error())`
|
|
- Zeile 480 (`handleUpdateUser`): `writeError(w, http.StatusBadRequest, err.Error())`
|
|
- Zeile 535 (`handleDeleteUser`): `writeError(w, http.StatusInternalServerError, err.Error())`
|
|
- Zeile 1541: `writeError(w, http.StatusConflict, err.Error())`
|
|
- Zeile 1917: `"could not write jail.local: "+err.Error()`
|
|
|
|
Ebenso in `internal/api/upload.go:56`: `"multipart parse failed: "+err.Error()`
|
|
|
|
**Risiko:** Interne Implementierungsdetails, Datenbank-Fehlermeldungen oder Dateipfade koennten an Angreifer weitergegeben werden. Dies erleichtert Reconnaissance.
|
|
|
|
**Fix-Vorschlag:** Generische Fehlermeldungen an den Client senden, Details nur ins Server-Log schreiben:
|
|
```go
|
|
s.logger.Error("create user failed", "err", err)
|
|
writeError(w, http.StatusBadRequest, "user creation failed")
|
|
```
|
|
|
|
---
|
|
|
|
#### F-02: Login-Handler gibt err.Error() im Audit-Detail weiter (Severity: LOW, Priority: P3)
|
|
|
|
**Datei:** `internal/api/server.go`
|
|
**Zeile:** 247
|
|
|
|
```go
|
|
Detail: err.Error(),
|
|
```
|
|
|
|
Der Audit-Log-Eintrag bei fehlgeschlagenem Login enthaelt den vollstaendigen Fehlertext. Dies ist fuer das Audit-Log akzeptabel (intern), aber der gleiche `err.Error()` koennte bei unsachgemaesser Audit-Log-Exposition Details verraten. In Kombination mit dem Audit-API-Endpoint (`GET /api/audit`, zugaenglich fuer `auditor`-Rolle) koennten LDAP-Fehlermeldungen mit internen Hostnamen/Ports sichtbar werden.
|
|
|
|
**Fix-Vorschlag:** Fehlerdetails im Audit-Log auf vordefinierte Kategorien beschraenken: `"invalid_password"`, `"user_not_found"`, `"ldap_error"`.
|
|
|
|
---
|
|
|
|
### Warnungen (WARN)
|
|
|
|
#### W-01: Fehlende Audit-Logs bei einigen sicherheitsrelevanten Aktionen
|
|
|
|
**Betroffene Handler:**
|
|
- `handleUpdateTenant` (`ldap_tenants.go:247-289`): Kein Audit-Log-Eintrag bei Tenant-Update.
|
|
- `handleAddTenantDomain` / `handleRemoveTenantDomain`: Kein Audit-Log-Eintrag.
|
|
- `handleGetAttachment`, `handleGetRaw`: Kein Audit-Log fuer einzelne Zugriffe (nur Export wird geloggt).
|
|
|
|
**Empfehlung:** Audit-Log-Eintraege fuer Tenant-Aenderungen und Domain-Zuweisungen hinzufuegen. Mail-Lesezugriffe sind optional, aber fuer GoBD-Compliance empfehlenswert.
|
|
|
|
---
|
|
|
|
#### W-02: Cookie Secure-Flag nicht gesetzt
|
|
|
|
**Datei:** `internal/api/server.go`
|
|
**Zeile:** 269
|
|
|
|
```go
|
|
// Secure: true -- enable when TLS is terminated at this server
|
|
```
|
|
|
|
Das `Secure`-Flag ist auskommentiert. Im On-Premise-Deployment ohne TLS-Terminierung am Go-Server ist dies verstaendlich, aber wenn ein Reverse-Proxy (nginx/caddy) TLS terminiert, koennte das Cookie ueber eine unverschluesselte Verbindung gesendet werden.
|
|
|
|
**Empfehlung:** Konfigurierbar machen oder automatisch setzen wenn hinter TLS-Proxy (`X-Forwarded-Proto: https`).
|
|
|
|
---
|
|
|
|
#### W-03: X-Forwarded-For Header nicht validiert
|
|
|
|
**Datei:** `internal/api/server.go`
|
|
**Zeile:** 880-884
|
|
|
|
```go
|
|
func remoteIP(r *http.Request) string {
|
|
if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" {
|
|
return strings.Split(fwd, ",")[0]
|
|
}
|
|
return r.RemoteAddr
|
|
}
|
|
```
|
|
|
|
Ein Angreifer kann den `X-Forwarded-For`-Header faelschen, um eine andere IP in Audit-Logs und Rate-Limiting zu injizieren. Dies ermoeglicht:
|
|
1. Rate-Limit-Umgehung beim Login (verschiedene gefaelschte IPs)
|
|
2. Falsche IP-Adressen in Audit-Logs
|
|
|
|
**Empfehlung:** Nur dem aeussersten Proxy-Hop vertrauen oder die Anzahl vertrauenswuerdiger Proxies konfigurierbar machen. Alternativ `r.RemoteAddr` verwenden wenn kein Proxy konfiguriert ist.
|
|
|
|
---
|
|
|
|
#### W-04: Attachment-Filename ohne Sanitierung im Content-Disposition
|
|
|
|
**Datei:** `internal/api/server.go`
|
|
**Zeile:** 1052
|
|
|
|
```go
|
|
w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, filename))
|
|
```
|
|
|
|
Der Attachment-Filename kommt direkt aus der geparsten E-Mail. Ein boesartiger Absender koennte einen Dateinamen mit `"` oder Newlines senden, was zu Header-Injection fuehren koennte.
|
|
|
|
Ebenso in `export.go:377` (dort ist die ID gekuerzt -- sicher) und `server.go:1098` (ID-basiert -- sicher).
|
|
|
|
**Empfehlung:** Dateinamen sanitieren: Nur alphanumerische Zeichen, Punkt, Bindestrich und Unterstrich erlauben. Alternativ RFC 6266 `filename*` mit Encoding verwenden.
|
|
|
|
---
|
|
|
|
#### W-05: HKDF ohne Info-Parameter
|
|
|
|
**Datei:** `cmd/archivmail/main.go`
|
|
**Zeilen:** 82, 87
|
|
|
|
```go
|
|
hkdf.New(sha256.New, masterKey, []byte("archivmail-jwt-v1"), nil)
|
|
```
|
|
|
|
Der HKDF-Aufruf nutzt den Salt-Parameter zur Schluesseltrennung, aber der `info`-Parameter ist `nil`. Gemaess RFC 5869 ist der `info`-Parameter fuer die Kontextbindung vorgesehen, waehrend `salt` fuer die Randomisierung dient. Funktional ist dies korrekt und sicher, da die verschiedenen Salt-Werte eine wirksame Schluesseltrennung gewaehrleisten. Es waere jedoch konzeptionell sauberer, den Kontext (`archivmail-jwt-v1`) als `info` und einen festen Salt zu verwenden.
|
|
|
|
**Empfehlung:** Kein sofortiger Handlungsbedarf -- die aktuelle Implementierung ist kryptographisch sicher. Bei einem zukuenftigen Refactoring koennte der Kontext in den `info`-Parameter verschoben werden.
|
|
|
|
---
|
|
|
|
### Geprueft und fuer gut befunden
|
|
|
|
1. **HKDF-Schluessel-Trennung (SEC-08):** Korrekt implementiert mit zwei unabhaengigen Ableitungen.
|
|
2. **Rollenhierarchie (SEC-01):** Konsistent in server.go und auth.go. Privilege-Escalation-Checks in allen drei User-Management-Handlern vorhanden.
|
|
3. **BUG-1 Fix (Tenant-LDAP Allowlists):** Beide Endpunkte (domain_admin und superadmin) pruefen sowohl `default_role` als auch `group_mappings[].role`.
|
|
4. **Login-Reihenfolge:** Korrekt: lokal -> tenant LDAP -> global LDAP.
|
|
5. **Mail-ID-Validierung (SEC-22):** Alle 5 Handler pruefen vor dem Store-Zugriff. Regex einmalig kompiliert.
|
|
6. **SMTP Fail-Closed (SEC-26):** Leere Allowlist blockiert alle Verbindungen.
|
|
7. **Export Tenant-Isolation (SEC-05):** PDF und ZIP Export pruefen Tenant-Zugehoerigkeit. Superadmin hat Vollzugriff.
|
|
8. **Tenant-LDAP Endpunkte:** Authentifizierung und Rollenpruefung korrekt. Passwort wird maskiert.
|
|
9. **SQL-Injection:** Keine String-Konkatenation in Queries gefunden.
|