Skip to content

fix(security): harden recovery key entropy and reset-password brute-force protection - #2477

Open
fishzjp wants to merge 2 commits into
langbot-app:masterfrom
fishzjp:fix/recovery-key-hardening
Open

fix(security): harden recovery key entropy and reset-password brute-force protection#2477
fishzjp wants to merge 2 commits into
langbot-app:masterfrom
fishzjp:fix/recovery-key-hardening

Conversation

@fishzjp

@fishzjp fishzjp commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #2392(对应 GHSA-4xcp-6758-rxqv)。

概述 / Overview

恢复密钥此前仅由 secrets.token_hex(3) 生成(6 位 hex = 24 位熵,全键空间 1677 万),而未鉴权的 POST /api/v1/user/reset-password 端点唯一的防自动化措施是固定的 asyncio.sleep(3)——异步 sleep 不限制并发吞吐,攻击者可用并发请求在数小时内穷举键空间并重置管理员密码。

三层加固:

  1. 熵提升:新恢复密钥改用 secrets.token_urlsafe(32)(256 位,与 API key 强度对齐)。已配置的旧密钥不会被自动重置(避免锁死现有部署),但检测到旧低熵密钥(长度 < 16)时启动输出 warning 提示运维更换。
  2. 失败锁定:端点增加失败计数器,5 次错误密钥后锁定 15 分钟——锁定期间所有请求(含正确密钥)一律 429,且在 sleep 与任何服务调用之前快速拒绝。成功重置后计数清零,正常用户输错几次不会被误锁。
  3. 恒时比较:密钥比较改为 hmac.compare_digest(bytes 编码),非字符串 / 非 ASCII 载荷安全降级为拒绝,消除时序侧信道。

更改前后对比 / Before & After

修改前(并发爆破,sleep(3) 形同虚设):

  • 24 位键空间,1000 并发 ≈ 333 次/秒,平均 ~7 小时穷举 → 管理员密码被接管

修改后:

  • 新部署密钥 256 位熵,穷举不可行
  • 每个实例每 15 分钟最多试 5 次(年均上限 ~1.7 万次尝试 vs 2^256 键空间)
  • 旧低熵密钥部署:启动日志提示更换 + 端点锁定兜底

验证 / Verification

  • 新增回归测试 11 个(tests/unit_tests/api/test_user_reset_password.py),修复前全部失败(_recovery_key_state 不存在 / 旧 6 字符密钥),修复后全部通过,覆盖:
    • 新密钥熵 ≥ 32 字符、已有密钥不重置、旧低熵密钥保留 + warning
    • 正确密钥重置成功、错误密钥 403 且不触碰密码
    • 5 次失败后锁定(正确密钥也 429)、锁定期在 sleep / 用户查询之前快速拒绝、锁定期过恢复、成功后计数清零
    • 非字符串 / 非 ASCII 密钥载荷 fail-closed 不 500
  • uv run pytest tests/unit_tests -q — 2807 passed, 1 skipped(master 基线 2796 + 11)
  • uv run pytest tests/integration/api/test_smoke.py -k recovery — 通过(存量集成测试兼容)
  • e2e 兼容性实证:token_urlsafe(32) 产物经 skills/scripts/e2e 的读取正则 + YAML 往返 1000 次模拟全部匹配
  • uv run ruff check / ruff format --check — 通过

