# Security Review — security-1 (jmap-states)

## Summary

I traced the two offset-based modseq reads, the `UserState/get` authorization
path, the hand-rolled argument parser, and the `EVENT_JMAP_STATES` gate. **The
offset math is memory-safe, the authorization is sound (it reuses the exact same
gate as `UserCounters/get`), and the parser is robust against malformed input.**
The one defensible issue is a **portability smell**: casting a pointer through
`off_t` (signed) instead of `uintptr_t`. It is not exploitable on any supported
build, but it is undefined-behavior-adjacent and should be corrected. No
high/critical security findings.

## What I Explored

- `imap/jmap_data_types.gperf` (the canonical table + every `modseq_offset`).
- `struct mboxname_counters` (`imap/mboxname.h:309-348`) and
  `jmap_req_t.counters` (`imap/jmap_api.h:173`) — confirmed both base pointers
  are this struct.
- `jmap_data_type_t` (`imap/jmap_api.h:110-116`) — `modseq_offset` is `off_t`.
- `add_userstate_cb` / `jmap_userstate_get` (`imap/jmap_core.c`) and
  `add_jmap_state_cb` / `mboxevent_extract_mailbox` (`imap/mboxevent.c`).
- The accountId authorization in `jmap_api()` (`imap/jmap_api.c:689-810`) and
  `lookup_capabilities` (`:497-574`); capability constant
  `JMAP_USERCOUNTERS_EXTENSION` granted via `jmap_core_capabilities`.
- `USER_COMPACT_EMAILIDS` (`imap/user.h:15`) — NULL-safe.
- `configure.ac` largefile handling (no `AC_SYS_LARGEFILE`).

## Findings

### Finding 1 — LOW: pointer arithmetic laundered through signed `off_t`

**Location**: `imap/jmap_core.c:308-309` (`add_userstate_cb`) and
`imap/mboxevent.c` `add_jmap_state_cb`:

```c
modseq_t modseq = *(modseq_t *)((off_t) &srock->req->counters + dtype->modseq_offset);
```

**Issue**: A pointer is cast to `off_t` (a *signed* integer), an `off_t` offset
is added, and the sum is cast back to `modseq_t *`. The correct integer type for
round-tripping a pointer is `uintptr_t`. `configure.ac` does **not** set
`_FILE_OFFSET_BITS=64` / `AC_SYS_LARGEFILE` (it only does `AC_CHECK_SIZEOF(off_t)`),
so `off_t` width is the platform default. On every modern 64-bit target this is
8 bytes and matches pointer width, so the code is correct in practice. On a
hypothetical ILP32/LFS-disabled build, `off_t` could be 32-bit while pointers are
64-bit, truncating the base address.

**Why it matters**: Strictly speaking, converting a pointer to an integer type
that cannot represent it is undefined behavior; a truncated base would then read
from an arbitrary address. This is a *latent* portability bug, not a reachable
attack — Cyrus's supported platforms all have 64-bit `off_t` and pointers. I'm
flagging it because this is a CVE-heavy codebase and the pattern is a foot-gun if
copied elsewhere.

**Attack vector**: None on supported builds. Requires a mis-configured 32-bit
`off_t` build to even theoretically misbehave, and even then the offsets are
tiny (< 300 bytes), so only the high bits of the base pointer would be lost.

**Suggestion**: Use `uintptr_t` for the round-trip, or — cleaner — index from a
`const char *` base:

```c
modseq_t modseq =
    *(const modseq_t *)((const char *)&srock->req->counters + dtype->modseq_offset);
```

`char *` arithmetic is the idiomatic, fully-defined way to apply a byte offset
and needs no integer cast at all. (Note the original `mboxevent.c` code used
`(size_t)&counters`, i.e. unsigned — the refactor switched it to signed `off_t`,
a small regression in correctness even if both work today.)

## Memory-safety verdict on the offset read (explicit)

**Sound.** Every `modseq_offset` actually dereferenced is
`offsetof(struct mboxname_counters, <a modseq_t field>)` — `mailmodseq`,
`submissionmodseq`, `sievemodseq`, `carddavmodseq`, `caldavmodseq`,
`notesmodseq`. Each lands on a `modseq_t` (8-byte) member well inside the struct,
correctly aligned, and reads exactly `sizeof(modseq_t)`. The base in both call
sites is a real `struct mboxname_counters`: `req->counters` (populated by
`mboxname_read_counters` at `jmap_api.c:798` before any method runs) and a stack
`counters` in `mboxevent_extract_mailbox` populated by `mboxname_read_counters`
at `mboxevent.c:1820`, with the offset math guarded behind `if (!r)`.

The `< 0` guard cannot be bypassed: `jmap_data_types_foreach` walks the
gperf array and only calls back for slots with a non-NULL `name`; the gperf
`%null-strings` / `initializer-suffix ,UINT32_MAX,false,MBTYPE_UNKNOWN,-1` means
every empty slot has `name == NULL` (skipped) AND `modseq_offset == -1`. Every
populated-but-stateless type (Core, Thread, Quota, MDN, …) is explicitly `-1` in
the `.gperf` and is filtered by `if (dtype->modseq_offset < 0) return;`. There is
no path that reaches the dereference with a negative or out-of-range offset. The
`UserState/get` parser applies the same `>= 0` filter again before inserting a
property name, so a client cannot select a stateless type to force a bad read.

