Skip to content

security(MEDIUM): bound canonicalJson recursion (DoS via deep JSON + Idempotency-Key)#365

Open
CryptoJones wants to merge 1 commit into
masterfrom
security/idempotency-canonical-json-recursion-limit
Open

security(MEDIUM): bound canonicalJson recursion (DoS via deep JSON + Idempotency-Key)#365
CryptoJones wants to merge 1 commit into
masterfrom
security/idempotency-canonical-json-recursion-limit

Conversation

@CryptoJones
Copy link
Copy Markdown
Owner

canonicalJson() recurses once per nesting level. A POST that
includes an Idempotency-Key header reaches hashBody(req.body) -> canonicalJson(...) synchronously inside the middleware. With Node's
default ~10000-frame call stack, depth ~5000 blows the stack with
RangeError — which Express's async error path surfaces as a 500.

Reach: any POST endpoint mounted under /v1 (every CREATE handler and
every /bulk handler). Body limit is 100KB (env-tunable), allowing up
to ~20000 nesting levels at 5 chars per level ({"a":), so a
single 25KB payload reliably reproduces the 500.

The rate limiter caps abuse at ~100 requests / 15min, so this isn't a
sustained-DoS vector — but it IS a reliable cheap 500 trigger, and a
caught DoS is better than a swallowed one.

Verified with a one-liner before the fix:
$ node -e "const {canonicalJson}=require('./app/middleware/idempotency.js');
function n(d){let o=0;for(let i=0;i<d;i++)o={a:o};return o}
try{canonicalJson(n(5000))}catch(e){console.log(e.name)}"
RangeError

Fix:

  • canonicalJson takes a depth arg (default 0) and throws a tagged
    CanonicalJsonDepthError when depth exceeds MAX_CANONICAL_DEPTH=64.
    64 is well above any plausible API body's nesting; the limit is
    purely a DoS guard, not a correctness boundary.
  • The middleware wraps hashBody() in a typed catch. On
    CanonicalJsonDepthError it returns a clean 400
    { message, code: 'body_too_deep' } and short-circuits. The
    message is hardcoded (not echoed from error.message) so it
    passes the controller-error-shape policy that bans
    message: error.message in middleware/controllers.
  • MAX_CANONICAL_DEPTH and CanonicalJsonDepthError are added to the
    module exports so unit tests can pin them.

Tests added:

tests/unit/idempotency.test.js (6 new tests under "bounded recursion"):
- shallow nesting succeeds
- depth at the limit succeeds
- depth + 5 throws CanonicalJsonDepthError (NOT RangeError)
- arrays bounded by the same depth
- the previously-DoS-able depth (5000) is rejected cleanly
- hashBody surfaces the same error so the middleware can catch

tests/api/idempotency.test.js (2 new tests under "DoS defense"):
- depth-5000 raw-string body returns 400 (NOT 500)
- normal-depth body with Idempotency-Key still passes through

Note on test construction: the API test builds the 5000-deep payload
as a raw JSON STRING and uses .send(payload) with explicit Content-
Type, because JSON.stringify itself is recursive and would overflow
before supertest could transmit the body. express.json's parser is
iterative (V8 >= 9) so the body reaches req.body intact.

804 tests pass (was 800); lint clean.

Self-review caveats:

  • MAX_CANONICAL_DEPTH=64 is an arbitrary pick. If the API ever exposes an endpoint that legitimately accepts deeply-nested config blobs (it doesn't today), the limit needs to rise. Worth raising to 128 if anyone hits it.
  • The depth check fires AFTER express.json() has already parsed the body. The CPU cost of parsing 100KB of {"a":...} is paid regardless. Tightening further would require an express.json depth option (not in the stable API today) or a custom body-parser.
  • The same recursion pattern exists in any future canonical-hashing call site; if someone adds another use, they need to inherit the bound. Worth a code-comment header on the function once we have more callers.

…ware

`canonicalJson()` recurses once per nesting level. A POST that
includes an Idempotency-Key header reaches `hashBody(req.body) ->
canonicalJson(...)` synchronously inside the middleware. With Node's
default ~10000-frame call stack, depth ~5000 blows the stack with
RangeError — which Express's async error path surfaces as a 500.

Reach: any POST endpoint mounted under /v1 (every CREATE handler and
every /bulk handler). Body limit is 100KB (env-tunable), allowing up
to ~20000 nesting levels at 5 chars per level (`{"a":`), so a
single 25KB payload reliably reproduces the 500.

The rate limiter caps abuse at ~100 requests / 15min, so this isn't a
sustained-DoS vector — but it IS a reliable cheap 500 trigger, and a
caught DoS is better than a swallowed one.

Verified with a one-liner before the fix:
  $ node -e "const {canonicalJson}=require('./app/middleware/idempotency.js');
             function n(d){let o=0;for(let i=0;i<d;i++)o={a:o};return o}
             try{canonicalJson(n(5000))}catch(e){console.log(e.name)}"
  RangeError

Fix:
  - canonicalJson takes a `depth` arg (default 0) and throws a tagged
    CanonicalJsonDepthError when depth exceeds MAX_CANONICAL_DEPTH=64.
    64 is well above any plausible API body's nesting; the limit is
    purely a DoS guard, not a correctness boundary.
  - The middleware wraps hashBody() in a typed catch. On
    CanonicalJsonDepthError it returns a clean 400
    `{ message, code: 'body_too_deep' }` and short-circuits. The
    message is hardcoded (not echoed from `error.message`) so it
    passes the controller-error-shape policy that bans
    `message: error.message` in middleware/controllers.
  - MAX_CANONICAL_DEPTH and CanonicalJsonDepthError are added to the
    module exports so unit tests can pin them.

Tests added:

  tests/unit/idempotency.test.js (6 new tests under "bounded recursion"):
    - shallow nesting succeeds
    - depth at the limit succeeds
    - depth + 5 throws CanonicalJsonDepthError (NOT RangeError)
    - arrays bounded by the same depth
    - the previously-DoS-able depth (5000) is rejected cleanly
    - hashBody surfaces the same error so the middleware can catch

  tests/api/idempotency.test.js (2 new tests under "DoS defense"):
    - depth-5000 raw-string body returns 400 (NOT 500)
    - normal-depth body with Idempotency-Key still passes through

  Note on test construction: the API test builds the 5000-deep payload
  as a raw JSON STRING and uses .send(payload) with explicit Content-
  Type, because JSON.stringify itself is recursive and would overflow
  before supertest could transmit the body. express.json's parser is
  iterative (V8 >= 9) so the body reaches req.body intact.

804 tests pass (was 800); lint clean.
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