Skip to content

A client-supplied identity must not address an existing object - #1690

Open
rubenvdlinde wants to merge 1 commit into
developmentfrom
fix/client-supplied-identity-cannot-address-an-object
Open

A client-supplied identity must not address an existing object#1690
rubenvdlinde wants to merge 1 commit into
developmentfrom
fix/client-supplied-identity-cannot-address-an-object

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Pre-existing. Found by sweeping the defect class from #1687, not from a report. Proven on a running instance before and after the fix.

The defect

ObjectService::saveObject() does not take its target as an argument the caller controls:

$providedId = $object['@self']['id'] ?? $object['id'] ?? null;

A match is treated as the uuid to UPDATE, and the write is PUT-semantic, so keys the payload omits are nulled.

Seven controllers build their payload from $this->request->getParams(). Several already stripped id for exactly this reason — the intent was right — but none stripped @self, which is the key saveObject reads first.

Measured, not reasoned

Against the mandate-roles create endpoint:

POST /api/mandate/rollen   {"roleName": "...", "@self": {"id": <victim uuid>}}
 -> 201, and the created id IS the victim's uuid
 -> the victim's row now holds the attacker's values

Any authenticated user could replace an arbitrary object of that schema, through an endpoint whose entire job is to create one.

The fix

Mostly central. ConfiguredRegistryService::save() already takes $id for the update case, so the payload's identity is stripped there and that parameter becomes the only way to address an existing object — which is what it was always for. Controllers that do not route through that service get the same strip at their own client boundary.

NrcController was checked and deliberately left alone: it echoes the body back in a response and writes nothing.

Evidence, in the order taken

  1. The live exploit above.
  2. A unit test with a negative control — removing the strip fails both new tests.
  3. A second test that the explicit $id still updates, so the guard cannot have traded a security hole for silent duplication.
  4. The same live exploit re-run against the fixed code:
{ "attackerGotDifferentId": true,
  "victimNameNow": "VICTIM2-original",
  "VERDICT": "SAFE — victim untouched, attacker got a new object" }

2769 tests, phpcs clean.

🤖 Generated with Claude Code

…object

PRE-EXISTING, and found by sweeping the class of the store defect in #1687
rather than by a report. Proven on a running instance, before and after.

`ObjectService::saveObject()` does not take its target as an argument the
caller controls. `extractUuidAndNormalizeObject()` reads
`$object['@self']['id'] ?? $object['id']` and treats a match as the uuid to
UPDATE, and the write is PUT-semantic, so keys the payload omits are NULLED.

Seven controllers build their payload from `$this->request->getParams()`.
Several already stripped `id` for exactly this reason — the intent was right —
but none stripped `@self`, which is the key saveObject reads FIRST.

MEASURED, not reasoned. Against the mandate-roles CREATE endpoint:

  POST /api/mandate/rollen  {"roleName": "...", "@self": {"id": <victim>}}
  -> 201, and the created id IS the victim's uuid
  -> the victim's row now holds the attacker's values

Any authenticated user could replace an arbitrary object of that schema
through an endpoint whose whole job is to create one.

The fix is mostly central. `ConfiguredRegistryService::save()` already takes
`$id` for the update case, so the payload's identity is stripped there and the
parameter becomes the ONLY way to address an existing object — which is what
it was always for. The controllers that do not route through that service get
the same strip at their own client boundary.

`NrcController` was checked and left alone: it echoes the body back in a
response and writes nothing.

Evidence, in the order it was taken: the live exploit above; a unit test with a
negative control (removing the strip fails both new tests); a second test that
the explicit `$id` still updates, so the guard cannot have traded a security
hole for silent duplication; and the same live exploit re-run against the fixed
code, where the attacker now gets a NEW object and the victim is untouched.

2769 tests, phpcs clean.
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/dossiq @ ae0dc7a

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue3-compile
test-l10n
format
check-schema-l10n
check-l10n-js
composer ✅ 106/106
npm ✅ 540/540
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-02 19:29 UTC

Download the full PDF report from the workflow artifacts.

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