Security fixes for 2026.10.28 #222

Merged
jcoffey-dev merged 21 commits from sec/batch-2026-10-27 into main 2026-10-28 07:17:09 +00:00
Owner

Security fixes for the next 2026.x release. Each one has a test that failed on main before the fix.

  • IMAP: bound CONDSTORE missing-message work by the mailbox, not the request
  • JMAP: place each create once when ordering creation references
  • Bitmap::remove clears the bit instead of toggling it
  • IMAP: CLOSE removes messages only with the rights EXPUNGE needs
  • iTIP: accept organizer-only methods only from the organizer
  • Sharing: a sharee passes on only the rights it holds
  • OIDC: never publish or reuse the fallback signing secret
  • Autoconfig: escape the caller's address in XML responses
  • OAuth: keep device codes apart from authorization codes
  • Calendar: refuse recurrence rules too large to expand
  • Email/set: a sharee changes only messages it can read
  • HTTP: believe forwarding headers only from trusted proxies
  • SMTP: never write an address with a line break into a command
  • Spam filter: stop looking for the end of an IPv6 literal after 47 characters
  • Spam filter: compare the first 50 recipients for similar addresses
  • Spam filter: search only the start of the Subject for recipients
  • JMAP: parse escaped slashes in a key as nested pointers only 8 deep
  • Files: FileNode/copy replaces only what the caller may delete, and only once the copy is made
  • Files: a sharee creates and moves file nodes only where it may
  • DAV: moving a whole calendar or address book needs the rights deleting it does
  • Accounts: change or delete an account only holding every permission it has

Advisories follow once the release is out and production runs it.

Two things change for operators:

  • Forwarding headers (X-Forwarded-For, Forwarded) are read only from trusted proxies: loopback and private ranges by default, INBUXA_HTTP_TRUSTED_PROXIES to change it. A proxy on a public address has to be listed.
  • Device sign-ins (OAuth device grant) in progress during the upgrade have to start again.
Security fixes for the next 2026.x release. Each one has a test that failed on main before the fix. - IMAP: bound CONDSTORE missing-message work by the mailbox, not the request - JMAP: place each create once when ordering creation references - Bitmap::remove clears the bit instead of toggling it - IMAP: CLOSE removes messages only with the rights EXPUNGE needs - iTIP: accept organizer-only methods only from the organizer - Sharing: a sharee passes on only the rights it holds - OIDC: never publish or reuse the fallback signing secret - Autoconfig: escape the caller's address in XML responses - OAuth: keep device codes apart from authorization codes - Calendar: refuse recurrence rules too large to expand - Email/set: a sharee changes only messages it can read - HTTP: believe forwarding headers only from trusted proxies - SMTP: never write an address with a line break into a command - Spam filter: stop looking for the end of an IPv6 literal after 47 characters - Spam filter: compare the first 50 recipients for similar addresses - Spam filter: search only the start of the Subject for recipients - JMAP: parse escaped slashes in a key as nested pointers only 8 deep - Files: FileNode/copy replaces only what the caller may delete, and only once the copy is made - Files: a sharee creates and moves file nodes only where it may - DAV: moving a whole calendar or address book needs the rights deleting it does - Accounts: change or delete an account only holding every permission it has Advisories follow once the release is out and production runs it. Two things change for operators: - Forwarding headers (X-Forwarded-For, Forwarded) are read only from trusted proxies: loopback and private ranges by default, `INBUXA_HTTP_TRUSTED_PROXIES` to change it. A proxy on a public address has to be listed. - Device sign-ins (OAuth device grant) in progress during the upgrade have to start again.
jcoffey-dev added 21 commits 2026-10-28 07:04:22 +00:00
STORE ... (UNCHANGEDSINCE n) worked out which requested messages no longer
exist by expanding every number in the client's sequence set into a hash
set, so a single "UID STORE 1:4294967295 (UNCHANGEDSINCE 1)" from any
logged-in user asked the server for billions of entries while it held the
mailbox state lock.

The missing set is now computed as ranges. Numbers up to the mailbox's
highest UID (or message count) are checked one by one; everything above
it is missing by definition and is emitted as a single range without
being walked. MODIFIED carries ranges and is serialized as a sequence set,
so its output is unchanged. Sequence::expand had no other caller and is
removed.

Tests: a range test that failed on main (expand(10) of 1:5000000 returned
5,000,000 entries) now passes against the new code, plus a reference
comparison of the range result against per-number expansion.
The topological sort of creation references (#id references between
creates in one /set or Blob/upload) never remembered which creates it had
already placed. When creates share references, e.g. each one referring to
the next two, it walked the same nodes again and again, about fib(n)
times: a request with 60-80 such creates kept a worker busy indefinitely.
The 1000-deep stack check only catches cycles, and this graph has none.

A create that has already been placed is now skipped. Its references were
placed before it, so the order is unchanged.