## Authorization verdict on `UserState/get` (explicit)

**Sound — identical to `UserCounters/get`, no new exposure.** `UserState/get`
is registered with capability `JMAP_USERCOUNTERS_EXTENSION` and flag
`JMAP_NEED_CSTATE`, exactly like `UserCounters/get` (`jmap_core.c:42-52`).
`accountId` is authorized centrally in `jmap_api()`:
`lookup_capabilities(accountid, httpd_userid, …)` returns `json_null()` unless
the authenticated user can see the target account (own account, admin, or a
shared account where `mboxlist_usermboxtree` finds a visible mailbox); a null
result yields `accountNotFound` and the method never runs
(`jmap_api.c:705-721`). Crucially, the `usercounters` capability is only added by
`jmap_core_capabilities`, which is granted to the primary account and to *visible*
shared accounts. The method then reads `req->counters`, which `jmap_api()`
populated from **that authorized account's** inbox — the callback cannot reach
another user's counters.

Marginal info exposure is **nil**: every value reported is a per-type modseq that
is already returned by `UserCounters/get` (the raw numbers) and by each
`Foo/get`'s `state` field. `UserState/get` just reformats those same modseqs as
state strings. No finer-grained per-collection access control is bypassed,
because these are user-level counters, not per-mailbox data.

## Untrusted-input verdict on the parser (explicit)

**Robust.** I checked each path in `jmap_userstate_get`:

- `ids`: only accepted if `json_is_array`; non-array non-null → `invalidArguments`.
  Elements are read with `json_string_value` (returns NULL for non-strings) and
  the NULL/`!singleton` case appends to `not_found` — no deref of a NULL id.
- `properties`: `construct_hash_table(get.props, json_array_size(arg) + 1, 0)`.
  `json_array_size` is bounded by the request body, which the HTTP/JMAP layer
  already size-limits (`maxSizeRequest`), so this is not an unbounded allocation
  primitive beyond what any JMAP array argument already allows. Each element goes
  through `json_string_value` (NULL-safe: `name ? lookup : NULL`) then
  `jmap_data_types_lookup`; unknown names or stateless types are reported invalid,
  not inserted. `hash_insert(name, …)` borrows `name` from the live `req->args`
  JSON, which outlives `get.props` (freed in `jmap_get_fini` before the request
  args are torn down) — key lifetime is fine, matching the sibling methods.
- Unknown top-level keys → `invalidArguments`. `null` `ids`/`properties` are
  ignored (treated as absent), matching JMAP convention and the singleton-default
  behavior.

No OOB, NULL-deref, or unbounded-allocation vector found.

## EVENT_JMAP_STATES gate (explicit)

The new mask `(MESSAGE_EVENTS|FLAGS_EVENTS|MAILBOX_EVENTS|SUBS_EVENTS)` restricts
the *expected* param to events that carry a mailbox, since the param is only
filled inside `mboxevent_extract_mailbox`. This is a debug-assert-correctness
change, not a security boundary — the data in the param (per-user modseqs) is the
same data already in the `EVENT_COUNTERS` param and is only emitted to the
configured notification socket, gated by `event_extra_params`. No new info leak:
the gate *narrows* the set of events carrying the param (mailbox-less events like
Login no longer carry it), which is strictly a reduction in surface.

## What's Working Well

- The `modseq_offset < 0` guard is applied **twice** for `UserState/get` (in the
  parser when validating a requested property, and again in the callback),
  defense-in-depth against ever dereferencing a stateless slot.
- Reusing the central `jmap_api()` accountId gate means `UserState/get` inherits
  the full shared-account authorization model for free — no bespoke authz code to
  get wrong.
- `USER_COMPACT_EMAILIDS` short-circuits on NULL `cstate`, so the mboxevent path
  is safe even if `mailbox_get_cstate` returns NULL (no `J` prefix, no crash).

## Clarifying Questions

- Is there any supported/CI build target with a non-8-byte `off_t`? If strictly
  no (LFS always on), Finding 1 stays LOW/cosmetic; if any 32-bit target survives,
  it rises to MEDIUM.

## Questions for Other Reviewers

- (correctness) The original `mboxevent.c` used `(size_t)` for the same cast; the
  refactor changed it to `(off_t)`. Worth a one-line fix to `char *` arithmetic
  to match house style and drop the integer cast entirely?
- (wire) The dropped keys (`Quota`, `Racl`, `Contact`, `ContactGroup`) and the
  Mailbox/Calendar modseq semantics change are out of my lane but affect any
  external consumer of `vnd.fastmail.jmapStates` — confirming "no internal
  consumer depends on the old shape" is a non-security correctness check I'm
  deferring to principal/testing.