diff --git a/docs/CODING-GUIDELINES-CORE.md b/docs/CODING-GUIDELINES-CORE.md new file mode 100644 index 0000000..ea4d48f --- /dev/null +++ b/docs/CODING-GUIDELINES-CORE.md @@ -0,0 +1,73 @@ +# NEXARCH Core – Projektweite Sicherheits-Coding-Konventionen + +Stand: 2026-08-28. Ticket: IAM-15. Ergänzt `docs/TESTSTRATEGIE-CORE.md` (QA-01) um Coding-Regeln, +die als Code-Review-Checkliste gelten — keine dieser Regeln ist optional oder "nur für ein Modul". + +## 1. Warum dieses Dokument existiert + +Eine sicherheitsrelevante Coding-Regel, die nur einmal an einer Stelle vorgemacht statt projektweit +verankert wird, wird beim nächsten neuen Vergleich vergessen. Das gilt für jede Regel in diesem +Dokument gleichermaßen — die erste Regel (SQL) ist bereits als Konvention etabliert, die zweite +(timing-safe Vergleich, IAM-15) macht sie hier zum ersten Mal explizit schriftlich. + +## 2. Regel: Kein `fmt.Sprintf` für SQL-Bestandteile aus Nutzereingabe + +**Spalten-/Tabellennamen ausschließlich aus statischen Konstanten bzw. einem geschlossenen +Enum/Switch-Typ, nie aus Nutzereingabe oder generischem String-Zusammenbau — auch nicht hinter +einer Whitelist-Funktion.** Werte (nicht Bezeichner) gehören als Parameter (`$1`, `$2`, …) in die +Query, niemals interpoliert. + +Lehre aus beiden Altsystemen (`known-issues-archivdms.md` Punkt 10, `known-issues-archivmail.md` +Punkt 12): dynamische Tabellennamen via `fmt.Sprintf`, nur durch eine fragile Whitelist-Funktion +abgesichert. Siehe DMS/Mail `SRC-11` für die board-spezifische Umsetzung dieser Regel im +Suchindex-Kontext. + +**Referenzbeispiel (korrekt):** `internal/tenant/lifecycle.go`, `ProcessDueDeletions` — Statuswerte +und IDs ausschließlich als Parameter (`$1`, `$2`, …), niemals interpoliert; der einzige Einsatz von +`fmt.Sprintf` im Package baut einen **Datenbanknamen aus einem bereits validierten Slug** +(`dbNameForSlug`, `slugPattern` in `tenant.go` erzwingt `^[a-z][a-z0-9_]{1,48}$` vor jeder +Verwendung) — keine ungeprüfte Nutzereingabe erreicht die Query. + +## 3. Regel: Timing-safe Vergleich für jede sicherheitsrelevante Zugriffsentscheidung (IAM-15) + +**Jeder Vergleich, der eine sicherheitsrelevante Zugriffsentscheidung trifft — Passwort-Hash, Token, +Signatur, 2FA-Code/-Wiederherstellungscode — nutzt einen timing-safe/constant-time Vergleich, nie +den regulären `==`-Operator.** Ein naiver `==`-Vergleich zweier Byte-Folgen bricht bei der ersten +abweichenden Stelle ab; die dadurch messbare Laufzeitdifferenz lässt sich aus der Ferne ausmessen und +erlaubt ein Byte-für-Byte-Erraten des korrekten Werts (Timing-Angriff). + +**So wird es gemacht:** `internal/timingsafe` (dieses Ticket) bündelt die kanonische Implementierung +(`crypto/subtle.ConstantTimeCompare`) für neue Vergleichsstellen: + +```go +import "gitea.perlbach24.de/scripte/nexarch/internal/timingsafe" + +if !timingsafe.EqualString(providedCode, expectedCode) { + return ErrInvalid +} +``` + +Ausnahme: `bcrypt.CompareHashAndPassword` (Passwort-Hashes) ist bereits von Haus aus timing-safe — +hier ist kein zusätzlicher Wrapper nötig. + +### 3.1 Audit bestehender Vergleichsstellen (Prüfung 2) + +Durchgeführt 2026-08-28, Ergebnis: **alle bestehenden sicherheitsrelevanten Vergleichsstellen +implementierten die Regel bereits korrekt**, unabhängig voneinander mit `crypto/subtle` — nichts +musste korrigiert werden (Akzeptanzkriterium 3, „ggf."). + +| Ort | Was wird verglichen | Fundstelle | +|---|---|---| +| `internal/totp/totp.go`, `Validate` | TOTP-Code (2FA) | nutzte bereits `subtle.ConstantTimeCompare` direkt, in diesem Ticket auf `timingsafe.EqualString` umgestellt (erster Verwender des neuen Packages) | +| `internal/webhook/dispatcher.go`, `VerifySignature` | HMAC-Webhook-Signatur | `subtle.ConstantTimeCompare(expectedBytes, gotBytes)` | +| `internal/moduleregistry/credentials.go`, `Authenticate` | Service-Credential-Secret-Hash | eigene `timingSafeEqual`-Hilfsfunktion, gleiches Muster | +| `internal/authtoken/token.go`, `Consume` (Passwort-Reset/Einladung) | Einmal-Token | Hash-Lookup über DB-Index (`WHERE token_hash = $1`), kein manueller Byte-Vergleich nötig — bei zufälligen, hochentropischen Token ist der indexierte Hash-Abgleich gleichwertig sicher | + +Neue Vergleichsstellen sollen `internal/timingsafe` verwenden, statt das Muster erneut inline zu +duplizieren — bestehende Stellen müssen dafür nicht umgebaut werden (kein Umbau angrenzender +Bereiche über Board-Branch-Grenzen hinweg). + +## 4. Wie diese Liste wächst + +Neue projektweite Sicherheits-Coding-Regeln werden hier ergänzt, sobald sie (wie SQL-Sprintf und +timing-safe Vergleich) mehr als einmal unabhängig als Lehre auftauchen — nicht vorab spekulativ. diff --git a/internal/timingsafe/timingsafe.go b/internal/timingsafe/timingsafe.go new file mode 100644 index 0000000..fec1942 --- /dev/null +++ b/internal/timingsafe/timingsafe.go @@ -0,0 +1,30 @@ +// Package timingsafe stellt die kanonische Implementierung der projektweiten +// Coding-Konvention aus IAM-15 bereit: jeder Vergleich, der eine +// sicherheitsrelevante Zugriffsentscheidung trifft (Passwort-Hash, Token, +// Signatur, 2FA-Wiederherstellungscode), nutzt einen timing-safe/constant-time +// Vergleich, nie den regulaeren ==-Operator. Siehe docs/CODING-GUIDELINES-CORE.md. +// +// Bestehende Vergleichsstellen (internal/totp, internal/webhook, +// internal/moduleregistry) implementieren dasselbe Muster bereits inline mit +// crypto/subtle direkt — dieses Package buendelt es fuer neue Vergleichsstellen, +// ersetzt die bestehenden nicht zwangsweise (kein Umbau angrenzender Bereiche). +package timingsafe + +import "crypto/subtle" + +// Equal vergleicht zwei Byte-Slices timing-safe. Unterschiedliche Laenge gilt +// als "nicht gleich", ohne dass die Laufzeit dabei die Laenge verraet, die +// zum Ergebnis gefuehrt hat, mehr als durch den Laengenunterschied ohnehin +// unvermeidbar waere. +func Equal(a, b []byte) bool { + if len(a) != len(b) { + return false + } + return subtle.ConstantTimeCompare(a, b) == 1 +} + +// EqualString ist die String-Variante von Equal fuer den haeufigen Fall, +// dass beide Seiten bereits als string vorliegen (z. B. TOTP-Codes). +func EqualString(a, b string) bool { + return Equal([]byte(a), []byte(b)) +} diff --git a/internal/timingsafe/timingsafe_test.go b/internal/timingsafe/timingsafe_test.go new file mode 100644 index 0000000..02c6352 --- /dev/null +++ b/internal/timingsafe/timingsafe_test.go @@ -0,0 +1,36 @@ +package timingsafe + +import "testing" + +func TestEqual_SameBytes(t *testing.T) { + if !Equal([]byte("geheimnis"), []byte("geheimnis")) { + t.Fatal("identische Byte-Slices sollten gleich sein") + } +} + +func TestEqual_DifferentBytes(t *testing.T) { + if Equal([]byte("geheimnis"), []byte("anders123")) { + t.Fatal("unterschiedliche Byte-Slices sollten ungleich sein") + } +} + +func TestEqual_DifferentLength(t *testing.T) { + if Equal([]byte("kurz"), []byte("laengererstring")) { + t.Fatal("unterschiedliche Laenge sollte immer ungleich sein") + } +} + +func TestEqual_EmptyVsEmpty(t *testing.T) { + if !Equal([]byte(""), []byte("")) { + t.Fatal("zwei leere Slices sollten gleich sein") + } +} + +func TestEqualString_MatchesEqual(t *testing.T) { + if !EqualString("abc123", "abc123") { + t.Fatal("identische Strings sollten gleich sein") + } + if EqualString("abc123", "xyz789") { + t.Fatal("unterschiedliche Strings sollten ungleich sein") + } +} diff --git a/internal/totp/totp.go b/internal/totp/totp.go index e69160d..cadb7aa 100644 --- a/internal/totp/totp.go +++ b/internal/totp/totp.go @@ -7,12 +7,13 @@ import ( "crypto/hmac" "crypto/rand" "crypto/sha1" - "crypto/subtle" "encoding/base32" "encoding/binary" "fmt" "net/url" "time" + + "gitea.perlbach24.de/scripte/nexarch/internal/timingsafe" ) // StepSeconds ist das TOTP-Zeitfenster (RFC-6238-Standard: 30 Sekunden). @@ -70,7 +71,7 @@ func Validate(secret, code string, t time.Time) (bool, error) { for delta := -DefaultSkewSteps; delta <= DefaultSkewSteps; delta++ { candidate := hotp(key, uint64(counter+int64(delta))) - if subtle.ConstantTimeCompare([]byte(candidate), []byte(code)) == 1 { + if timingsafe.EqualString(candidate, code) { return true, nil } }