Test: sorting a 60-create chain did not finish within 2 seconds on main;
it now returns at once, with every create after the ones it references.
remove() XORed the bit, so removing an item that wasn't set added it.
IMAP SETACL with "-<rights>" removes rights one at a time through it, so
"SETACL mailbox user -x" against a user without "x" granted them "x".
The JMAP callers remove from a full set, where both forms agree.

Test: removing an unset item left it set on main; it now stays unset.
The client address came from the left-most X-Forwarded-For or Forwarded
entry, which the client writes itself, so a banned or rate-limited client
behind a proxy could pick any address. Headers are now read only when the
connection comes from a trusted proxy, and the client is the right-most
hop no trusted proxy accounts for.

Trusted by default: loopback and the private ranges. INBUXA_HTTP_TRUSTED_PROXIES
replaces that list. A direct client's headers are ignored, and scanner bans
apply to it even with forwarding on.
TLS-RPT report addresses come from DNS, and quoted-printable in a
rua=mailto: URI can put CR/LF into one. The address went into RCPT TO
as it was, so the line break started a command of the record owner's
choosing. Addresses with control characters, spaces or angle brackets
are now refused when they're queued, and any already in the queue fail
instead of being sent.
The tokenizer that finds URLs and email addresses in a message looked
for the closing bracket of an IPv6 host with no limit, buffering every
token after an unclosed "[", and then took those tokens off the front
of a Vec one at a time. A long "a@[1:1:1:..." run in inbound mail took
time quadratic in its length: 7.4 s for 200,000 characters, hours for
a full-size message. No IPv6 literal is longer than 47 characters with
its brackets, so the search stops there, and the buffer is a VecDeque.
The similar-recipients check ran an edit distance over every pair of
To/Cc addresses. A To header with thousands of addresses took time
quadratic in its length: 15 s for 5,000, hours for a large header in
one inbound message. The check now looks at the first 50, which is
plenty to tell a list of near-identical addresses from a real one.
Each recipient's address and local part were searched for in the whole
Subject, and nothing limits how long a Subject or a To header can be.
Both long took time proportional to the two lengths together: 7.4 s for
5,000 recipients and a 2 MB Subject, about a minute for a 1 MB To and a
1 MB Subject, per inbound message. Only the first 1,000 characters of
the Subject are searched now.
A property key whose first part holds an escaped "/" (~1) is parsed
again as a pointer, and each level can escape the one inside it. A key
built that way and nested a few thousand deep, well inside the default
request size, recursed once per level: on a 2 MiB stack, like a tokio
worker's, it overflowed and aborted the server. Past 8 levels the text
is now kept as a plain key, which is also what RFC 6901 says "~1" means.
With onExists=replace, a copy that collided with an existing node put
that node on the list to delete before checking whether the caller
could add to the folder, and nothing took it off again when a later
check refused the copy. The node and everything under it were then
deleted with no Delete check. A sharee who knew a folder id and a name
in it could delete any of the owner's files. The folder is now checked
before names are looked at, replacing needs Delete on the node and all
under it, as FileNode/set's destroy does, and the replaced node is
deleted only when the copy is written.
FileNode/set checked a sharee's rights on the node itself but never on
the folder: anyone a folder was shared with could create files in it,
read-only or not, and anyone who could rename a node could move it into
any of the owner's folders or to the top of the account. FileNode/copy
already checked the destination. Creating now needs mayAddChildren on
the folder, and moving needs mayDelete on the folder it leaves and
mayAddChildren on the one it goes to, never the top. A writing delegate
(AL-7) is unaffected.
A MOVE of a whole collection out of another account checked only that
the caller could remove items from it, then took the collection from its
owner. A JMAP calendar share with mayWriteAll gives that without
mayDelete, so a sharee could move the owner's calendar into their own
account. MOVE now needs Delete as well as RemoveItems on the collection,
as DELETE does. Address books get the same check; there RemoveItems only
ever comes with Delete.
Accounts: change or delete an account only holding every permission it has
ci / fork-checks (pull_request) Skipped
ci / build (pull_request) Skipped
github/ci (branch) GitHub Actions
ci / github (pull_request) Successful in 8m6s
32559d5bd0
Permissions were compared only when they were what a change touched, and
not at all on delete, so an administrator with account-update rights
could reset the password of an administrator holding more, or delete
the account. Changing or deleting an account now needs every permission
it has.

SCIM keeps managing every account on its domains, administrators
included (SCIM-13). Requests through the SCIM endpoint skip the new
check, and the permission change they make is checked for what it adds
rather than for everything the account holds, so SCIM can turn off an
administrator's sign-in and still can't grant anything its key lacks.
jcoffey-dev merged commit 626c298804 into main 2026-10-28 07:17:09 +00:00
jcoffey-dev deleted branch sec/batch-2026-10-27 2026-10-28 07:17:10 +00:00
Sign in to join this conversation.