兼容性说明 / Compatibility

  • 旧密钥继续有效(不自动重置),仅提示更换——升级不锁死任何现有部署
  • 锁定为进程内计数器:单进程部署(当前默认)完全生效;重启清零属预期权衡
  • 前端无需改动:429 响应体格式与既有错误一致({"code":-1,"msg":...}

检查清单 / Checklist

PR 作者完成 / For PR author

项目维护者完成 / For project maintainer

  • 相关 issues 链接了吗? / Have you linked the related issues? — Unauthenticated admin account takeover via low-entropy (24-bit) recovery key and missing brute-force protection #2392
  • 配置项写好了吗?迁移写好了吗?生效了吗? / Have you written the configuration items? Have you written the migration? Has it taken effect? — 不涉及(复用现有 system.recovery_key 配置键)
  • 依赖加到 pyproject.toml 和 core/bootutils/deps.py 了吗 / Have you added the dependencies to pyproject.toml and core/bootutils/deps.py? — 不涉及(仅标准库 hmac/time/logging)
  • 文档编写了吗? / Have you written the documentation? — 不涉及(配置项含义未变,仅强度提升)

…orce protection

The recovery key was generated with only 24 bits of entropy
(secrets.token_hex(3)), and the unauthenticated reset-password endpoint
relied solely on a fixed asyncio.sleep(3), which does not throttle
concurrent requests. An attacker could exhaust the keyspace in hours and
take over the admin account (GHSA-4xcp-6758-rxqv).

- Generate recovery keys with secrets.token_urlsafe(32), matching the
  API-key strength; existing configured keys are preserved, and legacy
  low-entropy keys trigger a startup warning instead of being reset.
- Add a failure-counter lockout on the endpoint: after 5 wrong keys
  every attempt (including correct ones) is rejected with 429 for 15
  minutes, checked before the sleep and any service call.
- Compare the key with hmac.compare_digest on encoded bytes so
  malformed or non-ASCII payloads fail closed instead of leaking a
  timing side channel.

Fixes langbot-app#2392.
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug? Bug或Bug修复相关 / maybe a bug labels Aug 26, 2026
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@dadachann dadachann left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two blocking issues remain:

  1. The web reset-password form is still hard-coded to a six-character, alphanumeric OTP and uppercases the value. token_urlsafe(32) produces a case-sensitive 43-character base64url key (including possible -/_), so newly generated keys cannot be submitted through the UI.
  2. The lockout check happens before an await, while failure accounting happens after multiple awaits. Concurrent requests can all pass the initial check before any request sets locked_until; a 20-request barrier test produced 20×403 and 0×429. This does not enforce the claimed five attempts per 15 minutes and leaves the concurrent brute-force path open.

Please adapt the frontend input and make attempt admission/accounting concurrency-safe, with regression tests for both.

ap.instance_config.data['system']['recovery_key'] = secrets.token_hex(3).upper()
# 256-bit key, aligned with the API key strength; the legacy 24-bit
# key (token_hex(3)) was brute-forceable within hours (#2392).
ap.instance_config.data['system']['recovery_key'] = secrets.token_urlsafe(32)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking frontend compatibility issue: token_urlsafe(32) is 43 characters and case-sensitive, with -/_ possible. web/src/app/reset-password/page.tsx still uses InputOTP maxLength={6}, an alphanumeric-only pattern, six slots, and uppercases every value. A newly generated recovery key therefore cannot be entered through the UI. Please update the reset form and add frontend coverage.

new_password = json_data['new_password']

# Reject while locked out, before any sleep or service call (#2392)
if time.monotonic() < _recovery_key_state['locked_until']:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking concurrency bypass: every concurrent request can pass this check before the first await asyncio.sleep(3), while the failure counter/lock is only updated after later awaits. In a 20-request barrier test, all 20 wrong guesses returned 403 (none returned 429), despite the five-attempt limit. Attempt admission/accounting needs to be atomic/concurrency-safe, and the regression test should launch concurrent requests.

Admission check and slot bump now share one await-free synchronous
critical section, so concurrent bursts within a single event loop can
no longer slip past failure accounting (langbot-app#2392). Quota uses a rolling
15-minute fixed window and every admitted attempt consumes it,
throttling both legacy 24-bit keyspace exhaustion and brute-force on
modern high-entropy keys.

Web: replace the digit-capped OTP input with a plain monospace field
so full-length case-sensitive recovery keys can be entered verbatim.

Tests: rewrite unit coverage around fixed-window semantics and add a
concurrent-burst regression asserting {403: 5, 429: 15} for 20-way
races.
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 27, 2026
@fishzjp

fishzjp commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up hardening — a4a8458b

Self-review of this approach surfaced two blockers in the current implementation; this commit fixes both and re-audits the result end-to-end.

Problems found

  1. Concurrent bypass (backend): failure accounting was incremented after awaiting on the request body / service call, so a 20-request burst could all pass the check before any counter bump — yielding {403: 20} instead of throttling.
  2. Key entry blocked (web): the OTP-style input enforced a digits-only regex with maxLength=6, making a 43-char case-sensitive recovery key impossible to enter; input was also force-uppercased.

Fixes

  • Backend: new _admit_reset_attempt() runs a fixed-window admission gate before any await point — the check and slot bump share one synchronous critical section, which is atomic under the single-threaded event loop without introducing a lock that would break test-time monkeypatching of asyncio. Every admitted attempt consumes quota regardless of outcome. Window: 5 attempts / 15 min, based on time.monotonic().
  • Web: replaced the capped OTP input with a plain monospace Input; keys are now sent verbatim (case-sensitive). The enterRecoveryKey i18n key already exists in all 8 locales.

Security review findings

No Critical/High issues. Two known trade-offs documented in code comments:

  1. Quota is consumed unconditionally — an attacker can burn out the window for a legitimate admin (same exposure as the previous lockout design; the 15-min rolling window mitigates).
  2. Limiter state is process-local — multi-worker deployments need a shared limiter upstream.

Low / out-of-scope: pre-existing 'User not found' vs 'Invalid recovery key' user-enumeration surface (unchanged semantics).

Regression evidence

Check Result
Concurrent-burst regression (asserts exactly {403: 5, 429: 15}) ×10 runs 10/10 pass
Unit tests (target file) 12 pass
Full unit suite 2808 pass, 1 skip
Integration suite 208 pass, 33 skip (env-dependent services)
ruff check + ruff format --check pass
web tsc --noEmit pass
web eslint (full project) 0 errors; 34 warnings all pre-existing outside touched files
Adversarial script: burst distribution / honest path / window rollover 3/3 phases PASS

Also verified per repo conventions: /reset-password is AuthType.NONE and not exposed by LangBot's own MCP server, so no MCP tool / skill sync was required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug? Bug或Bug修复相关 / maybe a bug size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unauthenticated admin account takeover via low-entropy (24-bit) recovery key and missing brute-force protection

2 participants