Skip to content

sec: require proof of key possession on POST /users/register - #363

Merged
ericmt-98 merged 1 commit into
mainfrom
fix/register-key-possession
Aug 25, 2026
Merged

sec: require proof of key possession on POST /users/register#363
ericmt-98 merged 1 commit into
mainfrom
fix/register-key-possession

Conversation

@ericmt-98

Copy link
Copy Markdown
Collaborator

Extracted from #344, which has been blocked in CONFLICTING since 2026-07-27. That PR carries four security fixes hostage behind 62 files of map work; this is the third and most serious of them (#361 and #362 cover the other two).

The vulnerability

POST /users/register on main accepts stellar_address and username and nothing else. It never verifies that the caller controls the address they are registering.

Stellar addresses are public. Anyone can take someone else's address off-chain and register it before they do — address squatting. The victim then finds their own address already taken, bound to an account they do not control.

The challenge/response machinery already existed for /auth/token (login). It simply was not applied to signup, which is where it matters most.

What changed

  • services/challenge.service.ts (new) — the challenge store, shared by /auth/token and /users/register, with issueChallenge() and verifyAndConsumeChallenge(). Challenges are address-bound, expiring and single-use.
  • routes/users.ts — register now requires challenge + signature, and validates the address with StrKey.isValidEd25519PublicKey before anything else.
  • routes/auth.ts — uses the shared module instead of its own local store.
  • frontend/src/services/api.tsregisterUser performs the challenge/sign round trip before posting.

Conflict resolutions, for review

Three blocks conflicted against 44 commits of drift. Each decision:

1. Memory-leak protections were ported, not dropped. The shared module as written had no size cap and no pruning. main had both, from the memory-leak fix in #341 (SEC-16). Moving the store without moving its bounds would have silently reintroduced that leak — an attacker rotating IPs grows the Map without limit. CHALLENGES_MAX_SIZE, oldest-entry eviction and the 60s prune interval with .unref() are now in challenge.service.ts, with a comment naming their origin so they do not get deleted again as apparent duplication.

2. phoneHash was preserved. main gained registerUser(username, phoneHash?) from the anti-abuse work in #319, and Register.tsx:53 passes it. It is forwarded untouched, orthogonal to key possession.

3. Map work was excluded. The conflicting hunk also carried MerchantLocation / updateMerchantLocation, which are not on main and belong to the map feature. They stay in #344; this PR is security only.

Also dropped generateFallbackAddress from registerUser: a synthetic address whose key the device does not hold could never sign the challenge, so it would fail server-side regardless.

Tests

The test file that came with the original commit was not running correctly. It declared MOCK_STELLAR=true in its docstring but never set it, and that default changed on main, so it failed on invalid checksum. It is now self-sufficient via a dynamic import after setting the env — config is built at import time, so a static import evaluates too early.

More importantly, those tests run in mock mode, where signature bytes are not verified at all. They cover address binding, expiry and single-use, but not the actual cryptography. Added challengeSignature.test.ts, which runs with MOCK_STELLAR=false:

  • a valid signature from the key holder is accepted
  • a signature made with a different key is rejected — this is the squatting case
  • a malformed signature is rejected without an uncaught throw
  • a valid signature for a different challenge is rejected

It lives in its own file on purpose: config is cached per process, so both modes cannot coexist in one run. A dynamic-import query trick appears to work but does not — it creates a fresh service module that still resolves the cached config, so verification stays skipped and the test passes without testing anything.

tsc backend               OK    tsc frontend              OK
npm run build             OK
test:security             OK    test:rate-limit           OK
test:challenge            OK    test:challenge-signature  OK

Behaviour change worth knowing

registerUser now requires a device keypair to exist. Of the three call sites, Register.tsx:53 generates one first and recoverSession is keypair-based by definition. The third is the demo-mode auto-provision at App.tsx:876, which does not generate one in that block and will now throw instead of creating a throwaway user. That seems correct — provisioning a user against an address nobody controls is exactly what this PR prevents — but if demo mode is in use, it needs a generateAndStoreKeypair() call there.

POST /users/register aceptaba cualquier stellar_address (solo validaba
longitud 56) y devolvia un JWT de 24h: cualquiera podia registrar la
direccion publica de otra persona antes que ella. Ver el finding
"Registro sin prueba de posesion de llave" en AUDIT_MOBILE_MAINNET.md.

- Extrae el challenge/response de auth.ts a challenge.service.ts
  (issueChallenge/verifyAndConsumeChallenge), ahora compartido por
  /auth/token y /users/register.
- register exige challenge+signature y valida con StrKey.
- El JWT de registro ahora lleva jti, asi que es revocable desde el
  primer momento (antes solo lo era tras el primer login).
- Frontend: registerUser() hace el mismo baile challenge -> firma ->
  registro; se elimina generateFallbackAddress de registerUser y
  getAuthToken (fabricaba direcciones invalidas en vez de fallar).

CAMBIO DE CONTRATO DE API: 2 campos nuevos requeridos. Backend y APK
deben desplegarse juntos; un APK viejo contra este backend recibe 400
en el registro.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ericmt-98
ericmt-98 merged commit 49aba1f into main Aug 25, 2026
2 checks passed
@ericmt-98
ericmt-98 deleted the fix/register-key-possession branch August 25, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant