## 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.
