From d9759573350bb77ff81aff6d02b1308818d36527 Mon Sep 17 00:00:00 2001 From: zuevav Date: Fri, 18 Sep 2026 21:25:35 +0300 Subject: [PATCH] =?UTF-8?q?=D0=91=D0=B5=D0=B7=D0=BE=D0=BF=D0=B0=D1=81?= =?UTF-8?q?=D0=BD=D0=BE=D1=81=D1=82=D1=8C:=20fail-closed=20auth=20(H1),=20?= =?UTF-8?q?=D0=B4=D0=BE=D0=B2=D0=B5=D1=80=D0=B5=D0=BD=D0=BD=D1=8B=D0=B9=20?= =?UTF-8?q?=D0=BF=D1=80=D0=BE=D0=BA=D1=81=D0=B8=20=D0=B4=D0=BB=D1=8F=20IP?= =?UTF-8?q?=20(H2),=20CSRF=20=D0=BD=D0=B0=20/order=20(M2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit По результатам security-аудита (docs, run-1). Три верифицированные находки. H1 (HIGH) — fail-open аутентификация. loadUsers() при ошибке чтения/парсинга users.json молча возвращал пустой стор → authOpen() считал это первичной настройкой → после удаления first-run файла весь /admin отдавался без входа. Теперь: постоянный маркер configured (пишется при создании первого админа и бэкфиллится при загрузке валидного стора); authOpen() открыт только на реально свежей системе; при пустом/битом сторе на настроенной системе — 503 и блокировка до восстановления файла (authLockedOut). Тест TestAuthFailClosed (4 сценария). H2 (HIGH) — интернет-слой доверял клиентскому X-Forwarded-For/X-Real-IP, что позволяло обойти лимит входа по IP и травить аудит. Теперь заголовкам доверяем только от доверенного прокси (по умолчанию loopback; расширяется BJ_TRUSTED_PROXIES), из XFF берём правый элемент. Исправлено и в Go-модуле (modules/lk/security.go), и в PHP-ретрансляторе. Тесты TestClientIPTrust (Go) и прогон логики PHP. M2 (MEDIUM) — POST /order (поручение на списание бумаг) не проверял CSRF, в отличие от /login и /impersonate. Добавлен csrfOK (форма уже несла токен). Тест TestOrderCSRF. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Q4YVo4UMopqT5DzicqpPjL --- internal/lkgateway/auth.go | 73 +++++++++++++++++++--- internal/lkgateway/auth_failclosed_test.go | 63 +++++++++++++++++++ modules/lk/clientip_test.go | 41 ++++++++++++ modules/lk/lk.go | 9 ++- modules/lk/lk_test.go | 23 +++++++ modules/lk/relay-php/index.php | 40 ++++++++++-- modules/lk/security.go | 69 +++++++++++++++++--- 7 files changed, 297 insertions(+), 21 deletions(-) create mode 100644 internal/lkgateway/auth_failclosed_test.go create mode 100644 modules/lk/clientip_test.go diff --git a/internal/lkgateway/auth.go b/internal/lkgateway/auth.go index e6ab9bb..312fcf6 100644 --- a/internal/lkgateway/auth.go +++ b/internal/lkgateway/auth.go @@ -37,12 +37,14 @@ import ( ) const ( - usersFile = "/var/lib/bj/.bj/users.json" sessionsFile = "/var/lib/bj/.bj/sessions.json" sessionCookie = "bj_session" sessionMaxLife = 24 * time.Hour ) +// usersFile — стор учётных записей (var, чтобы подменять в тестах). +var usersFile = "/var/lib/bj/.bj/users.json" + // ---------- роли и права ---------- type roleDef struct { @@ -224,19 +226,51 @@ type userStore struct { var globalUsers = loadUsers() +// usersLoadFailed — файл учётных записей есть, но прочитать/разобрать его не +// удалось (повреждение, права). В этом случае НЕЛЬЗЯ открывать админку без входа: +// пустой стор из-за сбоя не должен выглядеть как «первичная настройка». +var usersLoadFailed bool + func loadUsers() *userStore { s := &userStore{users: map[string]*User{}} - if b, err := os.ReadFile(usersFile); err == nil { + b, err := os.ReadFile(usersFile) + switch { + case err == nil: var list []*User - if json.Unmarshal(b, &list) == nil { - for _, u := range list { - s.users[strings.ToLower(u.Login)] = u - } + if json.Unmarshal(b, &list) != nil { + usersLoadFailed = true // файл есть, но это не корректный JSON — сбой, не «нет пользователей» + return s } + for _, u := range list { + s.users[strings.ToLower(u.Login)] = u + } + if len(s.users) > 0 { + markConfigured() // бэкфилл маркера для установок, заведённых до этой защиты + } + case os.IsNotExist(err): + // файла нет: либо действительно первый запуск, либо стор потеряли на + // настроенной системе — различаем по постоянному маркеру ниже (authOpen). + default: + usersLoadFailed = true // есть, но недоступен (права и т. п.) } return s } +// configuredMarker — постоянный признак «система уже была настроена» (заведён +// хотя бы один пользователь). В отличие от first-run-файла он НЕ удаляется, и по +// нему видно, что нулевой стор — это потеря данных, а не свежая установка. +var configuredMarker = "/var/lib/bj/.bj/configured" + +func isConfigured() bool { _, err := os.Stat(configuredMarker); return err == nil } + +// markConfigured — отметить систему настроенной (идемпотентно, best-effort). +func markConfigured() { + if isConfigured() { + return + } + _ = os.WriteFile(configuredMarker, []byte(time.Now().UTC().Format(time.RFC3339)+"\n"), 0o600) +} + func (s *userStore) save() error { list := s.list() b, _ := json.MarshalIndent(list, "", " ") @@ -244,7 +278,13 @@ func (s *userStore) save() error { if err := os.WriteFile(tmp, b, 0o600); err != nil { return err } - return os.Rename(tmp, usersFile) + if err := os.Rename(tmp, usersFile); err != nil { + return err + } + if len(list) > 0 { + markConfigured() // с этого момента нулевой стор трактуется как потеря данных + } + return nil } func (s *userStore) list() []*User { @@ -328,7 +368,17 @@ func checkPassword(hash, p string) bool { } // authOpen — режим первичной настройки: пользователей нет, вход не требуется. -func authOpen() bool { return globalUsers.Count() == 0 } +// authOpen — режим первичной настройки: вход не требуется. Открываем ТОЛЬКО на +// действительно свежей системе — стор пуст, не было сбоя загрузки и система ещё +// не отмечена настроенной. Потеря/порча users.json на настроенной системе сюда +// НЕ попадает (fail-closed): её ловит запрет в authMiddleware. +func authOpen() bool { return globalUsers.Count() == 0 && !usersLoadFailed && !isConfigured() } + +// authLockedOut — стор недоступен/пуст на уже настроенной системе: админку +// блокируем, пока файл не восстановят. +func authLockedOut() bool { + return globalUsers.Count() == 0 && (usersLoadFailed || isConfigured()) +} // ---------- сессии ---------- @@ -890,6 +940,13 @@ func authMiddleware(rc *RuntimeConfig, next http.Handler) http.Handler { next.ServeHTTP(&authRW{ResponseWriter: w, user: nil}, r) return } + if authLockedOut() { + // Учётные записи потеряны/повреждены на настроенной системе. + // НЕ открываем админку без входа — блокируем до восстановления файла. + log.Printf("auth: админ-панель заблокирована — стор учётных записей недоступен (usersLoadFailed=%v, configured=%v)", usersLoadFailed, isConfigured()) + http.Error(w, "Учётные записи недоступны — админ-панель заблокирована. Восстановите /var/lib/bj/.bj/users.json из резервной копии и перезапустите bj-server.", http.StatusServiceUnavailable) + return + } var u *User if c, err := r.Cookie(sessionCookie); err == nil && c.Value != "" { if s := globalSessions.Get(c.Value, rc.Snapshot().Auth.idle()); s != nil { diff --git a/internal/lkgateway/auth_failclosed_test.go b/internal/lkgateway/auth_failclosed_test.go new file mode 100644 index 0000000..240c034 --- /dev/null +++ b/internal/lkgateway/auth_failclosed_test.go @@ -0,0 +1,63 @@ +package lkgateway + +import ( + "os" + "path/filepath" + "testing" +) + +// H1: пустой/битый users.json на настроенной системе НЕ должен открывать админку. +func TestAuthFailClosed(t *testing.T) { + dir := t.TempDir() + oldUsers, oldMarker, oldStore, oldFailed := usersFile, configuredMarker, globalUsers, usersLoadFailed + t.Cleanup(func() { + usersFile, configuredMarker, globalUsers, usersLoadFailed = oldUsers, oldMarker, oldStore, oldFailed + }) + usersFile = filepath.Join(dir, "users.json") + configuredMarker = filepath.Join(dir, "configured") + + reload := func() { + usersLoadFailed = false + globalUsers = loadUsers() + } + + // 1) свежая система: файла нет, маркера нет → открыто для первичной настройки + reload() + if !authOpen() || authLockedOut() { + t.Fatalf("свежая система: ожидали open, got open=%v locked=%v", authOpen(), authLockedOut()) + } + + // 2) корректный стор с админом → не открыто, не заблокировано, маркер создан + if err := os.WriteFile(usersFile, []byte(`[{"login":"admin","role":"admin","active":true}]`), 0o600); err != nil { + t.Fatal(err) + } + reload() + if authOpen() || authLockedOut() { + t.Fatalf("настроенная система: open=%v locked=%v", authOpen(), authLockedOut()) + } + if !isConfigured() { + t.Fatal("маркер настройки не создан при загрузке валидного стора") + } + + // 3) файл ПОВРЕЖДЁН (битый JSON) → fail-closed + if err := os.WriteFile(usersFile, []byte(`{ не json`), 0o600); err != nil { + t.Fatal(err) + } + reload() + if authOpen() { + t.Error("битый users.json открыл админку без входа — fail-open!") + } + if !authLockedOut() { + t.Error("битый users.json должен блокировать админку") + } + + // 4) файл ПРОПАЛ, но система была настроена (маркер есть) → fail-closed + os.Remove(usersFile) + reload() + if authOpen() { + t.Error("пропавший users.json на настроенной системе открыл админку — fail-open!") + } + if !authLockedOut() { + t.Error("пропавший стор на настроенной системе должен блокировать админку") + } +} diff --git a/modules/lk/clientip_test.go b/modules/lk/clientip_test.go new file mode 100644 index 0000000..caa13e9 --- /dev/null +++ b/modules/lk/clientip_test.go @@ -0,0 +1,41 @@ +package lk + +import ( + "net/http" + "testing" +) + +// H2: X-Forwarded-For/X-Real-IP от НЕдоверенного источника не должны подменять IP. +func TestClientIPTrust(t *testing.T) { + req := func(remote string, hdr map[string]string) *http.Request { + r, _ := http.NewRequest("POST", "/login", nil) + r.RemoteAddr = remote + for k, v := range hdr { + r.Header.Set(k, v) + } + return r + } + // прямой клиент из интернета подставляет заголовки — игнорируем + if got := clientIP(req("203.0.113.7:5000", map[string]string{"X-Real-IP": "1.2.3.4", "X-Forwarded-For": "9.9.9.9"})); got != "203.0.113.7" { + t.Errorf("недоверенный источник: ожидали 203.0.113.7, получили %s (спуфинг прошёл)", got) + } + // доверенный прокси (loopback) сообщает реальный адрес — доверяем X-Real-IP + if got := clientIP(req("127.0.0.1:5000", map[string]string{"X-Real-IP": "1.2.3.4"})); got != "1.2.3.4" { + t.Errorf("loopback X-Real-IP: ожидали 1.2.3.4, получили %s", got) + } + // доверенный прокси, только XFF: берём ПРАВЫЙ элемент (левые подставил клиент) + if got := clientIP(req("127.0.0.1:5000", map[string]string{"X-Forwarded-For": "9.9.9.9, 5.6.7.8"})); got != "5.6.7.8" { + t.Errorf("loopback XFF: ожидали правый 5.6.7.8, получили %s", got) + } + // без заголовков — адрес соединения + if got := clientIP(req("198.51.100.2:443", nil)); got != "198.51.100.2" { + t.Errorf("без заголовков: ожидали 198.51.100.2, получили %s", got) + } + // настраиваемый доверенный прокси + old := trustedProxies + trustedProxies = loadTrustedProxies("10.0.0.0/8") + defer func() { trustedProxies = old }() + if got := clientIP(req("10.1.2.3:5000", map[string]string{"X-Real-IP": "1.2.3.4"})); got != "1.2.3.4" { + t.Errorf("доверенный CIDR: ожидали 1.2.3.4, получили %s", got) + } +} diff --git a/modules/lk/lk.go b/modules/lk/lk.go index dd2219f..dcd0f40 100644 --- a/modules/lk/lk.go +++ b/modules/lk/lk.go @@ -1082,7 +1082,14 @@ func (s *Server) buildHandler() http.Handler { "PickISIN": r.URL.Query().Get("isin"), "Today": time.Now().Format("02.01.2006")}) return } - _ = r.ParseForm() + // Изменяющее действие (поручение на списание бумаг) — только по своей форме + // с CSRF-токеном, как /login и /impersonate. Иначе чужая страница могла бы + // отправить поручение от имени вошедшего клиента. + if !csrfOK(r) { + setFlashCookie(w, r, "Форма устарела, откройте поручение заново.") + http.Redirect(w, r, "/order", http.StatusSeeOther) + return + } var secs []map[string]any for _, isin := range r.Form["isin"] { q := strings.TrimSpace(r.FormValue("qty_" + isin)) diff --git a/modules/lk/lk_test.go b/modules/lk/lk_test.go index e7cb9b9..840fb68 100644 --- a/modules/lk/lk_test.go +++ b/modules/lk/lk_test.go @@ -614,3 +614,26 @@ func TestChartAndProfile(t *testing.T) { t.Error("POST /documents всё ещё принимает заказ документа") } } + +// M2: POST /order без валидного CSRF-токена не должен уходить в ядро. +func TestOrderCSRF(t *testing.T) { + var transferCalled bool + bj := fakeBJ(t) + // добавим перехват transfer поверх fakeBJ нельзя (mux фиксирован) — проверяем по редиректу. + s, err := New(Config{BJURL: bj.URL, BJKey: "lk-key", Title: "Кабинет"}) + if err != nil { + t.Fatal(err) + } + h := s.Handler() + // без csrf-куки и поля — ожидаем редирект на /order, поручение не подаётся + form := url.Values{"isin": {"RU0009029540"}, "qty_RU0009029540": {"10"}, "receiving_inn": {"7700000000"}} + req := httptest.NewRequest(http.MethodPost, "/order", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(&http.Cookie{Name: "bj_lk", Value: "sess-1"}) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/order" { + t.Errorf("POST /order без CSRF: ожидали редирект на /order, got %d %s", rec.Code, rec.Header().Get("Location")) + } + _ = transferCalled +} diff --git a/modules/lk/relay-php/index.php b/modules/lk/relay-php/index.php index 1b83a96..94ed3cf 100644 --- a/modules/lk/relay-php/index.php +++ b/modules/lk/relay-php/index.php @@ -97,14 +97,46 @@ function isLocal(): bool return $h === 'localhost' || $h === '127.0.0.1' || $h === '::1'; } +// peerTrusted — непосредственный источник запроса (REMOTE_ADDR) — доверенный прокси: +// loopback или адрес/CIDR из переменной окружения BJ_TRUSTED_PROXIES (через запятую). +function peerTrusted(string $remote): bool +{ + if ($remote === '') return false; + if ($remote === '127.0.0.1' || $remote === '::1' || str_starts_with($remote, '127.')) return true; + $list = getenv('BJ_TRUSTED_PROXIES') ?: ''; + foreach (explode(',', $list) as $cidr) { + $cidr = trim($cidr); + if ($cidr === '') continue; + if (!str_contains($cidr, '/')) { if ($remote === $cidr) return true; continue; } + [$net, $bits] = explode('/', $cidr, 2); + $ipb = @inet_pton($remote); $netb = @inet_pton($net); + if ($ipb === false || $netb === false || strlen($ipb) !== strlen($netb)) continue; + $bytes = intdiv((int)$bits, 8); $rem = (int)$bits % 8; + if (($bytes > 0 && substr($ipb, 0, $bytes) !== substr($netb, 0, $bytes))) continue; + if ($rem === 0) return true; + $mask = chr((0xff << (8 - $rem)) & 0xff); + if ((ord($ipb[$bytes]) & ord($mask)) === (ord($netb[$bytes]) & ord($mask))) return true; + } + return false; +} + +// clientIP — адрес посетителя. X-Real-IP / X-Forwarded-For подставляет клиент, поэтому +// доверяем им ТОЛЬКО от доверенного прокси; иначе берём адрес соединения. Иначе +// злоумышленник подделал бы свой IP и обошёл лимит входа и журнал аудита в ядре. function clientIP(): string { - foreach (['HTTP_X_REAL_IP', 'HTTP_X_FORWARDED_FOR'] as $k) { - if (!empty($_SERVER[$k])) { - return trim(explode(',', $_SERVER[$k])[0]); + $remote = $_SERVER['REMOTE_ADDR'] ?? ''; + if (peerTrusted($remote)) { + if (!empty($_SERVER['HTTP_X_REAL_IP'])) { + return trim($_SERVER['HTTP_X_REAL_IP']); + } + if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { + // правый элемент добавил наш доверенный прокси; левые может подставить клиент + $parts = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']); + return trim($parts[count($parts) - 1]); } } - return $_SERVER['REMOTE_ADDR'] ?? ''; + return $remote; } function keyOK(string $key): bool diff --git a/modules/lk/security.go b/modules/lk/security.go index d543383..1a44fd0 100644 --- a/modules/lk/security.go +++ b/modules/lk/security.go @@ -11,6 +11,7 @@ import ( "log" "net" "net/http" + "os" "strings" ) @@ -56,17 +57,69 @@ func secureCookies(r *http.Request) bool { return httpsRequest(r) || !localRequest(r) } -// clientIP — адрес посетителя кабинета (за nginx — из X-Real-IP / X-Forwarded-For). +// trustedProxies — сети, которым разрешено сообщать адрес клиента заголовками +// X-Real-IP / X-Forwarded-For. По умолчанию — только loopback (nginx на том же +// хосте). Расширяется списком CIDR/адресов в BJ_TRUSTED_PROXIES (через запятую). +var trustedProxies = loadTrustedProxies(os.Getenv("BJ_TRUSTED_PROXIES")) + +func loadTrustedProxies(s string) []*net.IPNet { + var out []*net.IPNet + for _, part := range strings.Split(s, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + if !strings.Contains(part, "/") { + if strings.Contains(part, ":") { + part += "/128" + } else { + part += "/32" + } + } + if _, n, err := net.ParseCIDR(part); err == nil { + out = append(out, n) + } + } + return out +} + +// peerTrusted — непосредственный источник запроса (адрес соединения) — доверенный +// прокси: loopback или явно указанный в BJ_TRUSTED_PROXIES. +func peerTrusted(host string) bool { + ip := net.ParseIP(host) + if ip == nil { + return false + } + if ip.IsLoopback() { + return true + } + for _, n := range trustedProxies { + if n.Contains(ip) { + return true + } + } + return false +} + +// clientIP — адрес посетителя кабинета. Заголовки X-Real-IP / X-Forwarded-For +// подставляет клиент, поэтому доверяем им ТОЛЬКО когда запрос пришёл от доверенного +// прокси (по умолчанию nginx на том же хосте). Иначе адрес берём из соединения — +// иначе злоумышленник подделал бы свой IP и обошёл лимит входа и журнал аудита. func clientIP(r *http.Request) string { - if v := strings.TrimSpace(r.Header.Get("X-Real-IP")); v != "" { - return v - } - if v := strings.TrimSpace(r.Header.Get("X-Forwarded-For")); v != "" { - return strings.TrimSpace(strings.Split(v, ",")[0]) - } host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { - return r.RemoteAddr + host = r.RemoteAddr + } + if peerTrusted(host) { + if v := strings.TrimSpace(r.Header.Get("X-Real-IP")); v != "" { + return v + } + if v := strings.TrimSpace(r.Header.Get("X-Forwarded-For")); v != "" { + // самый правый элемент добавляет наш доверенный прокси (его peer); + // левые может подставить клиент, поэтому берём правый. + parts := strings.Split(v, ",") + return strings.TrimSpace(parts[len(parts)-1]) + } } return host }