commit 8b6932843b87183548532dae81ebad384663950a from: David Williams date: Thu Sep 10 03:55:27 2026 UTC libtls fake-key invariant checks, a MODIFIED fix, a credentials-file permission tightening, and a dedup pass listener terminates TLS without holding the private key: it installs libtls's placeholder key via tls_config_use_fake_private_key() and an RSA_METHOD/EC_KEY_METHOD override that forwards every private-key operation to keymgr. Three properties of libtls internals make that work, and until now all three held by inspection only. Nothing in the daemon checked them, and nothing underneath does either, libtls skips SSL_CTX_check_private_key() whenever the fake key is in use. keymgr_assert_fake_key() now checks all three on every private-key operation, before anything is composed for keymgr: A the key object carries no private component. Under the fake key libtls builds it from X509_get_pubkey(), and a public-key decode never writes d or priv_key, so this is a NULL-pointer test rather than a value test. If it fires, libtls has begun loading real key material into the process that parses hostile TLS records, and the separation this mechanism exists for is not in effect. B the pubkey-hash tag on ex_data slot 0 is present. smtpd's ca.c treats an unset tag as an unrelated key belonging to some other part of the process and falls through to the real method. That is right for smtpd's dispatcher and wrong here: listener configures exactly one keypair, once, at spawn, does no client-certificate verification, and cannot reach these callbacks with an ephemeral ECDHE key, since only sign_sig is overridden and ECDH agreement uses a different method slot. With no legitimate unrelated-key case, an absent tag is the regression, so the three fall-through branches are gone. C the tag is a NUL-terminated string within KEYMGR_HASH_MAX bytes, checked before anything reads it as one. Not merely a consistency check: strlcpy(3) walks its source to the NUL to compute its return value, unbounded once the destination is full, so keymgr_forward_rsa()'s own length guard could only fire after an over-read had already happened. libtls stores a struct tls_config pointer in the adjacent ex_data slot 1, so a slot renumbering was all it would have taken to point that walk at a C struct. Each failure is fatalx(). listener is a per-connection worker, so a regression costs that connection rather than the daemon, and costs it before any key operation is performed or forwarded. New testing/keymgr_fakekey_test.c asserts the same three properties against the real installed libtls, so a libtls change is caught by running a test rather than by a handshake misbehaving in production. It generates its own RSA and EC self-signed certificates in process, installs the same engine override, and drives four real handshakes over a non-blocking socketpair: RSA and EC, TLS 1.2 and 1.3. Two faults are injectable, so each check is seen to fire rather than assumed to. "-f realkey" configures a genuine private key using public API alone and requires A and B to fire; "-f untermtag" places an unterminated tag against a guard page, where C rejects it safely and a forked child running strlcpy(3) on the same tag dies with SIGSEGV. RSA_PRIVDEC is not exercised, reaching it needs static-RSA key exchange, which TLS 1.3 does not have and the "secure" cipher selection excludes, and the program says so in its own output rather than implying the coverage. No new dependency surface. The checks use RSA_get0_d(), EC_KEY_get0_private_key() and the ex_data getters that listener.c already includes and for, all public and exported, and add no libtls-internal declaration beyond the tls_config_use_fake_private_key() extern already in the file. README now states which OpenBSD this builds on: -current, not 7.9. Building on the most recent stable release is a goal for 1.0. The credentials-file permission check auth.c's cred_lookup() applies now also rejects a file that is group-executable or not owned by root or the process's own (post-chroot, post-setresuid) uid. It already refused a world-accessible or group-writable file; a credentials file left mode 0650, or owned by neither root nor _imapauth, was accepted without comment. cred_file_secure() replaces the inline check and mirrors parse.y's check_file_secrecy(), the policy imapd.conf itself is already held to. New testing/cred_file_perm_test.c (generated by testing/gen_cred_perm_test.py, same splice-and-verify shape as mailbox_name_test.c) drives cred_file_secure() over 14 synthetic (mode, owner) cases, no real files or root needed, and fails on exactly the three the old check missed before this fix, passing all fourteen after it. An incomplete RFC 7162 MODIFIED set is no longer sent. SS3.1.3 requires the set to list every message that failed the UNCHANGEDSINCE test, and SS3.1.3's own client guidance is that a client re-checks and retries what it finds there, so a message missing from the set is one the client believes was stored and will never revisit. Three paths could produce that. A failed realloc(3) in session_handle_store_modified() dropped entries silently, and if it failed on the first entry the count stayed 0, the MODIFIED block was skipped entirely, and the client was told "STORE completed". A failed malloc(3) of the response buffers dropped the response code, which per SS3.1.3 says the same thing. A formatter truncation logged a warning and sent the short list anyway. All three now set one sticky flag and answer NO [UNAVAILABLE], RFC 5530 SS3, marking it transient so a client retries rather than treating the STORE as rejected. That matches what every other variable-length list here already does on a failed grow: search_alloc_failed, copy_alloc_failed and qresync_alloc_failed all refuse rather than send a short answer, and store_ipc.c says why for VANISHED, "a dropped range would leave the client holding a phantom UID it can never be told about". store_do() also gains the defensive pre-command reset that search_dispatch() has and it lacked. New mboxname.c/mboxname.h hold the mailbox-name rules once. The listener's copy of store.c's syntax check had already drifted once, missing the rejection of "." / ".." and of the on-disk index filenames, and utf8.c exists because the UTF-8 half of the same rule drifted before that. The four reserved filenames move into that header too, since spelling them as literals on one side and constants on the other is how the drift happened; store_internal.h includes it, so the store side is unchanged. Both validators survive as separate entry points, the store does not trust the listener, and re-checking on the far side of the imsg boundary is the point, but what they check is now one function. mailbox_name_is_inbox() joins it, replacing a byte-identical one-liner on each side. The rest is duplication with no behaviour attached. hdr_next_field() (mime.c) walks one RFC 5322 header field, replacing the line-end, CRLF-vs-LF, blank-line and obs-fold logic written out twice; its tri-state return also makes explicit the difference between "header ended cleanly" and "ran out", which read_message_header_fields() previously encoded as two different breaks setting two different values twenty lines apart. seqset_position() (index.c) answers "does this command apply to this message?" for FETCH, STORE and COPY. append_range_token() (store_cmd.c) carries the token/comma/budget tail format_seq_list() and format_range_list() both spelled out. send_mbox_request() now composes CREATE/DELETE/RENAME/LIST/STATUS too, taking every listener-to-store request through one function, and grew a no-trailing-array fast path so a fixed-size request no longer mallocs a copy of itself. session_writef() (listener.c) carries the CRLF- preservation fixup session_reply() and session_untagged() shared. mbox_root_enter() (mbox_manage.c) opens CREATE/DELETE/RENAME/LIST. read_file_capped() (parent.c) carries the fgetc(3) probe that tells "a file exactly the buffer's size" from "there is more". setup_peer_send() absorbs its search-oracle twin. keymgr_try_reload() and index_lines_grow() are byte-identical extractions. commit - 2a6467d3157cdcdc9f6a69080095e37a528abd2c commit + 8b6932843b87183548532dae81ebad384663950a blob - cc65af9f3e5ce618d06b770df9b41cf25029c780 blob + a048b0269361225d8c0318f6c477b70062f00f9b --- README.md +++ README.md @@ -6,7 +6,7 @@ A from-scratch IMAP4rev2 ([RFC 9051](https://www.rfc-e ## What it is -- **Privilege-separated**, `smtpd`-style, across six processes: a root *parent* reads configuration and binds the listening sockets; unprivileged *listener* and *auth* children handle the network and credential checks; *keymgr* holds the TLS private key and performs every private-key operation on request, so the process terminating TLS never has the key in its address space; a per-connection *search-oracle* parses the `SEARCH` grammar — the largest attacker-reachable parser in the daemon — in a process with no descriptors and no filesystem; and a *store* child is forked per authenticated session, chroots into the mail spool, and drops privileges to that session's own user before ever touching a message. `pledge(2)`, `unveil(2)`, and `chroot(2)` enforce these boundaries, not just convention: *parent* is the only process that can pass a file descriptor at all, and *search-oracle* runs on bare `stdio`. +- **Privilege-separated**, `smtpd`-style, across six processes: a root *parent* reads configuration and binds the listening sockets; unprivileged *listener* and *auth* children handle the network and credential checks; *keymgr* holds the TLS private key and performs every private-key operation on request, so the process terminating TLS never has the key in its address space; a per-connection *search-oracle* parses the `SEARCH` grammar, the largest attacker-reachable parser in the daemon — in a process with no descriptors and no filesystem; and a *store* child is forked per authenticated session, chroots into the mail spool, and drops privileges to that session's own user before ever touching a message. `pledge(2)`, `unveil(2)`, and `chroot(2)` enforce these boundaries, not just convention: *parent* is the only process that can pass a file descriptor at all, and *search-oracle* runs on bare `stdio`. - **Storage**: stock maildir format (`tmp/`/`new/`/`cur/`, atomic delivery via `rename(2)`), readable with `ls` and `grep`, and natively understood by `smtpd(8)`'s own `maildir` delivery action. IMAP's extra bookkeeping (UIDs, UIDVALIDITY, per-message mod-sequences, keywords) lives in a small, `flock(2)`-guarded, line-oriented index file per mailbox, plain colon-delimited text, not a database. - **Transport**: STARTTLS on port 143 and implicit TLS on port 993 ([RFC 8314](https://www.rfc-editor.org/rfc/rfc8314)), via `libtls`. `AUTH=PLAIN` only, refused before TLS is established. @@ -16,14 +16,16 @@ A from-scratch IMAP4rev2 ([RFC 9051](https://www.rfc-e `SUBSCRIBE`, `UNSUBSCRIBE`, and ACL/shared-mailbox support are deliberately left out. -**`IDLE` is a poll, not a kernel-driven push.** An `IDLE`ing session rechecks its selected mailbox every `idle poll` seconds (default 5, see `imapd.conf`), so new mail — whether delivered by an external MTA or by another IMAP session — is reported within one interval rather than instantly. Most polls are two `stat(2)` calls and no lock: the store child only re-reads the index and streams UIDs when the mailbox directory or `new/` has actually been touched. Setting `idle poll 0` disables polling entirely, which restores the earlier behaviour where an `IDLE`ing session saw nothing until it sent `DONE`. +**`IDLE` is a poll, not a kernel-driven push.** An `IDLE`ing session rechecks its selected mailbox every `idle poll` seconds (default 5, see `imapd.conf`), so new mail, whether delivered by an external MTA or by another IMAP session, is reported within one interval rather than instantly. Most polls are two `stat(2)` calls and no lock: the store child only re-reads the index and streams UIDs when the mailbox directory or `new/` has actually been touched. Setting `idle poll 0` disables polling entirely, which restores the earlier behaviour where an `IDLE`ing session saw nothing until it sent `DONE`. ## Requirements -OpenBSD only. This depends on ``, `pledge(2)`, `unveil(2)`, and libutil's `imsgbuf_*` API, none of which exist outside OpenBSD. Developed and tested against OpenBSD 8.0. Links against libevent, libtls/libssl/libcrypto, and libutil, all base-system libraries (see `src/Makefile`). +OpenBSD only. This depends on ``, `pledge(2)`, `unveil(2)`, and libutil's `imsgbuf_*` API, none of which exist outside OpenBSD. Links against libevent, libtls/libssl/libcrypto, and libutil, all base-system libraries (see `src/Makefile`). -`keymgr`, the process that isolates the TLS private key from `listener`, additionally depends on `tls_config_use_fake_private_key()` and undocumented ex_data-tagging behavior inside `tls_keypair_load()` — both unexported libtls/LibreSSL internals with no compatibility promise, and neither declared in libtls's public, installed `tls.h`. +**imapd requires OpenBSD -current, and will not build on 7.9.**. Building on the most recent stable release is a goal for 1.0. +`keymgr`, the process that isolates the TLS private key from `listener`, additionally depends on `tls_config_use_fake_private_key()` and undocumented ex_data-tagging behavior inside `tls_keypair_load()`, both unexported libtls/LibreSSL internals with no compatibility promise, and neither declared in libtls's public, installed `tls.h`. + ## Building and installing ``` @@ -53,7 +55,7 @@ Every directive is documented inline in the sample fil ## Creating an account -imapd's users aren't real system accounts — `imapduser(8)` manages a bespoke credentials file (`username:passwordhash:uid:gid:maildir`, bcrypt via `crypt_checkpass(3)`) and the matching maildir ownership together, since no combination of `useradd(8)`/`userdel(8)` can safely keep both in sync: +imapd's users aren't real system accounts, `imapduser(8)` manages a bespoke credentials file (`username:passwordhash:uid:gid:maildir`, bcrypt via `crypt_checkpass(3)`) and the matching maildir ownership together, since no combination of `useradd(8)`/`userdel(8)` can safely keep both in sync: ``` doas imapduser -a someuser @@ -74,9 +76,9 @@ Beyond the deliberate protocol-scope decisions covered - If the listener or auth process exits unexpectedly after startup, it is not automatically restarted. Recovery is `rcctl restart imapd`. See `imapd(8)`. -`SIGHUP` reloads `spool`, `attachment max`, and the TLS certificate/key without dropping connected sessions, `listen on` and `credentials` changes still require a restart. See `imapd(8)`. +`SIGHUP` reloads `spool`, `attachment max`, `idle poll`, `startups`, and the TLS certificate/key without dropping connected sessions, `listen on` and `credentials` changes still require a restart. See `imapd(8)`. -IPv6 is supported (`listen on ::` or `listen on *` for dual-stack) but not the default — see `imapd(8)`'s `listen on` directive. +IPv6 is supported (`listen on ::` or `listen on *` for dual-stack) but not the default, see `imapd(8)`'s `listen on` directive. ## Getting the source blob - 83844217b4c7c6b04ec517451e215b394640e175 blob + 269c1b36d07fd286e1773f1a58763b2b5e5182c3 --- contrib/imapduser.8 +++ contrib/imapduser.8 @@ -2,7 +2,7 @@ .\" .\" Written for the OpenIMAPD project. Public domain / no rights reserved. .\" -.Dd $Mdocdate: September 6 2026 $ +.Dd $Mdocdate: September 9 2026 $ .Dt IMAPDUSER 8 .Os .Sh NAME blob - 1cc25d6cde58222be9921a9f95c5ee229e4510a8 blob + d2d44336dd52fce32311a046d5ceb0a96265e1fc --- src/Makefile +++ src/Makefile @@ -6,7 +6,7 @@ PROG= imapd -SRCS= main.c parent.c log.c imsgev.c parse.y utf8.c \ +SRCS= main.c parent.c log.c imsgev.c parse.y utf8.c mboxname.c \ listener.c auth_cmd.c mailbox_cmd.c append_cmd.c fetch_cmd.c \ search_cmd.c store_cmd.c store_ipc.c \ auth.c \ blob - 0c2a64146c0476e8b02ed9b60b02b6b2ca467835 blob + 79a3db86c5d124e3e3840c6d57cf4566a6586c13 --- src/append_cmd.c +++ src/append_cmd.c @@ -14,10 +14,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -/* - * append_cmd.c, APPEND: literal-driven message upload, and its - * asynchronous IMSG_MBOX_APPENDED completion handling. - */ +/* append_cmd.c: APPEND literal-driven message upload and its async IMSG_MBOX_APPENDED completion handling. */ #include #include @@ -41,6 +38,7 @@ #include "imapd.h" #include "log.h" #include "listener.h" +#include "mboxname.h" /* RFC 9051 SS9 date-time via sscanf(3); calendar validity (e.g. Feb 31) unchecked, timegm(3) normalizes it. */ int @@ -85,17 +83,7 @@ parse_date_time(const char *s, int64_t *out) if (zsign == '-') zoff = -zoff; - /* - * The year is already required to be 1970 or later, but nothing bounds - * the zone -- sscanf's %c%2d%2d accepts up to +9999, and RFC 9051 SS9's - * "zone = ("+" / "-") 4DIGIT" does not constrain the value either. So - * "01-Jan-1970 00:00:00 +9959" resolves to -359940, and - * handle_mbox_append() formats the delivery timestamp straight into the - * maildir basename -- producing a file called "-359940.pid_n.host" in a - * maildir the README advertises as readable with ls(1) and grep(1), - * where any shell glob over cur/ hands that name to rm(1) as an - * option rather than a file. Out of the intended range, so refuse it. - */ + /* Reject out-of-range zone offsets -- sscanf/RFC 9051 don't bound them, and an extreme offset can produce a maildir basename unsafe for shell globs. */ if ((int64_t)t - zoff < 0) return (-1); @@ -221,15 +209,7 @@ parse_append_args(char *args, struct append_parsed *ou memcpy(digitsbuf, start, digits_len); digitsbuf[digits_len] = '\0'; - /* - * RFC 9051 SS9: literal = "{" number64 ["+"] "}", and - * number64 = 1*DIGIT -- no sign. strtoull(3) accepts one, so - * "{-1}" would arrive as ULLONG_MAX with errno untouched and - * be answered NO [LIMIT] "message too large" by cmd_append() - * rather than the BAD a syntax error deserves. Same - * first-character-is-a-digit guard listener.c's own literal - * pre-scan already applies to the same announcement. - */ + /* RFC 9051 SS9's number64 is unsigned, but strtoull(3) accepts a sign, so "{-1}" would arrive as ULLONG_MAX and get a misleading NO [LIMIT] instead of BAD -- same digit guard listener.c's literal pre-scan already applies. */ if (digitsbuf[0] < '0' || digitsbuf[0] > '9') { *errmsg = "malformed literal octet count"; return (-1); @@ -326,17 +306,7 @@ cmd_append(struct session *s, const char *tag, char *a s->literal_remaining = parsed.litlen; s->literal_pending = 1; - /* - * RFC 9051 SS4.3: only synchronizing literals need a "+" continuation; - * harmless but misleading to send for non-sync. - * - * sizeof() - 1, not a hand-counted constant: this line used to pass 27 - * for a 26-byte string, which wrote the string literal's own NUL - * terminator onto the wire ahead of the tagged APPEND response. NUL is - * not a legal octet in the IMAP stream; clients mostly tolerate it, - * which is why it went unnoticed. Same idiom as listener.c's own - * session_write(s, bad, sizeof(bad) - 1) call sites. - */ + /* RFC 9051 SS4.3: "+" continuation is only for synchronizing literals. Uses sizeof()-1, not a hand count -- a wrong hand-counted length once wrote a stray NUL onto the wire, same idiom as listener.c's session_write() calls. */ if (!parsed.litnonsync) { static const char cont[] = "+ Ready for literal data\r\n"; @@ -402,12 +372,7 @@ session_finish_append(struct session *s) s->state = SESSION_APPENDING; - /* - * Same fail-soft shape send_mbox_request() now uses: a compose failure - * would otherwise leave s->state at SESSION_APPENDING with nothing in - * flight to move it back, and session_is_busy() then blocks every - * further command while the client waits for a tagged reply. - */ + /* Same fail-soft shape as send_mbox_request(): without it, a compose failure leaves s->state stuck at SESSION_APPENDING and session_is_busy() blocks every further command. */ if (imsg_compose(&s->store_iev->ibuf, IMSG_MBOX_APPEND, 0, 0, -1, combined, combined_len) == -1) { log_warn("session %u: imsg_compose IMSG_MBOX_APPEND", s->id); @@ -442,8 +407,8 @@ session_handle_mbox_appended(struct session *s, } /* INBOX compared case-insensitively (SS5.1); any other mailbox name case-sensitively. */ - if (listener_mailbox_name_is_inbox(s->append_mailbox) && - listener_mailbox_name_is_inbox(s->selected_mailbox)) + if (mailbox_name_is_inbox(s->append_mailbox) && + mailbox_name_is_inbox(s->selected_mailbox)) appended_to_selected = 1; else appended_to_selected = blob - 6e913abfb622ccd27527ba535ef21fdd7468da89 blob + 27fa92b6d7ede434293de8142b8d3afcc477c6c6 --- src/auth.c +++ src/auth.c @@ -52,64 +52,8 @@ static void auth_verify(struct imsg_auth_request *, static void auth_dispatch(int, short, void *); static void auth_dispatch_parent(int, short, void *); -/* - * SS6.2's retrofit: does a second IMSG_AUTH_REQUEST for a session_id - * auth already resolved successfully get treated as a fresh login - * attempt, or refused? A correctly-behaving listener never sends one - * -- AUTHENTICATE is only offered from SESSION_NOT_AUTH (listener.c's - * command table), and a session leaves that state for good the moment - * auth grants it -- so this is pure defense against a compromised or - * buggy listener replaying (or fabricating a duplicate of) a grant it - * already received, mirroring keymgr.c's SS6.1 keymgr_got_init gate: - * explicit and checkable, not merely "the code that could send this - * doesn't exist yet." - * - * A fixed-size ring, not a TAILQ: auth is never told when a session - * ends (listener/store don't notify it -- only parent's - * store_children TAILQ has session lifecycle visibility, over a - * different channel), so there is no event to free an entry on. The - * alternative is unbounded growth; a ring just ages the oldest entry - * out instead. That's safe, not merely convenient: session_id is - * parent.c's monotonically increasing next_session_id (spawn_ - * connection()), minted once per accepted connection and never - * reused for the life of the daemon -- SS7 moved this counter, and - * the accept() loop that used to feed it, from listener.c to - * parent.c (see parent.c's own header comment). SS7 also makes - * reuse-within-one-process a stronger guarantee than that alone: - * each auth-worker is now spawned fresh per connection and paired - * with exactly one listener-worker for its own whole (short) life - * (parent.c's spawn_connection() again), so a single auth-worker - * can structurally never observe more than one distinct session_id - * in the first place. Kept as a ring rather than a single slot - * anyway, so this file doesn't need to change again if that ever - * stops being true. Sized well above parent.c's - * STORE_CHILD_MAX (64 concurrent sessions) so eviction shouldn't - * happen at any realistic session volume. - */ -/* - * Cap on failed authentication attempts this auth-worker will spend a - * bcrypt on, i.e. sshd_config(5)'s MaxAuthTries, whose own default this - * matches. - * - * A plain static counter IS a per-connection counter here: SS7 spawns one - * auth-worker per connection, paired with exactly one listener-worker for - * its whole (short) life (parent.c's spawn_connection()) -- the same fact - * the auth_resolved comment above relies on. - * - * Why it is needed: listener.c returns a failed session to - * SESSION_NOT_AUTH and AUTHENTICATE is ST_NOTAUTH, so a client may retry - * without limit. Each retry costs this process one bcrypt -- ~100ms of - * CPU, deliberately -- and costs the client one small packet on a - * connection it already holds. That asymmetry is backwards: bcrypt's work - * factor is meant to be paid by whoever is guessing, and without a cap an - * attacker converts cheap packets into unbounded server CPU across as - * many connections as MaxStartups allows. - * - * Past the cap, requests are refused WITHOUT calling crypt_checkpass(3), - * which is what removes the cost. The connection is not dropped -- doing - * that needs a listener-side change and a way to say so on the wire; the - * CPU asymmetry, which is the actual damage, is closed either way. - */ +/* SS6.2: refuses a second IMSG_AUTH_REQUEST for an already-resolved session_id, defending against a compromised/buggy listener replaying a grant; tracked in a fixed-size ring (not a TAILQ) since auth is never notified of session end, and session_id is unique-per-daemon so an evicted entry is harmless. */ +/* Caps bcrypt-costing auth attempts per connection (sshd's MaxAuthTries default) so a client retrying without limit can't convert cheap packets into unbounded server CPU; past the cap, requests are refused without calling crypt_checkpass(3), closing the cost asymmetry (though not dropping the connection). */ #define AUTH_MAX_TRIES 6 static unsigned int auth_failures; @@ -172,14 +116,7 @@ auth_main(void) imsg_get_type(&imsg)); if (imsg_get_data(&imsg, &init, sizeof(init)) == -1) fatalx("auth: bad IMSG_AUTH_INIT payload"); - /* - * imsg_get_data() guarantees size, not NUL termination, force it -- - * same rule as req.username/req.password below, and parent.c's own - * inbound maildir. strlcpy(3) reads its source to the NUL to compute - * its return value, so an unterminated field here would be an - * unbounded read past this stack struct, on the message that decides - * what directory this process chroot(2)s into. - */ + /* imsg_get_data() guarantees size, not NUL termination -- force it, since strlcpy(3) would otherwise read unboundedly past this stack struct while computing the chroot(2) target. */ init.cred_file[sizeof(init.cred_file) - 1] = '\0'; imsg_free(&imsg); @@ -192,13 +129,7 @@ auth_main(void) if (strlcpy(chrootdir, init.cred_file, sizeof(chrootdir)) >= sizeof(chrootdir)) fatalx("cred_file too long: %s", init.cred_file); - /* - * Actually test what the message below claims. strrchr() finding a - * '/' only means the path HAS a directory component: "etc/creds" - * would pass and then chroot(2) relative to whatever working - * directory rc.d(8) left this daemon in. parse.y only length-checks - * the "credentials" directive, so nothing upstream enforces this. - */ + /* Actually verifies what the fatalx() below claims: strrchr() finding '/' only proves a directory component exists, not an absolute path (e.g. "etc/creds" would otherwise chroot(2) relative to cwd), and parse.y doesn't enforce this upstream. */ if (init.cred_file[0] != '/') fatalx("cred_file must be an absolute path: %s", init.cred_file); @@ -223,17 +154,7 @@ auth_main(void) setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid) == -1) fatal("cannot drop privileges to _imapauth"); - /* - * SS7: auth-worker's one and only peer, wired by parent.c's - * spawn_connection() via setup_peer_send() the moment both it - * and the listener-worker it's paired with exist. No - * IMSG_SETUP_DONE ack round-trip -- see parent.c's header - * comment for why spawn_connection() doesn't use one for - * per-connection wiring; this boot sequence already reads a - * fixed, statically known set of messages (just IMSG_AUTH_INIT - * above, then this) before ever touching the event loop, - * regardless of any ack. - */ + /* SS7: wires auth-worker's one and only peer via parent.c's spawn_connection()/setup_peer_send(), with no IMSG_SETUP_DONE ack needed since this boot sequence already reads a fixed, statically-known message set before touching the event loop. */ peer_fd = setup_recv_one_peer(&ibuf3); event_init(); @@ -253,27 +174,7 @@ auth_main(void) fatal("unveil lock"); } - /* - * No recvfd, no sendfd. This process receives exactly one descriptor - * in its life -- the peer fd from setup_recv_one_peer() above, which - * has already arrived by the time this line runs -- and it never - * sends one: the parent is the only process in the tree that attaches - * a descriptor to an imsg (parent.c's setup_peer_send(), - * setup_search_peer_send() and IMSG_LISTENER_SESSION_INIT are the - * only five such call sites). - * - * An earlier version of this comment kept both promises on the theory - * that an imsgbuf_allow_fdpass() channel uses sendmsg(2)/recvmsg(2) - * for all of its traffic. It does -- but that is not what the two - * promises gate. SYS_sendmsg and SYS_recvmsg are PLEDGE_STDIO - * (sys/kern/kern_pledge.c); "sendfd"/"recvfd" are checked in - * unp_internalize()/unp_externalize() (sys/kern/uipc_usrreq.c), which - * the kernel reaches only when SCM_RIGHTS is actually attached to the - * message. A plain imsg with fd == -1 needs neither. - * - * If that reasoning is wrong, pledge(2) does not degrade: a violation - * is an uncatchable SIGABRT with a core dump, and this line reverts. - */ + /* No recvfd, no sendfd: this process gets its one peer fd via setup_recv_one_peer() before this line and never attaches a descriptor to an imsg itself (only parent.c does); a plain imsg with fd == -1 needs neither pledge promise, and a wrong guess here is an uncatchable SIGABRT, not silent breakage. */ #ifdef __OpenBSD__ if (pledge("stdio rpath", NULL) == -1) fatal("pledge"); @@ -300,20 +201,7 @@ auth_dispatch(int fd, short event, void *arg) if ((n = imsgbuf_read(&iev->ibuf)) == -1) fatal("imsgbuf_read"); if (n == 0) { - /* - * SS7: this process was spawned (parent.c's - * spawn_connection()) to serve exactly this one - * connection's listener-worker and will never serve - * another -- exit now rather than sit in - * event_dispatch() forever with nothing left to do. - * parent.c's reap_child() already documents this - * exact expectation ("left to notice its own peer - * channel EOF and exit on its own") and already - * treats an auth-worker exit as the ordinary, - * expected end of a session, not something to warn - * about. Matches store.c's store_shutdown() for the - * same reason on that per-session worker. - */ + /* SS7: this auth-worker was spawned to serve exactly one connection and will never serve another, so it exits here on listener EOF rather than idling in event_dispatch() forever -- the ordinary, expected end of a session per parent.c's reap_child(). */ log_debug("auth-worker: listener closed channel, " "exiting"); exit(0); @@ -367,11 +255,7 @@ auth_dispatch(int fd, short event, void *arg) cred.session_id = res.session_id; cred.uid = res.uid; cred.gid = res.gid; - /* cred.maildir and res.maildir are both sized - * AUTH_MAILDIR_MAX, truncation is structurally - * impossible, so the return value is discarded - * deliberately, same as imsg_store_init's own - * maildir field elsewhere. */ + /* cred.maildir and res.maildir are both sized AUTH_MAILDIR_MAX, so truncation is structurally impossible and the strlcpy() return value is discarded deliberately. */ (void)strlcpy(cred.maildir, res.maildir, sizeof(cred.maildir)); if (imsg_compose(&iev_parent.ibuf, @@ -429,12 +313,7 @@ auth_dispatch_parent(int fd, short event, void *arg) (void)fd; } -/* - * Usernames come off the network, so they reach syslog only through this: - * anything outside printable ASCII becomes '?'. listener.c is meant to - * reject a bare CR/LF in a command line, but auth is a separate process and - * does not get to assume that held. - */ +/* Usernames come off the network and reach syslog only through this: anything outside printable ASCII becomes '?', since auth can't assume listener.c's CR/LF rejection held. */ static void auth_safe_name(const char *in, char *out, size_t outsize) { @@ -457,13 +336,7 @@ auth_verify(struct imsg_auth_request *req, struct imsg int found; char safename[AUTH_USERNAME_MAX]; - /* - * Budget spent: refuse without spending a bcrypt. Deliberately - * before cred_lookup(), so a client past the cap cannot even make - * this process re-read and re-scan the credential file. Logged and - * reported exactly like any other failure -- the client learns - * nothing it did not already know. - */ + /* Budget spent: refuses before cred_lookup() so a client past AUTH_MAX_TRIES can't even trigger a re-read/re-scan of the credential file; reported identically to any other failure. */ if (auth_failures >= AUTH_MAX_TRIES) { char overname[AUTH_USERNAME_MAX]; @@ -490,15 +363,7 @@ auth_verify(struct imsg_auth_request *req, struct imsg auth_failures++; } - /* - * Nothing used to record an authentication outcome at all, so a - * password-guessing run left no trace in the logs and there was - * nothing for pf(4)/fail2ban-style tooling to key on. log_info() is - * not gated on verbosity (see log.c), so these are always emitted. - * The failure line deliberately does not distinguish "no such user" - * from "wrong password" -- that would hand back the same enumeration - * oracle crypt_checkpass(NULL) exists to close. - */ + /* Logs every authentication outcome (previously nothing did, leaving password-guessing runs untraceable for fail2ban-style tooling); log_info() is always emitted, and the failure line deliberately doesn't distinguish "no such user" from "wrong password" to avoid an enumeration oracle. */ auth_safe_name(req->username, safename, sizeof(safename)); if (res->ok) log_info("session %u: authentication succeeded for \"%s\" " @@ -511,6 +376,17 @@ auth_verify(struct imsg_auth_request *req, struct imsg explicit_bzero(&ce, sizeof(ce)); } +/* True if a privileged file at st is safe to trust here: owned by root or the current (post-chroot, post-setresuid) uid, and not group-writable, group-executable, or accessible to world at all; same policy parse.y's check_file_secrecy() applies to imapd.conf (parse.y:678-695), kept as its own function since that one runs pre-privsep against an fd the parent still owns and logs a different message. */ +static int +cred_file_secure(const struct stat *st) +{ + if (st->st_uid != 0 && st->st_uid != getuid()) + return (0); + if (st->st_mode & (S_IWGRP | S_IXGRP | S_IRWXO)) + return (0); + return (1); +} + /* linear scan of "username:passwordhash:uid:gid:maildir" lines */ static int cred_lookup(const char *path, const char *username, struct cred_entry *out) @@ -524,19 +400,7 @@ cred_lookup(const char *path, const char *username, st return (-1); } - /* - * parent.c refuses to load the TLS private key unless it is - * root-owned and no looser than 0740; this file holds every user's - * bcrypt hash and had no such check. A flat text file people edit - * by hand very easily ends up 0644, at which point any local user - * can take the hashes away and attack them offline. - * - * Deliberately permissive about ownership and group-read, so the - * usual "root:_imapauth 0640" and "_imapauth 0400" layouts both - * pass; only world access and group-write are refused. Fails - * closed: a credential store with the wrong mode stops logins - * rather than serving them, and says so loudly. - */ + /* Rejects a credentials file not owned by root or the current uid, or that is group-writable, group-executable, or accessible to world at all (parent.c already enforces an equivalent policy for the TLS key, and parse.y's check_file_secrecy() for imapd.conf itself; this file holding every bcrypt hash had no such check until this one); group-read stays permissive so the documented "root:_imapauth 0640" layout keeps working, and a bad mode or owner fails closed. */ { struct stat st; @@ -545,11 +409,14 @@ cred_lookup(const char *path, const char *username, st fclose(fp); return (-1); } - if (st.st_mode & (S_IROTH | S_IWOTH | S_IXOTH | S_IWGRP)) { - log_warnx("%s: insecure permissions (mode %04o), must " - "not be world-accessible or group-writable; " - "refusing all authentication until this is fixed", - path, (unsigned)(st.st_mode & 07777)); + if (!cred_file_secure(&st)) { + log_warnx("%s: insecure (mode %04o, owner uid %u), " + "must be owned by root or the current user and " + "not group-writable, group-executable, or " + "world-accessible; refusing all authentication " + "until this is fixed", + path, (unsigned)(st.st_mode & 07777), + (unsigned)st.st_uid); fclose(fp); return (-1); } @@ -562,11 +429,7 @@ cred_lookup(const char *path, const char *username, st char *ep; unsigned long ulval; - /* - * fgets(3) splits an over-long line, and the tail would then be - * parsed as its own entry -- a phantom credential rather than an - * error. Refuse to read the file at all rather than guess. - */ + /* fgets(3) silently splits an over-long line, which would otherwise parse the tail as a phantom credential entry -- refuses to read the whole file rather than guess. */ if (strchr(line, '\n') == NULL && strlen(line) == sizeof(line) - 1) { log_warnx("%s: over-long line, refusing to parse the " @@ -599,39 +462,10 @@ cred_lookup(const char *path, const char *username, st strlcpy(out->passwordhash, fields[1], sizeof(out->passwordhash)) >= sizeof(out->passwordhash)) continue; - /* - * crypt_checkpass(3) returns SUCCESS when the stored hash - * and the supplied password are both empty - * (lib/libc/crypt/cryptutil.c's "empty password" case), so a - * blank second field is not a disabled account -- it is a - * login with an empty password. Require a bcrypt hash, for - * the same reason as the uid/gid checks below: the - * credential file should not be able to express this in the - * first place. - * - * Only the empty case needs catching. Any other non-bcrypt - * value ("!", "*", a legacy crypt string, garbage) already - * falls through crypt_checkpass()'s own "$2" test to its - * fake: label, which burns a bcrypt for timing and fails. - * - * "continue" rather than a distinct error, matching every - * other malformed-entry case here: the client must not be - * able to tell this apart from "no such user", or the - * enumeration oracle crypt_checkpass(NULL) exists to close - * comes back through the error text. - * - * To disable an account, use imapduser -d, which removes the - * line. - */ + /* Requires a bcrypt hash ("$2" prefix): crypt_checkpass(3) treats an empty stored hash plus empty password as a successful login rather than a disabled account, so a blank field must be rejected here, and non-bcrypt values are skipped the same way as every other malformed entry to avoid an enumeration oracle -- use imapduser -d to disable an account instead. */ if (fields[1][0] != '$' || fields[1][1] != '2') continue; - /* - * strtoul(3) accepts a leading "-", so "-1" would arrive here as - * 0xffffffff, and "0" is root. parent.c refuses uid/gid 0 at the - * spawn boundary, which is the check that matters -- but the - * credential file should not be able to express either in the - * first place, and the wrap case is caught nowhere else. - */ + /* strtoul(3) accepts a leading '-', so "-1" would parse as 0xffffffff (and "0" is root); the credential file shouldn't be able to express either uid/gid, so both are rejected here before the wrap case slips through unnoticed. */ if (fields[2][0] < '0' || fields[2][0] > '9' || fields[3][0] < '0' || fields[3][0] > '9') continue; @@ -654,12 +488,7 @@ cred_lookup(const char *path, const char *username, st break; } - /* - * line[] held the raw credential record -- username, bcrypt hash, - * uid, gid, maildir -- for every entry scanned. auth_verify() - * scrubs its struct cred_entry and auth_dispatch() scrubs the - * password; this buffer was the one left behind. - */ + /* line[] held the raw credential record (username, bcrypt hash, uid, gid, maildir) for every entry scanned; auth_verify() and auth_dispatch() scrub their own copies, so this buffer is the one left behind. */ explicit_bzero(line, sizeof(line)); fclose(fp); return (found ? 0 : -1); blob - a83bf2678a4738a483467710477fd2c2d2e0eb29 blob + 86be637642814c5966c967710f302bf7574884b3 --- src/auth_cmd.c +++ src/auth_cmd.c @@ -14,11 +14,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -/* - * auth_cmd.c, CAPABILITY/NOOP/LOGOUT/ID/LOGIN/STARTTLS/ - * AUTHENTICATE/ENABLE: command-any and command-nonauth handlers that - * don't need an established mailbox session. - */ +/* auth_cmd.c: CAPABILITY/NOOP/LOGOUT/ID/LOGIN/STARTTLS/AUTHENTICATE/ENABLE handlers that don't need a mailbox session. */ #include #include @@ -43,9 +39,7 @@ #include "log.h" #include "listener.h" -/* - * RFC 9051 SS6.1.1 capability strings, selected by session->tls_active. - */ +/* RFC 9051 SS6.1.1 capability strings, selected by session->tls_active. */ #define CAPABILITY_PRE_TLS "IMAP4rev2 STARTTLS LOGINDISABLED ID CONDSTORE QRESYNC" #define CAPABILITY_POST_TLS "IMAP4rev2 AUTH=PLAIN LOGINDISABLED ID CONDSTORE QRESYNC" @@ -96,10 +90,7 @@ cmd_id(struct session *s, const char *tag, char *args) } -/* - * LOGIN is permanently disabled, matching LOGINDISABLED in both - * CAPABILITY strings above. - */ +/* LOGIN is permanently disabled, matching LOGINDISABLED in both CAPABILITY strings above. */ int cmd_login(struct session *s, const char *tag, char *args) { @@ -211,15 +202,7 @@ sasl_plain_finish(struct session *s, const char *tag, session_reply(s, tag, "NO", "[SERVERBUG] internal error"); return (1); } - /* - * SS7: this connection's auth-worker may not exist -- parent.c's - * spawn_connection() tolerates that fork failing independently - * of the listener-worker's own. listener_main()'s boot-drain - * loop leaves iev_auth.ibuf.fd at -1 in that case rather than - * wiring it to a real peer (see listener.c's globals-block - * comment on iev_auth). Fail gracefully instead of composing to - * an unwired imsgev. - */ + /* SS7: the auth-worker may not exist if its fork failed (parent.c), leaving iev_auth.ibuf.fd at -1 -- fail gracefully rather than compose to an unwired imsgev. */ if (iev_auth.ibuf.fd == -1) { session_reply(s, tag, "NO", "[UNAVAILABLE] authentication " "temporarily unavailable"); @@ -310,8 +293,7 @@ cmd_authenticate(struct session *s, const char *tag, c } if (initial != NULL) { /* RFC 9051 SS6.2.2 initial-resp: finishes in one round trip */ - /* `initial` is the base64 of the cleartext password and points - * into s->inbuf; have the reader scrub it once consumed. */ + /* `initial` is the base64 cleartext password and points into s->inbuf; have the reader scrub it once consumed. */ s->scrub_inbuf = 1; return sasl_plain_finish(s, tag, initial, 1); } @@ -370,11 +352,7 @@ cmd_enable(struct session *s, const char *tag, char *a if (newly_condstore || newly_qresync) session_condstore_enable(s); - /* - * buf[64] can never truncate here: the only strings ever appended - * are these two fixed literals plus one separator space, 18 bytes - * total in the worst case ("QRESYNC CONDSTORE"). - */ + /* buf[64] can never truncate here: worst case is two fixed literals plus a space, 18 bytes ("QRESYNC CONDSTORE"). */ buf[0] = '\0'; if (newly_qresync) (void)strlcat(buf, "QRESYNC", sizeof(buf)); blob - 4ece4d36298f7a478bc532382f97e19b0132e581 blob + f9228ac75a7d99082afc200f02579cf5847e1b7a --- src/envelope.c +++ src/envelope.c @@ -40,8 +40,7 @@ int envbuf_append(char *buf, size_t bufsize, size_t *outlen, const char *data, size_t datalen) { - /* subtract rather than add: "*outlen + datalen" wraps if datalen is - * ever close to SIZE_MAX, and the check would then pass */ + /* Subtract rather than add -- "*outlen + datalen" could wrap near SIZE_MAX and falsely pass the check. */ if (*outlen > bufsize || datalen > bufsize - *outlen) return (-1); memcpy(buf + *outlen, data, datalen); @@ -72,9 +71,7 @@ envbuf_append_nstring(char *buf, size_t bufsize, size_ for (i = 0; i < vallen; i++) { char c = val[i]; - /* RFC 9051 SS4.3: a quoted string is TEXT-CHAR only, which - * excludes NUL, CR and LF; substitute rather than reject so - * one odd byte in a header doesn't drop the whole field */ + /* RFC 9051 SS4.3 quoted strings exclude NUL/CR/LF; substitute rather than reject so one bad byte doesn't drop the whole field. */ if (c == '\0' || c == '\r' || c == '\n') c = ' '; if ((c == '"' || c == '\\') && @@ -88,8 +85,7 @@ envbuf_append_nstring(char *buf, size_t bufsize, size_ return (0); fail: - /* all-or-nothing: a partial append leaves an unterminated quoted - * string in the caller's buffer */ + /* All-or-nothing: a partial append would leave an unterminated quoted string in the caller's buffer. */ *outlen = save; return (-1); } @@ -261,9 +257,7 @@ envbuf_append_one_address(char *buf, size_t bufsize, s if (envbuf_append(addrbuf, sizeof(addrbuf), &addrlen, ")", 1) == -1) return (-1); - /* one atomic append: either the whole "(...)" tuple lands in the - * caller's buffer or none of it does (envbuf_append() leaves - * *outlen untouched on failure) */ + /* One atomic append: the whole "(...)" tuple lands or none of it does, since envbuf_append() leaves *outlen untouched on failure. */ return (envbuf_append(buf, bufsize, outlen, addrbuf, addrlen)); } @@ -317,11 +311,7 @@ envbuf_append_address_list(char *buf, size_t bufsize, tok_len--; if (tok_len > 0) { - /* a malformed or non-fitting address leaves buf/outlen - * untouched -- envbuf_append_one_address() builds the - * whole "(...)" tuple locally before its one atomic - * append into buf -- so skipping it and continuing is - * safe */ + /* Safe to skip a malformed or non-fitting address and continue: envbuf_append_one_address() builds the tuple locally before one atomic append, leaving buf/outlen untouched on failure. */ if (envbuf_append_one_address(buf, bufsize, outlen, val + tok_start, tok_len) == 0) any = 1; blob - 3d9bc13cb2e5fed06610f3907760529a6f0c3fce blob + 204ba724b7978cb4705e7709d340b52914883ba1 --- src/fetch_cmd.c +++ src/fetch_cmd.c @@ -14,10 +14,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -/* - * fetch_cmd.c, FETCH: attribute/section-spec parsing and - * response building. - */ +/* FETCH: attribute/section-spec parsing and response building. */ #include #include @@ -60,14 +57,7 @@ parse_nz_number(const char *str, uint32_t *out) return (0); } -/* - * Parses one range token -- no ':'-split ambiguity beyond the existing - * single colon, no comma -- into r. This used to be parse_seq_range()'s - * entire body, factored out so parse_sequence_set() below can reuse it - * once per comma-separated segment instead of duplicating it (parse_ - * seq_range() itself is gone now -- QRESYNC known-uids, its last - * caller, moved to parse_sequence_set() directly; see mailbox_cmd.c). - */ +/* Parses one range token (single optional colon, no comma) into r; factored out of the old parse_seq_range() so parse_sequence_set() can reuse it per comma-separated segment. */ static int parse_one_seq_range(const char *tok, struct seq_range *r) { @@ -112,16 +102,7 @@ parse_one_seq_range(const char *tok, struct seq_range return (0); } -/* - * RFC 9051 SS9 sequence-set: (seq-number/seq-range) *("," seq-number/ - * seq-range) -- what every parse_seq_range() call site used to reject a - * comma for, rather than actually parse. Splits text on top-level commas - * (no nesting or quoting in this grammar, so a plain scan is exact) and - * parses each segment with parse_one_seq_range() above. Writes up to - * SEQSET_MAX_RANGES entries to ranges[] and the count to *nranges; - * returns -1 (with *errmsg set) if any segment is malformed, empty, or - * there are more segments than SEQSET_MAX_RANGES allows. - */ +/* Parses an RFC 9051 SS9 sequence-set (comma-separated seq-number/seq-range) by splitting on top-level commas and parsing each with parse_one_seq_range(), writing up to SEQSET_MAX_RANGES entries to ranges[] and the count to *nranges, or returning -1 with *errmsg set on a malformed, empty, or excess segment. */ int parse_sequence_set(const char *text, struct seq_range ranges[SEQSET_MAX_RANGES], uint32_t *nranges, const char **errmsg) @@ -833,15 +814,7 @@ parse_fetch_modifiers(char *modspec, struct imsg_mbox_ "mod-sequence value"; return (-1); } - /* - * RFC 7162 SS7: chgsince-fetch-mod takes a - * mod-sequence-value, "1*DIGIT ... (1 <= n <= - * 9,223,372,036,854,775,807)". strtoull(3) accepts a - * leading sign, so "-1" would otherwise arrive as - * ULLONG_MAX with errno untouched and match nothing, - * silently. Same first-character-is-a-digit guard - * auth.c, index.c and listener.c's literal parser use. - */ + /* RFC 7162 SS7 mod-sequence-values are unsigned only, but strtoull(3) accepts a leading sign, so a guard rejects non-digit-leading input to stop "-1" silently becoming ULLONG_MAX (same check as auth.c/index.c/listener.c's literal parser). */ if (*valtok < '0' || *valtok > '9') { *errmsg = "invalid CHANGEDSINCE mod-sequence"; return (-1); blob - ec0bf31d729d076828fe70cb87dd5dd50c2dd6a8 blob + 3063e8439e40e298e3a780bbc9955988325c4100 --- src/imapd.8 +++ src/imapd.8 @@ -3,7 +3,7 @@ .\" Written for the OpenIMAPD project. Public domain / no rights reserved, .\" matching the project's ports-oriented, OpenBSD-base-inclusion goal. .\" -.Dd $Mdocdate: September 6 2026 $ +.Dd $Mdocdate: September 9 2026 $ .Dt IMAPD 8 .Os .Sh NAME @@ -196,6 +196,8 @@ rereads and reloads the .Ic spool , .Ic attachment max , +.Ic idle poll , +.Ic startups , .Ic tls certificate , and .Ic tls key blob - 0cf2479b94bc7d44cefac9c27ab52e1c0ccfd64e blob + b39ecd65a8d22008183ce59025d7bcd9da215595 --- src/imapd.h +++ src/imapd.h @@ -31,7 +31,7 @@ #include #include -#define IMAPD_VERSION "0.1.3" +#define IMAPD_VERSION "0.1.4" /* * Process roles, selected at exec time via "-x ". See main.c. blob - 2aa0bb8c8aa051fa5f41d67e9576286e411585cf blob + 1867c3df066b4fed8fcc23aac6f1b02eb03727eb --- src/imsgev.c +++ src/imsgev.c @@ -1,6 +1,9 @@ /* * Copyright (c) 2026 David Williams * Copyright (c) 2009 Eric Faurot + * Copyright (c) 2005 Claudio Jeker + * Copyright (c) 2004 Esben Norby + * Copyright (c) 2003, 2004 Henning Brauer * * This file's name and its "struct imsgev" wrapper-around-imsgbuf+ * event(3) concept match Eric Faurot's imsgev.c in OpenBSD's ldapd @@ -10,6 +13,15 @@ * libevent handler, vs. ldapd's callback+needfd model) were written * independently and differ from his implementation. * + * imsgev_add() below is the exception: it is a verbatim copy (up to + * whitespace and the choice of iev vs. iev->data as event_set()'s last + * argument) of the imsg_event_add() idiom shared across OpenBSD privsep + * daemons, traceable to usr.sbin/ospfd/ospfd.c:525-534 (Claudio Jeker + * 2005, Esben Norby 2004, Henning Brauer 2003-2004) and copied with + * only that one-argument variation into dvmrpd.c, npppd.c, and rad.c. + * Their copyright is carried forward for that function specifically, + * same rationale as log.c's and parse.y's shared-idiom copyright chains. + * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above * copyright notice and this permission notice appear in all copies. @@ -23,21 +35,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -/* - * imsgev.c, shared wrapper around imsgbuf + event(3), used by - * parent.c, listener.c, auth.c, and store.c. - * - * IMSG API VERSION. This tree calls the current libutil imsg interface -- - * imsgbuf_init()/imsgbuf_read()/imsgbuf_write()/imsgbuf_flush()/ - * imsgbuf_clear() and imsgbuf_get() -- and not the older imsg_init()/ - * imsg_read()/imsg_get() spellings. That is not cosmetic: OpenBSD kept - * imsg_get() for a while as a compatibility wrapper (it called - * imsgbuf_get() and translated a success into a byte count) and has since - * removed it, so a build against current headers fails to link on that - * symbol alone. imsgbuf_get() returns 1 for a message, 0 for none and -1 on - * error; every call site in this tree tests only the 0 and -1 cases, which - * the wrapper passed through unchanged, so the switch was exact. - */ +/* Shared imsgbuf+event(3) wrapper for parent/listener/auth/store, built on the current imsgbuf_*() API (imsg_get() and friends were removed upstream); imsgbuf_get()'s 1/0/-1 return is handled exactly as before. */ #include @@ -48,26 +46,7 @@ #include "imapd.h" #include "log.h" -/* - * The one place an imsgbuf gets its imapd-wide settings. Every channel in - * the daemon goes through here, including the five that each role builds by - * hand on fd 3 before the event loop exists -- they used to open-code two of - * these three lines and omit the third, which left the two ends of the - * parent<->child channel disagreeing about the size limit. - * - * On imsgbuf_set_maxsize(3): its argument is the maximum PAYLOAD, not the - * maximum message. It adds IMSG_HEADER_SIZE before storing (libutil's - * imsgbuf_set_maxsize()), and imsgbuf_init(3) has already installed - * MAX_IMSGSIZE as the whole-message limit. So this call RAISES the limit by - * IMSG_HEADER_SIZE rather than clamping it -- which is fine, but it means - * every channel has to make the same call or the ends differ by 16 bytes. - * - * On imsgbuf_allow_fdpass(3): the parent fd-passes on each of these channels - * at spawn time (setup_peer_send(), IMSG_LISTENER_SESSION_INIT), so every - * channel needs it. Note that the parent is the ONLY process that ever - * attaches a descriptor to an imsg -- see the pledge comments in auth.c, - * keymgr.c, listener.c, search_oracle.c and store.c. - */ +/* Sets imapd-wide imsgbuf settings for every channel: imsgbuf_set_maxsize() raises the whole-message limit by IMSG_HEADER_SIZE since its argument is payload-only, and imsgbuf_allow_fdpass() is needed since the parent fd-passes on these channels at spawn time. */ void imsgev_ibuf_init(struct imsgbuf *ibuf, int fd) { @@ -78,29 +57,7 @@ imsgev_ibuf_init(struct imsgbuf *ibuf, int fd) imsgbuf_allow_fdpass(ibuf); } -/* - * Arm EV_WRITE for a channel that has just queued a message. - * - * libutil calls this from imsg_close() (imsg.c:397-399), which is the funnel - * every queueing path goes through: imsg_compose(), imsg_composev() and - * imsg_forward() all end there. So it fires once per message queued, on every - * path, and no call site can forget it. It replaces the 36 hand-written - * "imsgev_add() after every imsg_compose()" pairings this tree used to carry. - * - * imsgbuf_set_close_callback(3) and imsgbuf_set_userdata(3) arrived in - * OpenBSD commit 348f1fc0836b (2026-09-04) -- the same commit that removed - * imsg_get() -- explicitly "to replace the bad imsgev wrappers in various - * deamons". - * - * On the early return: a single FETCH can queue hundreds of messages, and - * imsgev_add() is event_del() + event_set() + event_add(). Once EV_WRITE is - * armed the rest of a batch has nothing to do. event_pending(3) is asked - * rather than iev->events because it reports what libevent actually holds and - * so cannot go stale; smtpd uses the same idiom (usr.sbin/smtpd/control.c:362). - * Auditing every early return in all 13 dispatch handlers found them all to be - * teardown paths, so iev->events would in fact have been safe here -- this is - * belt and braces, not a fix for a known hole. - */ +/* Arms EV_WRITE via libutil's imsg_close() callback so every queued message gets it exactly once; the early return skips re-arming once EV_WRITE is already pending mid-batch. */ static void imsgev_on_compose(struct imsgbuf *ibuf, void *arg) { @@ -126,15 +83,7 @@ imsgev_init(struct imsgev *iev, int fd, void (*handler event_set(&iev->ev, fd, iev->events, iev->handler, iev->data); event_add(&iev->ev, NULL); - /* - * After event_set()/event_add(), because the callback touches iev->ev, - * and after imsgev_ibuf_init(), because imsgbuf_init() memset()s the - * whole imsgbuf and would wipe both of these. Deliberately NOT done in - * imsgev_ibuf_init() itself: the five roles call that on fd 3 before - * any event loop exists and then compose synchronously through - * setup_recv_done_and_ack(), where a callback would reach an event that - * has never been event_set(). - */ + /* Must follow event_set()/event_add() (callback touches iev->ev) and imsgev_ibuf_init() (imsgbuf_init() memset()s the struct); not done inside imsgev_ibuf_init() itself since roles call it pre-event-loop on fd 3. */ imsgbuf_set_userdata(&iev->ibuf, iev); imsgbuf_set_close_callback(&iev->ibuf, imsgev_on_compose); } @@ -173,29 +122,7 @@ imsgev_add(struct imsgev *iev) event_add(&iev->ev, NULL); } -/* - * Re-arm a channel's read event at the end of its libevent dispatch handler. - * - * imsgev_init() arms EV_READ without EV_PERSIST, so libevent drops the event - * once it has fired. Every dispatch handler in this daemon must therefore - * re-arm before returning, or that channel is never read again -- a hang - * rather than a crash, and not one any single test names. - * - * This is the SAME work as imsgev_add(), on purpose, under a second name. - * imsgev_add()'s other job -- arming EV_WRITE after an imsg_compose() -- now - * belongs to imsgev_on_compose(), which libutil calls from imsg_close(). The - * two jobs used to be spelled identically at 49 call sites, which is what made - * "delete the 36 the callback replaces" a change no reviewer could check by - * reading the diff. Naming them apart is what made that diff legible; it is - * not a behaviour change, and delegating rather than duplicating keeps it from - * becoming one. - * - * Note that it still arms EV_WRITE when output is queued, because a handler - * that composed a reply and is now returning needs exactly that. - * - * See docs/Opus-5-security-review-2/Opus-5-DESIGN-imsgev-retirement.md - * sections 3 and 6. - */ +/* Re-arms EV_READ (which imsgev_init() sets without EV_PERSIST, so it drops after firing) at the end of every dispatch handler, delegating to imsgev_add() -- kept as a separate name for clarity, not different behavior. */ void imsgev_rearm_read(struct imsgev *iev) { blob - c7edf6f62c43e5467c67983c0fd8ca81f21dc298 blob + 9a77c2b79924002c66a151ee72482f4bebf69da2 --- src/index.c +++ src/index.c @@ -36,21 +36,8 @@ #include "log.h" #include "store_internal.h" -/* - * The index is a colon-delimited, line-oriented text file, so nothing - * written into one of its fields may contain ':', CR or LF -- an LF - * especially, since it would make the next index_save() emit a second, - * fully attacker-chosen physical line. index_append() has always - * enforced this for the basename it writes; the sites that need a - * keywords field bypass index_append() and hand-build the line, so the - * rule lives here where every one of them can share it. - */ -/* - * RFC 7162 SS7: a mod-sequence is a "Positive unsigned 63-bit integer - * (1 <= n <= 9,223,372,036,854,775,807)". Values read back from the index - * are bounded by it, as the client-facing parsers in mailbox_cmd.c, - * fetch_cmd.c and store_cmd.c bound the ones read off the wire. - */ +/* Index lines are colon-delimited text, so no field may contain ':', CR, or LF -- centralized here since keywords-field callers bypass index_append() and hand-build lines. */ +/* RFC 7162 SS7 bounds a mod-sequence to a positive 63-bit integer; values read back from the index are bounded the same way the wire-facing parsers already are. */ #define INDEX_MODSEQ_MAX INT64_MAX int @@ -59,20 +46,13 @@ index_field_valid(const char *field) return (field != NULL && strpbrk(field, ":\r\n") == NULL); } -/* - * A basename read back OUT of the index is concatenated into "new/%s" / - * "cur/%s" and handed to open(2)/stat(2)/rename(2) (mime.c, mbox_store.c). - * unveil(2) stops such a path leaving the maildir, but nothing stops it - * moving around inside it, so the format's rules are enforced on load as - * well as on the write side that already refuses these. - */ +/* A basename read from the index is pasted into paths for open(2)/stat(2)/rename(2); unveil(2) only stops it leaving the maildir, so load-time enforces the same format rules as the write side. */ int index_basename_valid(const char *basename) { const unsigned char *p; - /* strictly stronger than index_field_valid(): whatever is unsafe to - * write into a line is also unsafe to paste into a path */ + /* Strictly stronger than index_field_valid(): whatever is unsafe to write into a line is also unsafe to paste into a path. */ if (!index_field_valid(basename)) return (0); /* excludes "", ".", "..", and dotfiles in one test */ @@ -85,6 +65,27 @@ index_basename_valid(const char *basename) return (1); } +/* Grows idx->lines by doubling (from 16) when it is full; index_load() and index_append() carried byte-identical copies of this block, so the growth policy and its failure log now live in one place. Returns 0 when there is room for one more line, -1 on allocation failure (already logged). */ +static int +index_lines_grow(struct mbox_index *idx) +{ + size_t newcap; + char **newlines; + + if (idx->nlines < idx->cap) + return (0); + + newcap = (idx->cap == 0) ? 16 : idx->cap * 2; + if ((newlines = reallocarray(idx->lines, newcap, + sizeof(*idx->lines))) == NULL) { + log_warn("session %u: reallocarray index", session_id); + return (-1); + } + idx->lines = newlines; + idx->cap = newcap; + return (0); +} + int index_load(int fd, struct mbox_index *idx) { @@ -110,17 +111,7 @@ index_load(int fd, struct mbox_index *idx) } while (fgets(line, sizeof(line), fp) != NULL) { - /* - * fgets(3) silently splits a line longer than the buffer, - * and the remainder is then parsed as its own record. A - * filled buffer with no '\n' in it is ambiguous by itself - * -- it's also what a LEGAL maximum-length line looks like, - * since its own trailing '\n' doesn't fit in this read. - * Peek at the next byte to tell them apart: the line's own - * '\n' (or EOF, the last line in a file missing its final - * newline) means this was exactly one record; anything else - * means fgets(3) really did split it. - */ + /* fgets(3) silently splits an over-long line; peek at the next byte to distinguish a legal max-length line (next byte is '\n' or EOF) from an actual split record. */ if (strchr(line, '\n') == NULL && strlen(line) == sizeof(line) - 1) { int c = fgetc(fp); @@ -146,16 +137,7 @@ index_load(int fd, struct mbox_index *idx) goto fail; } *colon = '\0'; - /* - * Each of the three header fields is digits or - * nothing, for the reason index_parse_line() states - * below for the UID field: strtoul(3) and strtoull(3) - * accept leading whitespace and a sign, so "-1:1:1" - * would load a UIDVALIDITY of 4294967295 and " 5:1:1" - * a UIDVALIDITY of 5, neither of which this format can - * express. Same guard auth.c, listener.c and the three - * mod-sequence parsers use. - */ + /* Each header field is digits-or-nothing: strtoul(3)/strtoull(3) accept leading whitespace and a sign, so unguarded input like "-1:1:1" would silently parse into a bogus value; same guard used elsewhere. */ if (line[0] < '0' || line[0] > '9') { log_warnx("session %u: malformed " "UIDVALIDITY: %s", session_id, line); @@ -208,19 +190,8 @@ index_load(int fd, struct mbox_index *idx) continue; } - if (idx->nlines == idx->cap) { - size_t newcap = (idx->cap == 0) ? 16 : idx->cap * 2; - char **newlines = reallocarray(idx->lines, newcap, - sizeof(*idx->lines)); - - if (newlines == NULL) { - log_warn("session %u: reallocarray index", - session_id); - goto fail; - } - idx->lines = newlines; - idx->cap = newcap; - } + if (index_lines_grow(idx) == -1) + goto fail; if ((idx->lines[idx->nlines] = strdup(line)) == NULL) { log_warn("session %u: strdup index line", session_id); goto fail; @@ -243,10 +214,7 @@ index_load(int fd, struct mbox_index *idx) return (0); fail: - /* idx may hold a partial set of already-allocated lines at this - * point; index_free() is a safe no-op if it doesn't. Every caller - * (refresh_index() included) documents/relies on "-1 means idx is - * already freed" -- this is what makes that true. */ + /* idx may hold partially-allocated lines here; index_free() is a safe no-op, making "-1 means idx is already freed" true for every caller including refresh_index(). */ index_free(idx); fclose(fp); return (-1); @@ -261,11 +229,7 @@ index_parse_line(const char *line, struct index_rec *r memset(rec, 0, sizeof(*rec)); - /* - * strtoul(3) accepts leading whitespace and a sign, so ":x:y:1" - * would parse as UID 0 and "-1:x:y:1" as UID 4294967295. The field - * is digits or nothing. - */ + /* strtoul(3) accepts leading whitespace and a sign, so an unguarded UID field could parse ":x:y:1" as 0 or "-1:x:y:1" as 4294967295; the field must be digits-or-nothing. */ if (line[0] < '0' || line[0] > '9') { log_warnx("session %u: corrupt index line (UID field is not " "a decimal number)", session_id); @@ -291,15 +255,7 @@ index_parse_line(const char *line, struct index_rec *r } memcpy(rec->basename, p, (size_t)(q - p)); rec->basename[q - p] = '\0'; - /* - * About to be pasted into "new/%s" / "cur/%s" and passed to - * open(2)/stat(2)/rename(2) by every caller. Refuse traversal, - * hidden names and control bytes here rather than relying on - * unveil(2) to catch the ones that would leave the maildir -- it - * does nothing about the ones that stay inside it. Logged by UID, - * not by basename: the basename is exactly the untrusted text that - * should not reach syslog raw. - */ + /* Refuses traversal, hidden names, and control bytes before this basename is pasted into open(2)/stat(2)/rename(2) paths, since unveil(2) only stops paths leaving the maildir; logged by UID, never by the untrusted basename itself. */ if (!index_basename_valid(rec->basename)) { log_warnx("session %u: refusing index line with unsafe " "basename (UID %u)", session_id, rec->uid); @@ -317,16 +273,7 @@ index_parse_line(const char *line, struct index_rec *r memcpy(rec->keywords, p, (size_t)(r - p)); rec->keywords[r - p] = '\0'; - /* - * Same rule as the UID field above, which this function has - * always enforced -- the MODSEQ field twelve lines down did - * not get it. RFC 7162 SS7 bounds a mod-sequence at - * 9,223,372,036,854,775,807, and strtoull(3)'s sign handling - * would otherwise turn "-1" into 18446744073709551615 with - * errno untouched, a value that then flows into CHANGEDSINCE - * and UNCHANGEDSINCE comparisons and out to the client as a - * MODSEQ FETCH item. - */ + /* Same digit-or-nothing guard as the UID field, now applied to MODSEQ: RFC 7162 SS7 bounds it at 9,223,372,036,854,775,807, but strtoull(3)'s sign handling would otherwise turn "-1" into 18446744073709551615 and leak into CHANGEDSINCE/UNCHANGEDSINCE and client-visible MODSEQ. */ if (r[1] < '0' || r[1] > '9') { log_warnx("session %u: malformed per-message MODSEQ " "in index line: %s", session_id, line); @@ -373,44 +320,7 @@ index_max_uid(struct mbox_index *idx) return (v); } -/* - * Resolves every "*" in a parsed sequence-set (an array of struct - * seq_range, e.g. from IMSG_MBOX_FETCH's trailing array) against max -- - * the store's live index_max_uid() for a by-UID request, idx->nlines - * for a sequence-number request, same values handle_mbox_fetch() and - * friends have always resolved "*" against. - * - * RFC 9051 SS9: a seq-range is unordered ("the first sequence number - * may be smaller or larger than the second"). parse_one_seq_range() - * (fetch_cmd.c) already swaps a backwards LITERAL range at parse time, - * but skips the swap when either side is "*", since only this function - * knows what "*" resolves to -- so e.g. "5:*" on a 2-message mailbox - * arrives here as lo=5, hi=2 and is swapped to lo=2, hi=5 below, same - * as mbox_copy.c's COPY/MOVE handling has always done for this case - * (formerly duplicated there, now centralized here so every caller, - * including FETCH/STORE/UID EXPUNGE, gets the same RFC-correct - * behavior for a backwards "*"-involving range). - * - * The swap runs before the clamps below: lo is then clamped up to at - * least 1, and hi is additionally clamped down to max when clamp_hi is - * set, matching this codebase's existing per-mode behavior (sequence - * numbers can never legitimately exceed idx->nlines, but an explicit - * (non-"*") UID above the highest UID in use is left alone rather than - * clamped, since it's simply a range that won't match anything past - * the last message). Every range is kept, even one still degenerate - * after the swap and clamps (possible only when max itself is 0, i.e. - * an empty mailbox, e.g. "*:*" resolving to lo=1 hi=0 after the lo<1 - * clamp) -- seqset_contains() below correctly treats lo > hi as "never - * matches", and every caller's own scan is bounded by the same empty - * idx->nlines/no-UIDs-in-use condition, so this can't cause an - * incorrect match, only a harmless unmatchable entry. Ranges are not - * merged or sorted -- SEQSET_MAX_RANGES already bounds the count, so - * letting seqset_contains() below do one full pass per resolved range - * at each membership test is cheap enough not to be worth a merge - * step. Returns the number of ranges written to resolved[] (always - * nranges; unlike before this function grew the swap, no range is - * ever dropped). - */ +/* Resolves "*" entries in a parsed sequence-set against max (index_max_uid() for UID requests, idx->nlines for sequence-number requests); swaps any backwards "*"-involving range per RFC 9051 SS9 (since parse_one_seq_range() can't), then clamps lo up to 1 and, when clamp_hi is set, hi down to max, keeping every range including degenerate ones that seqset_contains() correctly treats as unmatchable. */ uint32_t seqset_resolve(const struct seq_range *ranges, uint32_t nranges, uint32_t max, int clamp_hi, struct seq_range resolved[SEQSET_MAX_RANGES]) @@ -452,13 +362,7 @@ seqset_contains(const struct seq_range *resolved, uint return (0); } -/* - * Highest hi across all resolved ranges (0 if nresolved == 0), for an - * early-exit bound on an ascending scan of idx->lines: once the loop's - * position/UID exceeds this, no later line can match any range, same - * early "break" every one of these loops already had for a single - * range's hi. - */ +/* Highest hi across all resolved ranges (0 if none), letting an ascending scan of idx->lines break early once past it, same as a single-range scan already did. */ uint32_t seqset_max_hi(const struct seq_range *resolved, uint32_t nresolved) { @@ -471,6 +375,20 @@ seqset_max_hi(const struct seq_range *resolved, uint32 return (max); } +/* The "does this command apply to this message?" rule for FETCH/STORE/COPY, in one place: RFC 9051 SS6.4.9 makes a UID command's sequence-set UID-space and a bare one position-space, so the caller passes both and by_uid picks. PAST_END is a stop signal, valid only because those three walk idx->lines in ascending order -- the compaction loops in move_same_mailbox()/handle_mbox_expunge() deliberately don't use this, since breaking early would leave the surviving lines they still have to copy down unwritten. */ +enum seqset_pos +seqset_position(const struct seq_range *resolved, uint32_t nresolved, + uint32_t max_hi, int by_uid, uint32_t uid, uint32_t seqno) +{ + uint32_t val = by_uid ? uid : seqno; + + if (val > max_hi) + return (SEQSET_PAST_END); + if (!seqset_contains(resolved, nresolved, val)) + return (SEQSET_SKIP); + return (SEQSET_MATCH); +} + /* Reports every UID in [lo, hi] absent from idx as IMSG_MBOX_SELECT_VANISHED ranges; RFC 7162 SS3.2.6 VANISHED modifier. */ void send_vanished_range(const struct mbox_index *idx, uint32_t lo, uint32_t hi, @@ -504,14 +422,7 @@ send_vanished_range(const struct mbox_index *idx, uint "IMSG_MBOX_SELECT_VANISHED", session_id); } - /* - * A UID of UINT32_MAX would wrap want to 0, after which the - * tail check below is trivially true and this function emits - * a VANISHED (EARLIER) range covering the whole UID space -- - * telling a QRESYNC client that every message in the mailbox - * is gone. Stop instead: there is nothing above this UID to - * report. - */ + /* Stop here: a UID of UINT32_MAX would wrap to 0 and make the tail check trivially true, emitting a VANISHED range that wrongly claims every message in the mailbox is gone. */ if (rec.uid == UINT32_MAX) return; want = rec.uid + 1; @@ -561,19 +472,7 @@ index_append(struct mbox_index *idx, uint32_t uid, con char line[STORE_INDEX_LINE_MAX]; int len; - /* - * RFC 9051 SS9 makes a uniqueid an nz-number, so UID 0 is not a UID. - * Callers assign from idx->uidnext and increment it afterwards, with - * no ceiling anywhere, so an exhausted uidnext wraps to 0 and the - * appends after that silently REUSE UIDs still in the mailbox -- - * which SS2.3.1.1 forbids outright, and which breaks - * index_max_uid()'s ascending-order assumption and every "*" - * resolution built on it. Refusing here turns that into a logged - * failure at the first append past the end. The RFC's actual answer - * to running out of UIDs is to change UIDVALIDITY, which needs - * persistent state this daemon does not keep yet; see the index.c - * review's finding #1. - */ + /* RFC 9051 SS9 forbids UID 0; with no ceiling on uidnext, exhaustion would wrap it to 0 and silently reuse in-use UIDs (forbidden by SS2.3.1.1, and breaking index_max_uid()'s ascending assumption), so refuse here instead -- the RFC's real fix, changing UIDVALIDITY, needs persistent state not yet kept (see index.c review's finding #1). */ if (uid == 0) { log_warnx("session %u: refusing index entry with UID 0 " "(uidnext exhausted or index header corrupt)", session_id); @@ -597,18 +496,8 @@ index_append(struct mbox_index *idx, uint32_t uid, con return (-1); } - if (idx->nlines == idx->cap) { - size_t newcap = (idx->cap == 0) ? 16 : idx->cap * 2; - char **newlines = reallocarray(idx->lines, newcap, - sizeof(*idx->lines)); - - if (newlines == NULL) { - log_warn("session %u: reallocarray index", session_id); - return (-1); - } - idx->lines = newlines; - idx->cap = newcap; - } + if (index_lines_grow(idx) == -1) + return (-1); if ((idx->lines[idx->nlines] = strdup(line)) == NULL) { log_warn("session %u: strdup index line", session_id); return (-1); @@ -722,18 +611,7 @@ qresync_send_resync(const struct imsg_mbox_select *req uint32_t nresolved, max_hi, i; if (req->qresync_has_uids) { - /* - * known-uids is a full RFC 9051 SS9 sequence-set - * (SS3.2.5.1), resolved/swapped/clamped the same way as - * every other UID-space consumer. "*" is forbidden in - * known-uids and already rejected by mailbox_cmd.c's - * parse_qresync_group(), so the max passed here is never - * actually consulted -- kept only for the same calling - * convention every other by-UID seqset_resolve() caller - * uses. clamp_hi is 0: a known UID above the highest one - * currently in use is exactly the case RFC 7162 SS3.2.5.1 - * wants reported VANISHED, not silently dropped. - */ + /* known-uids is a full RFC 9051 SS9 sequence-set resolved the same way as any UID-space consumer; max is unused since "*" is already rejected upstream, and clamp_hi is 0 because a known UID above the current highest is exactly what RFC 7162 SS3.2.5.1 wants reported VANISHED, not dropped. */ nresolved = seqset_resolve(ranges, nranges, index_max_uid(idx), 0, resolved); } else { @@ -746,17 +624,7 @@ qresync_send_resync(const struct imsg_mbox_select *req nresolved = 1; } - /* - * RFC 7162 SS3.2.6: VANISHED (EARLIER) MUST precede FETCH in the - * response stream; guaranteed not by send order here but by - * store_ipc.c's session_handle_mbox_selected(), which buffers - * both kinds separately while SESSION_SELECTING and flushes all - * VANISHED ranges before any FETCH -- the same two-pass split - * handle_mbox_fetch() already uses for its own FETCH ... - * (VANISHED) modifier, and send_vanished_range() itself is the - * exact same helper that call site uses, one resolved range at a - * time. - */ + /* RFC 7162 SS3.2.6 requires VANISHED (EARLIER) precede FETCH; ordering is guaranteed by store_ipc.c's session_handle_mbox_selected(), which buffers and flushes VANISHED before FETCH, the same two-pass split and helper handle_mbox_fetch() uses. */ for (i = 0; i < nresolved; i++) send_vanished_range(idx, resolved[i].lo, resolved[i].hi, iev); @@ -794,19 +662,7 @@ qresync_send_resync(const struct imsg_mbox_select *req } } -/* - * Takes the mailbox's index lock (op is LOCK_EX or LOCK_SH) and opens the - * index under it, filling *il. Returns 0, or -1 with nothing held. - * - * The order matters and is the whole point: the lock is taken on - * STORE_INDEX_LOCK_NAME, whose inode is stable, and the index is opened only - * afterwards, so the descriptor cannot refer to an inode that a concurrent - * index_save() has already renamed away. See STORE_INDEX_LOCK_NAME's comment - * in store_internal.h for what went wrong when the lock was taken on the - * index itself. - * - * Callers release with index_lock_release(), which is idempotent. - */ +/* Takes the index lock (LOCK_EX/LOCK_SH) on STORE_INDEX_LOCK_NAME's stable inode before opening the index, so the descriptor can't refer to an inode a concurrent index_save() already renamed away; release with the idempotent index_lock_release(). */ int index_lock_acquire(struct index_lock *il, int op) { @@ -852,35 +708,7 @@ index_lock_release(struct index_lock *il) } } -/* - * Issues a UIDVALIDITY for a mailbox that has none, and records it. - * - * RFC 9051 SS2.3.1.1 requires that a mailbox which loses its UIDs be given a - * UIDVALIDITY greater than the one it had before, and recommends a timestamp - * on the grounds that this "ensures that the value is unique and always - * increases". Two calls inside one second break that promise, and a client - * cannot detect it: an unchanged UIDVALIDITY is exactly how the protocol says - * "your cache is still good". - * - * So the timestamp is kept as a floor, not as the answer: the value returned - * is the later of the clock and one past the highest value this user has ever - * been issued, and the new high-water mark is written back before returning. - * The record lives at the maildir root (STORE_UIDVALIDITY_NAME) because the - * mailbox's own state is gone after DELETE -- that is what DELETE means -- so - * the memory has to outlive it. - * - * Called from index_load() with the mailbox's index lock already held. See - * STORE_UIDVALIDITY_NAME's comment for why this lock is safe to nest there, - * and for the rule that keeps it that way. - * - * Every failure degrades to the old behaviour -- a bare timestamp -- rather - * than failing the operation the caller was performing: a mailbox that opens - * with a slightly weaker UIDVALIDITY guarantee is better than a mailbox that - * will not open. One case deliberately does NOT write back: a file that - * exists and holds something unparseable is left exactly as it is, because - * overwriting it would replace a floor we failed to read with a lower one, - * which is the single worst thing this function could do. - */ +/* Issues and records a new UIDVALIDITY: RFC 9051 SS2.3.1.1 requires it strictly increase, so the timestamp is only a floor -- the value returned is max(clock, last-issued+1), the high-water mark is persisted at the maildir root (STORE_UIDVALIDITY_NAME, since per-mailbox state is gone after DELETE), and every failure degrades to a bare timestamp except an unparseable-but-present file, which is left untouched rather than overwritten with a lower floor. */ uint32_t uidvalidity_next(void) { @@ -894,11 +722,7 @@ uidvalidity_next(void) ssize_t n; int fd, writeback = 1; - /* - * The namespace is flat -- select_mailbox_dir() leaves a mailbox with - * a single chdir("..") -- so the root is either the cwd (INBOX) or - * exactly one level up. - */ + /* The namespace is flat -- select_mailbox_dir() reaches a mailbox with a single chdir("..") -- so the root is either the cwd (INBOX) or exactly one level up. */ path = current_mailbox_dir[0] == '\0' ? STORE_UIDVALIDITY_NAME : "../" STORE_UIDVALIDITY_NAME; @@ -917,11 +741,7 @@ uidvalidity_next(void) return (val != 0 ? val : 1); } - /* - * A zero-length file is the ordinary just-created case, not damage: - * floor 0 is correct for it. Anything present but unreadable is - * damage, and is left alone. - */ + /* A zero-length file is the ordinary just-created case (floor 0 is correct); anything present but unreadable is damage and is left alone. */ if (fstat(fd, &st) == 0 && st.st_size > 0) { if ((n = read(fd, buf, sizeof(buf) - 1)) <= 0) { log_warn("session %u: read %s (UIDVALIDITY floor)", @@ -930,9 +750,7 @@ uidvalidity_next(void) } else { buf[n] = '\0'; buf[strcspn(buf, "\r\n")] = '\0'; - /* same digit guard as the index header: strtoul(3) - * accepts a leading sign, so "-1" would read as - * 4294967295 and pin the floor at its ceiling */ + /* same digit guard as the index header: strtoul(3) accepts a leading sign, so "-1" would read as 4294967295 and pin the floor at its ceiling */ errno = 0; parsed = strtoul(buf, &ep, 10); if (buf[0] < '0' || buf[0] > '9' || *ep != '\0' || @@ -950,8 +768,7 @@ uidvalidity_next(void) if (writeback) { if (val <= floor) { if (floor == UINT32_MAX) { - /* 4 billion issued values, or a clock past - * 2106: nothing greater is representable. */ + /* 4 billion issued values, or a clock past 2106: nothing greater is representable. */ log_warnx("session %u: UIDVALIDITY floor is " "exhausted (%u); reusing it", session_id, floor); @@ -974,12 +791,7 @@ uidvalidity_next(void) "may be issued again", session_id, path); } - /* - * A successful floor consultation is otherwise silent, and a - * UIDVALIDITY that is quietly wrong looks exactly like one that is - * right -- the client cannot tell, which is the whole reason this - * function exists. At -v this says what was read and what was issued. - */ + /* A successful floor consultation is otherwise silent, and a quietly-wrong UIDVALIDITY looks identical to a correct one to the client; -v logs what was read and what was issued. */ log_debug("session %u: UIDVALIDITY: floor %u in %s -> issued %u%s", session_id, floor, path, val, writeback ? "" : " (floor NOT updated)"); @@ -988,21 +800,7 @@ uidvalidity_next(void) return (val); } -/* - * Walks new/ for maildir deliveries the index does not know about yet. - * - * mutate == 0 answers only "is there at least one?", stops at the first, and - * touches neither idx nor the filesystem -- so it is safe under a SHARED - * index lock. That is the question handle_mbox_idle_refresh()'s poll asks - * every few seconds, and the answer is almost always no. - * - * mutate == 1 indexes every one it finds, which is what refresh_index() wants - * and which needs the exclusive lock its callers hold. - * - * Returns 1 if anything was found (mutate == 1: added), 0 if not, -1 on error - * -- and on error with mutate set, idx has already been index_free()'d, which - * is refresh_index()'s long-standing contract with ITS callers. - */ +/* Walks new/ for undiscovered maildir deliveries: mutate==0 only answers "is there at least one?" without touching idx or the filesystem (safe under a shared lock, used by the frequent IDLE poll); mutate==1 indexes everything found and needs the exclusive lock. Returns 1 (found/added), 0, or -1 on error -- on error with mutate set, idx is already index_free()'d, per refresh_index()'s contract. */ static int index_scan_new(struct mbox_index *idx, int mutate) { @@ -1024,12 +822,7 @@ index_scan_new(struct mbox_index *idx, int mutate) continue; /* ".", "..", and dotfiles, maildir delivery never creates the latter */ /* never index a filename with ':' or newline, would corrupt the index line format */ if (strpbrk(de->d_name, ":\r\n") != NULL) { - /* - * Logged only on the mutating pass. The read-only one - * runs once per poll interval for the whole life of an - * IDLE, and one badly-named file would otherwise fill - * the log with the same line forever. - */ + /* Logged only on the mutating pass -- the read-only pass runs every poll interval for an IDLE's whole life, and a badly-named file would otherwise fill the log forever. */ if (mutate) log_warnx("session %u: skipping new/ file " "with unsafe name (contains ':' or " @@ -1066,12 +859,7 @@ refresh_index(struct mbox_index *idx, int fd) if ((added = index_scan_new(idx, 1)) == -1) return (-1); /* index_scan_new() has already freed idx */ - /* - * Nothing new: don't pay index_save()'s cost for a no-op refresh -- - * unless the header itself is new, in which case the cost is the - * point. A freshly invented UIDVALIDITY that is never written down is - * invented again, differently, on the next call. - */ + /* Skip index_save()'s cost on a no-op refresh, unless the header itself is new -- a freshly invented UIDVALIDITY that's never written down would just be invented again, differently, next call. */ if (!added && !idx->fresh) return (0); @@ -1082,33 +870,7 @@ refresh_index(struct mbox_index *idx, int fd) return (0); } -/* - * Cheap change probe for handle_mbox_idle_refresh(). - * - * Two stat(2) calls, no lock and no read, answering "can anything possibly - * have changed since the last look?". Both directories are needed and neither - * is redundant: - * - * "." every mutation imapd itself makes ends in index_save(), which - * creates imapd.index.tmp and rename(2)s it over imapd.index. Both - * are operations on this directory, so its mtime moves for APPEND, - * STORE, EXPUNGE, COPY and MOVE alike. - * "new" an external MTA's delivery lands a file here and touches nothing - * else until something indexes it, so "." alone would miss the one - * case IDLE exists for. - * - * The sample is taken and stored BEFORE the caller does any work, on purpose. - * Recording it afterwards would be tidier -- our own index_save() moves "."'s - * mtime, so a real change costs one extra no-op refresh on the following poll - * -- but it would also record a state that was never reported, and a write - * that landed between the work and the sample would then be invisible for - * good. One redundant refresh is a much better bug than a lost notification. - * - * The residual hole is timestamp granularity: two changes in the same - * nanosecond, either side of a sample, are indistinguishable. On OpenBSD's - * nanosecond st_mtim that is theoretical, and it is written down here rather - * than left to be rediscovered. - */ +/* Cheap change probe: two stat(2) calls (no lock, no read) on "." (moved by every index_save()-based mutation: APPEND/STORE/EXPUNGE/COPY/MOVE) and "new" (touched by an external MTA delivery before anything indexes it); sampled before the caller's work so a change is never missed, at the cost of one harmless extra refresh, modulo theoretical same-nanosecond races. */ static struct { int valid; ino_t dir_ino; @@ -1138,8 +900,7 @@ idle_probe_unchanged(void) int same; if (stat(".", &dst) == -1) { - /* cannot tell, so do not claim to know: fall through to the - * full refresh, which will report the failure properly. */ + /* Cannot tell, so do not claim to know: fall through to the full refresh, which will report the failure properly. */ idle_probe.valid = 0; return (0); } @@ -1175,34 +936,17 @@ handle_mbox_idle_refresh(struct imsgev *iev) memset(&reply, 0, sizeof(reply)); - /* - * Cheapest question first. On an untouched mailbox this is the whole - * of the work: no lock, no index read, and no IMSG_MBOX_IDLE_UID - * stream -- which matters because that stream is one message per - * message in the mailbox, and the poll runs every few seconds. - */ + /* Cheapest question first: on an untouched mailbox this is the whole job, with no lock, no index read, and no per-message IMSG_MBOX_IDLE_UID stream, which matters since the poll runs every few seconds. */ if (idle_probe_unchanged()) { reply.ok = 1; reply.unchanged = 1; - /* - * The whole mechanism is otherwise silent, and a poll that - * has quietly stopped firing is indistinguishable from a - * healthy daemon -- which is exactly how the dead - * cross-session push survived unnoticed. At -v these two - * lines make the poll observable: one per interval while - * nothing happens, the other when there is real work. - */ + /* The poll mechanism is otherwise silent and a dead one is indistinguishable from a healthy one (as the cross-session push bug showed); at -v these lines make each poll and any real work observable. */ log_debug("session %u: idle refresh: unchanged (probe: no " "change to . or new/)", session_id); goto send; } - /* - * Something moved, so the index must be read -- but reading is all - * the common case needs, so take the SHARED lock and escalate only if - * new/ actually holds a delivery that has to be written into the - * index. - */ + /* Something moved, so the index must be read; take the shared lock since reading alone covers the common case, and escalate only when new/ actually holds a delivery to index. */ if (index_lock_acquire(&il, LOCK_SH) == -1) goto send; if (index_load(il.fd, &idx) == -1) { @@ -1214,21 +958,9 @@ handle_mbox_idle_refresh(struct imsgev *iev) index_lock_release(&il); goto send; } - /* - * idx.fresh joins pending here: index_load() has just invented a - * UIDVALIDITY for a mailbox that had no index, and writing it down - * needs the exclusive lock as much as indexing a delivery does. - */ + /* idx.fresh joins pending here: index_load() just invented a UIDVALIDITY for a header-less mailbox, and persisting it needs the exclusive lock just as indexing a delivery does. */ if (pending || idx.fresh) { - /* - * Drop the shared lock and redo the whole thing exclusively. - * Deliberately not an in-place flock(2) upgrade: that - * conversion is not atomic, so another process can slip in - * between the two states and what was read under the shared - * lock cannot be trusted afterwards. Re-reading under - * LOCK_EX costs one extra index_load() on the rare path that - * had real work to do anyway. - */ + /* Deliberately drop the shared lock and redo everything under LOCK_EX rather than upgrading in place -- flock(2) has no atomic upgrade, so another process could slip in between states and invalidate what was read under the shared lock. */ index_free(&idx); index_lock_release(&il); if (index_lock_acquire(&il, LOCK_EX) == -1) blob - f7ebcb8722030c44e1c797cbe9f9048f4d8e536a blob + c359eac84115ad33f85b403204722cfa81cc2edc --- src/keymgr.c +++ src/keymgr.c @@ -41,33 +41,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -/* - * keymgr.c, TLS private-key isolation process: holds the real RSA/EC - * private key; listener.c gets libtls's "fake private key" instead and - * forwards every sign/decrypt operation here over imsg. See - * docs/openimap-tls-privsep-design.md SS5 for the full design. - * - * Deliberately diverges from ca.c's fatalx()-on-bad-request style for - * anything content-dependent: imapd's parent.c does not auto-restart a - * dead listener/auth/keymgr child (see parent.c's reap_child()), it only - * logs and tells the operator to "rcctl restart imapd" -- a crashed - * keymgr would be worse than smtpd's ca dying, since every future TLS - * handshake needs it. This file follows the same fatal/graceful split - * every other role in this tree already uses (compare auth_main()'s - * fatalx()-on-bad-INIT-framing vs. auth_verify()'s graceful "wrong - * password" reply, or listener_main()'s own "no TLS cert/key received" - * warning instead of a fatalx()): boot-time *plumbing* failures (no - * peer, a parent that closes the channel mid-handshake, getpwnam/ - * chroot/privilege-drop failing) are still fatalx() -- there is no - * sensible degraded mode for those. Boot-time *content* failures (an - * unparseable cert or key) and any later per-request failure (unknown - * hash, a key that won't do the requested operation) are logged and - * degrade instead: keymgr keeps running with no usable key (or its last - * known-good one), and answers every signing request with a plain - * failure until a good SIGHUP reload arrives, exactly mirroring how - * listener.c already treats its own "no TLS cert/key received" case as - * non-fatal. - */ +/* keymgr.c: holds the real TLS private key for listener.c's fake-key/imsg forwarding (docs/openimap-tls-privsep-design.md SS5); boot-time plumbing failures are fatal, but content failures (bad cert/key) and per-request failures degrade gracefully, keeping the process alive with no usable key rather than crashing. */ #include #include @@ -92,28 +66,11 @@ #include "imapd.h" #include "log.h" -/* - * Matches parent.c's send_tls_cert()/send_keymgr_key() read buffer size - * (8192), same reasoning as listener.c's own TLS_CERT_MAX -- kept as a - * separate local constant rather than a shared imapd.h macro, same as - * parent.c's own unnamed 8192 and listener.c's TLS_CERT_MAX/TLS_KEY_MAX - * today; nothing outside this file needs to agree on the exact value, - * only that it's large enough for parent.c's own read buffer. - */ +/* Matches parent.c's read buffer size (8192), kept as a separate local constant rather than a shared imapd.h macro since nothing else needs to agree on the exact value. */ #define KEYMGR_CERT_MAX 8192 #define KEYMGR_KEY_MAX 8192 -/* - * SS7: keymgr is the one remaining boot-time, daemon-lifetime - * singleton (this file's header comment) but now serves every live - * connection's listener-worker, not just one -- each gets its own - * peer entry, wired in as parent.c's spawn_connection() forks it - * (keymgr_dispatch_parent()'s own IMSG_SETUP_PEER case below), torn - * down independently when that one listener-worker's channel closes - * (keymgr_dispatch_listener()). Named (not anonymous) so a forward - * declaration isn't needed above keymgr_dispatch_parent()'s use of - * the type -- same reasoning as listener.h's own struct session_list. - */ +/* SS7: keymgr now serves every live connection's listener-worker via its own peer entry, wired in by IMSG_SETUP_PEER and torn down on channel close; named so keymgr_dispatch_parent() doesn't need a forward declaration. */ struct keymgr_peer { uint32_t session_id; struct imsgev iev; @@ -138,6 +95,7 @@ static char keymgr_hash[KEYMGR_HASH_MAX]; static int keymgr_got_init; static int keymgr_load(const char *, size_t, const char *, size_t); +static void keymgr_try_reload(void); static int keymgr_pubkey_hash(X509 *, char *, size_t); static void keymgr_dispatch_listener(int, short, void *); static void keymgr_peer_teardown(struct keymgr_peer *); @@ -165,20 +123,7 @@ keymgr_main(void) /* fd-passing is allowed on this channel for the fd-passed IMSG_SETUP_PEER peer fds; see imsgev_ibuf_init()'s own comment */ imsgev_ibuf_init(&ibuf3, 3); - /* - * IMSG_TLS_CERT (cert bytes) and IMSG_KEYMGR_INIT (key bytes) must - * both be read before the peer handshake -- same "config before - * anything else" ordering auth_main() already uses for - * IMSG_AUTH_INIT (auth.c's comment: "must be read first"), so that - * keymgr_got_init/keymgr_pkey are in their final boot-time state - * before listener's peer channel can possibly send a signing - * request. Two separate imsg types rather than one combined - * payload deliberately mirrors parent.c's send_tls_cert()/ - * send_keymgr_key() split (see parent.c's send_keymgr_init() - * comment) rather than packing cert+key into a single imsg, which - * would leave uncomfortably little headroom under MAX_IMSGSIZE - * (16384) once both are near their own 8192-byte caps. - */ + /* Both IMSG_TLS_CERT and IMSG_KEYMGR_INIT must be read before the peer handshake so keymgr_got_init/keymgr_pkey are final before any signing request can arrive; sent as two imsgs (mirroring parent.c's send_tls_cert()/send_keymgr_key() split) rather than one, to stay comfortably under MAX_IMSGSIZE. */ while (!got_cert || !got_key) { if ((n = imsgbuf_get(&ibuf3, &imsg)) == -1) fatal("imsgbuf_get"); @@ -254,17 +199,7 @@ keymgr_main(void) setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid) == -1) fatal("cannot drop privileges to _imapkey"); - /* - * SS7: unlike listener/auth-worker, keymgr stays the one - * boot-time, daemon-lifetime child (this file's header comment) - * -- but no peer is wired to it at boot any more. parent.c's - * boot sequence sends it only IMSG_SETUP_DONE, with no preceding - * IMSG_SETUP_PEER (see parent.c's own boot comment on why); its - * first, and every later, listener-worker peer instead arrives - * post-boot over this same fd 3 channel, as an ordinary - * IMSG_SETUP_PEER per spawn_connection() call, handled by - * keymgr_dispatch_parent()'s own case below -- not here. - */ + /* SS7: keymgr stays the one boot-time, daemon-lifetime child, but no peer is wired to it at boot -- parent.c sends only IMSG_SETUP_DONE; every listener-worker peer arrives later over this same channel via IMSG_SETUP_PEER, handled below. */ setup_recv_done_and_ack(&ibuf3); event_init(); @@ -273,25 +208,7 @@ keymgr_main(void) NULL); #ifdef __OpenBSD__ - /* - * recvfd, and only recvfd. Unlike every other child, keymgr keeps - * receiving descriptors for its whole life: it is the one - * daemon-lifetime singleton, and spawn_connection() sends it a fresh - * IMSG_SETUP_PEER per accepted connection (parent.c), handled by - * keymgr_dispatch_parent()'s own case below. - * - * No sendfd. keymgr never attaches a descriptor to an imsg -- the - * parent is the only process in the tree that does. An earlier - * version of this comment claimed sendfd was needed "for - * imsg_compose()'s reply path"; it is not. SYS_sendmsg is - * PLEDGE_STDIO (sys/kern/kern_pledge.c), and "sendfd" is checked in - * unp_internalize() (sys/kern/uipc_usrreq.c), which the kernel - * reaches only when SCM_RIGHTS is actually attached. keymgr_reply() - * composes with fd == -1. - * - * No rpath either: keymgr touches no filesystem at all (SS5.5), which - * is why this is auth.c's promise minus rpath. - */ + /* recvfd only: keymgr keeps receiving peer fds via IMSG_SETUP_PEER for its whole life but never sends one (only parent attaches descriptors to imsgs, and keymgr_reply() composes with fd == -1); no rpath either since keymgr touches no filesystem (SS5.5). */ if (pledge("stdio recvfd", NULL) == -1) fatal("pledge"); #endif @@ -300,26 +217,12 @@ keymgr_main(void) fatalx("exited event loop"); } -/* - * Replicates smtpd's ssl.c hash_x509() byte-for-byte (see this file's - * header comment for why the exact format is load-bearing, not - * cosmetic): SHA256 digest of the certificate's DER SubjectPublicKeyInfo, - * formatted "SHA256:" followed by lowercase hex. - */ +/* Replicates smtpd's ssl.c hash_x509() byte-for-byte: SHA256 of the cert's DER SubjectPublicKeyInfo, formatted "SHA256:" plus lowercase hex -- the exact format is load-bearing (see file header), not cosmetic. */ static int keymgr_pubkey_hash(X509 *cert, char *hash, size_t hashlen) { static const char hex[] = "0123456789abcdef"; - /* - * unsigned char/unsigned int, not smtpd hash_x509()'s char/int: - * X509_pubkey_digest(3) takes "unsigned char *md, unsigned int - * *len" (x509.h), and the signed spellings draw a -Wpointer-sign - * and an incompatible-pointer diagnostic under this Makefile's - * -Wall. The emitted string is unchanged -- with an unsigned - * digest, digest[i] >> 4 is already the high nibble in 0..15, so - * the & 0x0f below becomes redundant rather than wrong, and it is - * kept so this stays visibly the same algorithm as hash_x509(). - */ + /* Uses unsigned char/unsigned int, not smtpd hash_x509()'s signed types, to match X509_pubkey_digest(3)'s prototype and avoid -Wpointer-sign warnings; the emitted string is unchanged since digest[i] is already unsigned. */ unsigned char digest[EVP_MAX_MD_SIZE]; size_t off; unsigned int dlen, i; @@ -338,23 +241,7 @@ keymgr_pubkey_hash(X509 *cert, char *hash, size_t hash return (0); } -/* - * Parses a cert+key pair and, only if both parse successfully, replaces - * the currently-loaded key. Deliberately parses the new pair fully - * before touching the old one, unlike a literal "free the old EVP_PKEY/ - * hash, then install the new one": a rejected or malformed SIGHUP - * reload (a typo'd path, a half-written file mid-rotation on the - * operator's side) should leave keymgr still answering with the last - * known-good key, not with none at all. This is not sourced against - * smtpd's ca.c, whose own dict_check()/dict_xset() reload behavior this - * project's design document already flags as unconfirmed either way - * (docs/openimap-tls-privsep-design.md SS5.5) -- it's a small, - * self-contained imapd choice, not a smtpd port. - * - * cert_buf/key_buf are not retained past this call; only the derived - * EVP_PKEY and hash string are kept. Callers are responsible for - * scrubbing their own copy of key_buf once this returns. - */ +/* Parses a new cert+key pair fully before replacing the live one, so a malformed SIGHUP reload leaves the last known-good key in place instead of none; not sourced from smtpd's ca.c reload logic. cert_buf/key_buf aren't retained past this call -- callers must scrub key_buf themselves. */ static int keymgr_load(const char *cert_buf, size_t cert_len, const char *key_buf, size_t key_len) @@ -392,21 +279,7 @@ keymgr_load(const char *cert_buf, size_t cert_len, con goto fail; } - /* - * Both halves parsing is not the same as them belonging together. - * A rotation that replaced the certificate but not the key (or the - * reverse) yields exactly that: two files that each parse cleanly - * and do not correspond. It is the most common way a rotation goes - * wrong, and it is precisely the case this function's - * parse-both-before-swapping design exists to survive -- so check - * it here, where both objects are in hand, and let the goto below - * keep the last known-good pair. - * - * The per-request hash check in keymgr_handle_rsa()/_ecdsa() - * cannot catch this: it compares the request's certificate hash - * against this process's certificate hash, never the certificate - * against the key. - */ + /* A cert and key can each parse fine yet not correspond to each other (e.g. a rotation that replaced only one) -- checked here via X509_check_private_key() before swapping, since the per-request hash check elsewhere can't catch a cert/key mismatch. */ if (X509_check_private_key(cert, pkey) != 1) { log_warnx("certificate and private key do not match, not " "(re)loading"); @@ -418,9 +291,7 @@ keymgr_load(const char *cert_buf, size_t cert_len, con EVP_PKEY_free(keymgr_pkey); keymgr_pkey = pkey; pkey = NULL; - /* strlcpy, not memcpy of sizeof(): keymgr_pubkey_hash() writes 72 - * of KEYMGR_HASH_MAX's 80 bytes, and copying the rest would drag - * eight uninitialised stack bytes into a static. */ + /* strlcpy, not memcpy of sizeof(): keymgr_pubkey_hash() only writes 72 of KEYMGR_HASH_MAX's 80 bytes, and memcpy would drag uninitialised stack bytes into a static. */ (void)strlcpy(keymgr_hash, hash, sizeof(keymgr_hash)); log_info("TLS key loaded (%s)", keymgr_hash); @@ -442,6 +313,20 @@ fail: return (-1); } +/* Commits a SIGHUP reload once BOTH halves have arrived: IMSG_TLS_CERT and IMSG_KEYMGR_INIT can land in either order, so both cases call this and only the second one finds the pair complete. The key buffer is scrubbed whether or not the load succeeded, and both flags clear so the next reload starts from a clean pair rather than half of this one. */ +static void +keymgr_try_reload(void) +{ + if (!reload_got_cert || !reload_got_key) + return; + + if (keymgr_load(reload_cert_buf, reload_cert_len, reload_key_buf, + reload_key_len) == -1) + log_warnx("SIGHUP reload: keeping previous key, see above"); + explicit_bzero(reload_key_buf, sizeof(reload_key_buf)); + reload_got_cert = reload_got_key = 0; +} + /* PARENT channel (fd 3): SIGHUP reload's IMSG_TLS_CERT/IMSG_KEYMGR_INIT pair (same paired-flags shape listener.c used for its own now-removed cert/key reload gating), plus IMSG_SETUP_PEER wiring in a fresh listener-worker's peer (SS7: one per parent.c's spawn_connection() call, imsg_get_id() carries session_id -- see listener.c's own IMSG_SETUP_PEER (store) case for the same pattern). */ static void keymgr_dispatch_parent(int fd, short event, void *arg) @@ -458,42 +343,7 @@ keymgr_dispatch_parent(int fd, short event, void *arg) if ((n = imsgbuf_read(&iev->ibuf)) == -1) fatal("imsgbuf_read"); if (n == 0) { - /* - * The parent is gone, so this process is done -- the - * same answer keymgr_main() already gives to the - * identical event before the event loop starts - * ("parent closed channel before INIT"), and what - * keymgr_dispatch_listener()'s comment below has - * always said this channel means. - * - * It used to event_del() and return, which left - * keymgr serving its existing listener peers until - * the last one closed. That protected nothing: - * keymgr is reached only through libtls's - * private-key callbacks, which fire during the TLS - * handshake, so an established session never asks it - * for anything again. What it cost was a process - * holding the TLS private key outliving its - * supervisor for as long as one client kept a - * connection open -- and IDLE means days -- while an - * "rcctl restart imapd" brought up a second keymgr - * with the same key. - * - * log_warnx, not log_debug: unlike auth-worker's and - * search-oracle's peer EOF, this is not the ordinary - * end of anything. An orderly shutdown SIGTERMs - * keymgr (parent.c's sigterm_handler()), so reaching - * here means the parent died without running it -- - * a crash, a SIGKILL, the OOM killer. An operator - * should see that without -v. - * - * exit(0), not fatalx: keymgr has not failed. It is - * ending because the process it exists to serve - * ended. fatalx would log "fatal:" at LOG_CRIT and - * exit 1, which misreports an orderly response to - * someone else's death -- and there is no parent - * left to read the status anyway. - */ + /* Parent gone means this process is done: it used to linger serving existing peers, but keymgr is only ever consulted during a TLS handshake, so that only kept a key-holding process alive for as long as any client held a connection open; log_warnx (an operator should notice) and exit(0) (not a failure, just following the parent's death) rather than fatalx(). */ log_warnx("parent closed channel, exiting"); exit(0); } @@ -543,17 +393,7 @@ keymgr_dispatch_parent(int fd, short event, void *arg) } reload_cert_len = len; reload_got_cert = 1; - if (reload_got_cert && reload_got_key) { - if (keymgr_load(reload_cert_buf, - reload_cert_len, reload_key_buf, - reload_key_len) == -1) - log_warnx("SIGHUP reload: " - "keeping previous key, see " - "above"); - explicit_bzero(reload_key_buf, - sizeof(reload_key_buf)); - reload_got_cert = reload_got_key = 0; - } + keymgr_try_reload(); break; } case IMSG_KEYMGR_INIT: { @@ -572,17 +412,7 @@ keymgr_dispatch_parent(int fd, short event, void *arg) } reload_key_len = len; reload_got_key = 1; - if (reload_got_cert && reload_got_key) { - if (keymgr_load(reload_cert_buf, - reload_cert_len, reload_key_buf, - reload_key_len) == -1) - log_warnx("SIGHUP reload: " - "keeping previous key, see " - "above"); - explicit_bzero(reload_key_buf, - sizeof(reload_key_buf)); - reload_got_cert = reload_got_key = 0; - } + keymgr_try_reload(); break; } default: @@ -596,14 +426,7 @@ keymgr_dispatch_parent(int fd, short event, void *arg) (void)fd; } -/* - * LISTENER channel: the three signing/decrypt request types - * (SS5.2/SS6.1). SS7: arg is this peer's owning struct keymgr_peer, - * not the bare struct imsgev directly (imsgev_init()'s own arg, - * set when keymgr_dispatch_parent()'s IMSG_SETUP_PEER case wires a - * new listener-worker in) -- needed on the EOF path below to know - * which one of possibly many live peers just went away. - */ +/* LISTENER channel: handles the three signing/decrypt request types (SS5.2/SS6.1); arg is the owning struct keymgr_peer (not a bare imsgev) so the EOF path knows which of possibly many live peers just went away. */ static void keymgr_dispatch_listener(int fd, short event, void *arg) { @@ -612,28 +435,7 @@ keymgr_dispatch_listener(int fd, short event, void *ar struct imsg imsg; ssize_t n; - /* - * A transport failure on ONE listener-worker's channel drops that - * peer, it does not end this process. This is the same - * plumbing-vs-degrade split this file's header comment already - * describes, applied to the per-peer channel: keymgr is the one - * daemon-lifetime singleton and parent.c's reap_child() - * deliberately does not restart it, so fatal()ing here would turn - * one connection's worker dying at the wrong moment (a crash, a - * SIGKILL, the shutdown race) into "no TLS for the whole daemon - * until an operator runs rcctl restart". EPIPE from a peer that - * went away with a reply still queued is exactly that case -- - * keymgr_reply() composes and returns, so replies really do sit - * queued for the event loop to write. - * - * parent.c's store_child_dispatch() is the in-tree precedent for - * this shape. keymgr_dispatch_parent() stays strict: that is the - * fd-3 channel, and a keymgr that has lost its parent has no - * future. Its transport errors fatal() and, since 2026-09-05, its - * EOF exits too -- that last path used to drain instead, which is - * the one place this sentence was describing something the code - * did not do. - */ + /* A transport failure on one listener-worker's channel drops only that peer, not the whole process -- fatal()ing here would turn one connection's worker dying into a daemon-wide TLS outage, since keymgr is never restarted by parent.c's reap_child(); keymgr_dispatch_parent() (the fd-3 channel) stays strict since losing the parent leaves keymgr with no future. */ if (event & EV_WRITE) { if (imsgbuf_write(&iev->ibuf) == -1) { log_warnx("session %u: write error on listener " @@ -683,18 +485,7 @@ keymgr_dispatch_listener(int fd, short event, void *ar (void)fd; } -/* - * Drops one listener-worker peer: unregisters its event, closes and - * clears its channel, unlinks it and frees it. Shared by every exit in - * keymgr_dispatch_listener() -- EOF and transport error alike -- which - * is the point: those are the same underlying event (that worker is - * gone) arriving through two code paths, and they must not have two - * different outcomes. - * - * imsgbuf_clear() is not optional: imsgbuf_init() allocates, and - * close(2) alone would leak it, one allocation per connection for the - * life of the daemon. - */ +/* Drops one listener-worker peer: unregisters its event, closes and clears its channel, unlinks and frees it -- shared by every exit path in keymgr_dispatch_listener() so EOF and transport error give the same outcome; imsgbuf_clear() is required or imsgbuf_init()'s allocation leaks. */ static void keymgr_peer_teardown(struct keymgr_peer *kp) { @@ -735,16 +526,7 @@ keymgr_reply(struct imsgev *iev, uint32_t type, uint32 struct imsg_keymgr_sign_reply rep; unsigned char combined[sizeof(rep) + KEYMGR_DATA_MAX]; - /* - * Every caller bounds its own result today (both handlers check - * RSA_size()/ECDSA_size() against their output buffer before - * operating), but combined[] is a fixed stack buffer in the - * key-holding process and this function should not depend on that - * discipline holding forever -- keymgr_recv_trailing(), its - * mirror image on the inbound side, does bound-check. A result - * that does not fit is reported as a failed operation, which is - * what struct imsg_keymgr_sign_reply's ok field is for. - */ + /* Every caller already bounds its own result, but combined[] is a fixed stack buffer holding the private key's output, so this function bound-checks independently rather than relying on that discipline holding forever; an oversized result is reported as a failed operation. */ if (ok && tolen > KEYMGR_DATA_MAX) { log_warnx("keymgr_reply: %zu-byte result exceeds " "KEYMGR_DATA_MAX (%d), refusing", tolen, @@ -764,12 +546,7 @@ keymgr_reply(struct imsgev *iev, uint32_t type, uint32 sizeof(rep) + (ok ? tolen : 0)) == -1) log_warn("imsg_compose reply"); - /* - * imsg_compose() has copied it. On an RSA_PRIVDEC this buffer - * held the session's decrypted premaster secret; the file scrubs - * the key material it is given (key_buf, reload_key_buf) and the - * output of the operations it performs belongs in the same rule. - */ + /* imsg_compose() has copied the reply; this buffer may hold a decrypted premaster secret (on RSA_PRIVDEC), so it's scrubbed here like every other key-material buffer in this file. */ explicit_bzero(combined, sizeof(combined)); } @@ -840,8 +617,7 @@ keymgr_handle_rsa(struct imsgev *iev, struct imsg *ims return; } keymgr_reply(iev, type, id, 1, to, (size_t)ret); - /* RSA_PRIVDEC's output is the session's decrypted premaster - * secret; do not leave it on this process's stack. */ + /* RSA_PRIVDEC's output is the session's decrypted premaster secret; don't leave it on this process's stack. */ explicit_bzero(to, sizeof(to)); } blob - 690803c7796434b4318d87e9ef1ca9a9282726d0 blob + 6d37b8daf020763e4daddf91c126a00a30bea7d4 --- src/listener.c +++ src/listener.c @@ -69,27 +69,11 @@ struct session_list sessions = TAILQ_HEAD_INITIALIZER(sessions); -struct imsgev iev_auth; /* channel to the AUTH process; .ibuf.fd == -1 - * if this connection's auth-worker spawn failed - * (see listener_main()'s boot-drain comment) -- - * auth_cmd.c's sasl_plain_finish() checks this - * before sending IMSG_AUTH_REQUEST */ -struct imsgev iev_search; /* channel to the search-oracle process (SS8.1); - * .ibuf.fd == -1 if this connection's oracle - * spawn failed (same boot-drain comment as - * iev_auth above) -- search_cmd.c's - * search_dispatch() checks this before sending - * IMSG_SEARCH_PARSE_REQUEST */ +struct imsgev iev_auth; /* Channel to the AUTH process; .ibuf.fd == -1 if this connection's auth-worker spawn failed, checked by auth_cmd.c's sasl_plain_finish() before sending IMSG_AUTH_REQUEST. */ +struct imsgev iev_search; /* Channel to the search-oracle process (SS8.1); .ibuf.fd == -1 if spawn failed, checked by search_cmd.c's search_dispatch() before sending IMSG_SEARCH_PARSE_REQUEST. */ struct imsgev iev_parent; /* fd 3, alive for the process's lifetime */ -/* - * Channel to the keymgr process: real TLS private-key operations are - * forwarded here synchronously, from inside OpenSSL's RSA_METHOD/ - * EC_KEY_METHOD callbacks (keymgr_engine_init() below), never through - * the normal event-driven imsgev dispatch -- nothing unsolicited ever - * arrives on this channel, so it's a plain struct imsgbuf, not a - * struct imsgev; see keymgr_forward_rsa()/keymgr_forward_ecdsa(). - */ +/* Channel to the keymgr process: private-key ops are forwarded here synchronously from OpenSSL callbacks, never via imsgev dispatch, so it's a plain struct imsgbuf; see keymgr_forward_rsa()/keymgr_forward_ecdsa(). */ static struct imsgbuf keymgr_ibuf; static struct tls_config *listener_tls_config; @@ -161,28 +145,10 @@ static const struct imap_cmd_entry imap_cmds[] = { }; #define NUM_IMAP_CMDS (sizeof(imap_cmds) / sizeof(imap_cmds[0])) -/* - * tls_config_use_fake_private_key() is internal to lib/libtls, not - * declared in the installed tls.h -- confirmed directly (checked - * lib/libtls/tls.h, the only public libtls header in openbsd_source/): - * neither it nor tls_config_set_sign_cb() (tls_internal.h) appears - * there. smtpd's smtp.c (smtp.c:54) forward-declares it the same way; - * see docs/openimap-tls-privsep-design.md SS9 on what depending on an - * unexported, no-compatibility-promise libtls internal costs. - */ +/* tls_config_use_fake_private_key() is an internal, undeclared libtls symbol forward-declared here, same as smtpd's smtp.c does; see docs/openimap-tls-privsep-design.md SS9 on the risk of depending on it. */ void tls_config_use_fake_private_key(struct tls_config *); -/* - * SS5.4/SS10.2's fake-key call shape, exactly as smtp.c:187-193 uses it - * (Gilles Chehade, Pierre-Yves Ritschard, Jacek Masiulaniec -- see this - * file's header comment): tls_config_use_fake_private_key() installs - * libtls's placeholder key, which keymgr_engine_init()'s RSA_METHOD/ - * EC_KEY_METHOD override below then intercepts every operation on; - * tls_config_set_keypair_mem() supplies the real certificate with a NULL - * key. Wrapped in one function so the tls_config-building if/else-if - * chains in listener_main() (one call per branch) don't need - * restructuring around two calls. - */ +/* Installs libtls's placeholder private key plus the real certificate (smtp.c:187-193's call shape) in one function so listener_main()'s tls_config-building if/else-if chains need only one call per branch. */ static int keymgr_set_fake_keypair(struct tls_config *config, const char *cert_buf, size_t cert_len) @@ -192,34 +158,14 @@ keymgr_set_fake_keypair(struct tls_config *config, con cert_len, NULL, 0); } -/* - * RSA/ECDSA privsep engine: installed once, process-wide, so every RSA/ - * EC private-key operation OpenSSL performs against this process's - * "fake" key -- inside a TLS handshake, on whatever connection happens - * to be negotiating at the time -- is intercepted here and forwarded to - * keymgr over keymgr_ibuf instead. Adapted from smtpd's ca.c - * (ca.c:289-558, Reyk Floeter, Gilles Chehade -- see this file's header - * comment); ca.c's own m_*()-based imsg framing (smtpd's message- - * abstraction layer) is replaced with imapd's native "fixed header + - * trailing raw bytes on one imsg" convention, and the imsg id field - * imapd already uses elsewhere (e.g. IMSG_SETUP_PEER's session id) takes - * the place of ca.c's own separate reqid bookkeeping. - */ +/* RSA/ECDSA privsep engine, installed once process-wide: intercepts every private-key operation OpenSSL performs against this process's fake key and forwards it to keymgr; adapted from smtpd's ca.c (ca.c:289-558) with imapd's own imsg framing. */ static const RSA_METHOD *keymgr_rsa_default; static RSA_METHOD *keymgr_rsae_method; static const EC_KEY_METHOD *keymgr_ecdsa_default; static EC_KEY_METHOD *keymgr_ecdsae_method; -/* - * Blocks reading keymgr_ibuf directly, bypassing the event loop -- - * called synchronously from inside an OpenSSL RSA_METHOD callback, which - * cannot itself be deferred to the normal libevent dispatch. Unlike - * ca.c's rsae_send_imsg() (ca.c:293-369), there's no "some other imsg is - * queued up, hand it to the normal dispatcher" branch: nothing besides - * these three request/reply pairs is ever multiplexed on the listener - * <->keymgr channel, keymgr never sends anything unsolicited. - */ +/* Blocks reading keymgr_ibuf directly from inside an OpenSSL RSA_METHOD callback; unlike ca.c's rsae_send_imsg(), nothing else is ever multiplexed on this channel so there's no need to hand off unrelated imsgs. */ static int keymgr_forward_rsa(uint32_t type, const char *hash, const unsigned char *from, int fromlen, unsigned char *to, size_t tosize, int padding) @@ -286,17 +232,7 @@ keymgr_forward_rsa(uint32_t type, const char *hash, co imsg_free(&imsg); break; } - /* - * tosize, not KEYMGR_DATA_MAX: `to` is OpenSSL's own - * output buffer, which the caller sized RSA_size(rsa) - * -- 256 bytes for a 2048-bit key, where KEYMGR_DATA_MAX - * is 1024, sized for an 8192-bit one. Bounding by the - * wire cap rather than by the buffer we were actually - * handed would let a longer-than-expected reply overrun - * it. smtpd's ca.c sends RSA_size() across for exactly - * this reason (ca.c:319, m_add_size()); this restores - * that bound on the receiving side. - */ + /* Bound by tosize (OpenSSL's actual output buffer, e.g. 256 bytes for a 2048-bit key), not by the larger KEYMGR_DATA_MAX wire cap, or an oversized reply could overrun it; mirrors ca.c's own RSA_size() bound. */ if (rep.ok && rep.tolen <= tosize && imsg_get_len(&imsg) == rep.tolen) { if (imsg_get_buf(&imsg, to, rep.tolen) == -1) @@ -396,44 +332,66 @@ keymgr_forward_ecdsa(const char *hash, const unsigned return (sig); } +/* Checks A, B and C1 (docs/OpenIMAPD-TLS-Review/Opus-5-DESIGN-B3-implementation.md) before any key op is forwarded to keymgr; fatalx(), not a log line, since a failure here means privilege separation isn't actually in effect. */ +static void +keymgr_assert_fake_key(const char *hash, const BIGNUM *priv, const char *op) +{ + size_t i; + + /* Check A: a public-key-only object from tls_config_use_fake_private_key() never has d/priv_key set, so a non-NULL priv here means libtls is no longer using the placeholder key. */ + if (priv != NULL) + fatalx("%s: key object carries a private component -- " + "libtls is no longer using a placeholder key, and this " + "process is not separated from the TLS private key", op); + + /* Check B: unlike smtpd's ca.c, listener configures exactly one keypair and never reaches this callback for an unrelated key, so a missing pubkey-hash tag is itself the regression, not a benign case to fall through on. */ + if (hash == NULL) + fatalx("%s: no pubkey-hash tag on the key object -- libtls's " + "ex_data slot 0 tagging has changed", op); + + /* Check C1, done before the tag is read as a string: strlcpy(3) has no bound once the destination is full, so this bounded loop (not memchr/strnlen) confirms the tag is NUL-terminated within KEYMGR_HASH_MAX bytes before anything trusts it. */ + for (i = 0; i < KEYMGR_HASH_MAX; i++) + if (hash[i] == '\0') + return; + fatalx("%s: pubkey-hash tag is not a NUL-terminated string within " + "%d bytes -- refusing to read it", op, KEYMGR_HASH_MAX); +} + static int keymgr_rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding) { - char *hash; + const char *hash = RSA_get_ex_data(rsa, 0); - if ((hash = RSA_get_ex_data(rsa, 0)) != NULL) - return (keymgr_forward_rsa(IMSG_KEYMGR_RSA_PRIVENC, hash, - from, flen, to, (size_t)RSA_size(rsa), padding)); - return (RSA_meth_get_priv_enc(keymgr_rsa_default)(flen, from, to, - rsa, padding)); + keymgr_assert_fake_key(hash, RSA_get0_d(rsa), "RSA_PRIVENC"); + return (keymgr_forward_rsa(IMSG_KEYMGR_RSA_PRIVENC, hash, + from, flen, to, (size_t)RSA_size(rsa), padding)); } static int keymgr_rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding) { - char *hash; + const char *hash = RSA_get_ex_data(rsa, 0); - if ((hash = RSA_get_ex_data(rsa, 0)) != NULL) - return (keymgr_forward_rsa(IMSG_KEYMGR_RSA_PRIVDEC, hash, - from, flen, to, (size_t)RSA_size(rsa), padding)); - return (RSA_meth_get_priv_dec(keymgr_rsa_default)(flen, from, to, - rsa, padding)); + keymgr_assert_fake_key(hash, RSA_get0_d(rsa), "RSA_PRIVDEC"); + return (keymgr_forward_rsa(IMSG_KEYMGR_RSA_PRIVDEC, hash, + from, flen, to, (size_t)RSA_size(rsa), padding)); } static ECDSA_SIG * keymgr_ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv, const BIGNUM *rp, EC_KEY *eckey) { - ECDSA_SIG *(*psign_sig)(const unsigned char *, int, const BIGNUM *, - const BIGNUM *, EC_KEY *); - char *hash; + const char *hash = EC_KEY_get_ex_data(eckey, 0); - if ((hash = EC_KEY_get_ex_data(eckey, 0)) != NULL) - return (keymgr_forward_ecdsa(hash, dgst, dgst_len)); - EC_KEY_METHOD_get_sign(keymgr_ecdsa_default, NULL, NULL, &psign_sig); - return (psign_sig(dgst, dgst_len, inv, rp, eckey)); + /* inv/rp are ECDSA_sign_setup() precomputation, unused since keymgr performs the operation, not this process. */ + (void)inv; + (void)rp; + + keymgr_assert_fake_key(hash, EC_KEY_get0_private_key(eckey), + "ECDSA_SIGN"); + return (keymgr_forward_ecdsa(hash, dgst, dgst_len)); } static void @@ -510,22 +468,7 @@ listener_main(void) /* fd-passing is allowed on this channel: it receives fd-passed peer/session messages below; see imsgev_ibuf_init()'s own comment */ imsgev_ibuf_init(&ibuf3, 3); - /* - * SS7: this process is spawned fresh per connection (parent.c's - * spawn_connection()), not once at daemon boot, so the peer - * handshake is folded into the same synchronous drain loop as - * the rest of boot below rather than kept as separate blocking - * setup_recv_one_peer() calls. The auth peer in particular may - * never arrive at all -- spawn_connection() skips wiring one - * when the auth-worker itself failed to fork -- so it can't be - * a fixed, blocking "read exactly one" step the way it was when - * a listener process's whole boot depended on both peers - * existing. IMSG_SETUP_PEER is told apart by id: 0 for the auth - * peer, session_id (always >= 1) for the keymgr peer, matching - * parent.c's own setup_peer_send() calls. There is no - * IMSG_SETUP_DONE/ack step either -- see parent.c's header - * comment for why spawn_connection() doesn't use one. - */ + /* SS7: this process is spawned fresh per connection, so the peer handshake is drained in this same synchronous loop rather than separate blocking calls; the auth peer may never arrive, and IMSG_SETUP_PEER's id (0 vs session_id) tells auth from keymgr, matching parent.c's setup_peer_send(). */ while (!got_cert || !got_session_init || !got_keymgr_peer) { if ((n = imsgbuf_get(&ibuf3, &imsg)) == -1) fatal("imsgbuf_get"); @@ -547,19 +490,7 @@ listener_main(void) "carried no fd"); break; } - /* - * parent sends each of these exactly once, so a - * repeat cannot happen today -- but this loop runs - * before pledge(2) and before the privilege drop, - * which makes it the one place where "trust the - * parent" is doing the most work. State the - * invariant rather than silently overwriting a - * descriptor. imsg_get_fd(3) has already passed - * responsibility for peer_fd to us (imsg_init(3): - * only UNCLAIMED descriptors are closed by - * imsg_free()), so the duplicate must be closed - * here. - */ + /* A repeat can't happen today, but this runs pre-pledge/pre-privdrop where trusting the parent matters most, so state the invariant rather than silently overwrite; imsg_get_fd(3) already handed us peer_fd, so the duplicate must be closed here. */ if (id == 0) { if (auth_peer_fd != -1) { log_warnx("listener: duplicate auth " @@ -616,12 +547,7 @@ listener_main(void) log_warnx("bad IMSG_LISTENER_SESSION_INIT"); break; } - /* - * Refuse before claiming, unlike the two peer - * cases above: an fd left unclaimed on this imsg - * is closed by imsg_free() below (imsg_init(3)), - * so there is nothing to clean up by hand. - */ + /* Refuse before claiming, unlike the peer cases above: an unclaimed fd on this imsg is closed by imsg_free() below, so there's nothing to clean up by hand. */ if (client_fd != -1) { log_warnx("listener: duplicate " "IMSG_LISTENER_SESSION_INIT, ignoring"); @@ -702,13 +628,7 @@ listener_main(void) event_init(); - /* - * auth_peer_fd may be -1 (no auth-worker for this connection -- - * see this function's header comment); iev_auth.ibuf.fd is left - * at -1 in that case (its default zero-init would be fd 0, a - * real, misleading fd), and auth_cmd.c's sasl_plain_finish() - * checks that before ever composing to it. - */ + /* auth_peer_fd may be -1 (no auth-worker spawned); iev_auth.ibuf.fd is left at -1 rather than defaulting to fd 0, and auth_cmd.c's sasl_plain_finish() checks that before composing to it. */ if (auth_peer_fd != -1) imsgev_init(&iev_auth, auth_peer_fd, listener_dispatch_auth, NULL); @@ -719,14 +639,7 @@ listener_main(void) "connection gets one", sinit.session_id); } - /* - * SS8.1: search_peer_fd may be -1 (no search-oracle for this - * connection -- parent.c's spawn_connection() tolerates that fork - * failing independently of listener/auth's own); iev_search.ibuf.fd - * is left at -1 in that case, and search_cmd.c's search_dispatch() - * checks that before ever composing to it, mirroring iev_auth just - * above. - */ + /* SS8.1: search_peer_fd may be -1 (no search-oracle spawned, independent of auth's fork); iev_search.ibuf.fd is left at -1, checked by search_cmd.c's search_dispatch() before composing to it, mirroring iev_auth above. */ if (search_peer_fd != -1) imsgev_init(&iev_search, search_peer_fd, listener_dispatch_search, NULL); @@ -745,48 +658,13 @@ listener_main(void) imsgev_init_from_ibuf(&iev_parent, &ibuf3, listener_dispatch_parent, NULL); - /* - * This process's one and only session, built from what boot just - * drained above -- see listener_start_session()'s own comment. - * Must run after event_init()/the TLS setup block above: - * session_tls_start()/session_arm_client_read() register - * libevent events, and an implicit-TLS session needs - * listener_tls_ctx already set. - */ + /* This process's one and only session, built from what boot just drained; must run after event_init() and the TLS setup above since session_tls_start()/session_arm_client_read() register libevent events needing listener_tls_ctx already set. */ listener_idle_poll_secs = sinit.idle_poll_secs; listener_start_session(sinit.session_id, client_fd, sinit.implicit_tls, &sinit.remote_ss, sinit.remote_sslen); - /* - * SS8.1 finding: this process makes no socket(2)/connect(2)/ - * bind(2)/listen(2)/accept(2) call anywhere any more -- parent.c's - * spawn_connection() owns every one of those now (SS7), and this - * process only ever inherits an already-accepted client_fd over - * IMSG_LISTENER_SESSION_INIT. The one remaining candidate for - * needing "inet" is listener_start_session()'s getnameinfo(3) call - * just above, formatting s->remote_addr -- but NI_NUMERICHOST| - * NI_NUMERICSERV means it never touches the resolver or the - * network, just formats already-numeric address bytes already in - * hand (see that call's own comment). Dropping "inet" here on that - * basis; if this turns out wrong, pledge(2) will kill the process - * on its next getnameinfo(3) call and this line reverts. - * - * "recvfd" stays: this process receives a descriptor after this line, - * the store child's peer fd, arriving as IMSG_SETUP_PEER on the fd 3 - * channel once parent.c's store-fork handshake completes - * (listener_dispatch_parent()'s own case below). - * - * "sendfd" goes: this process never attaches a descriptor to an imsg. - * The parent is the only one in the tree that does - * (setup_peer_send(), setup_search_peer_send(), - * IMSG_LISTENER_SESSION_INIT -- five call sites, all in parent.c). - * SYS_sendmsg is PLEDGE_STDIO (sys/kern/kern_pledge.c); "sendfd" is - * checked in unp_internalize() (sys/kern/uipc_usrreq.c), which the - * kernel reaches only when SCM_RIGHTS is actually attached, so an - * imsgbuf_allow_fdpass() channel carrying only fd == -1 messages does - * not need the promise. - */ + /* pledge(2) promises: no socket/connect/bind/listen/accept call remains here (SS7 moved them to parent.c), so "inet" is dropped since getnameinfo(3) below only formats already-numeric bytes; "recvfd" stays for the store child's peer fd arriving later; "sendfd" goes since this process never attaches a descriptor to an imsg. */ #ifdef __OpenBSD__ if (pledge("stdio recvfd", NULL) == -1) fatal("pledge"); @@ -796,21 +674,7 @@ listener_main(void) fatalx("listener: exited event loop"); } -/* - * Builds and starts this process's one and only session, from the - * IMSG_LISTENER_SESSION_INIT payload listener_main() drained at boot - * (SS7: parent.c's spawn_connection() forks one listener-worker per - * accepted connection instead of a single long-lived listener - * accept()ing every one itself; MaxStartups admission control moved - * with it, checked in parent.c's parent_accept() before this process - * even exists -- see parent.c's own count_startups()/ - * startups_should_drop(), moved there verbatim from what used to - * live in this file). Does the same work the old accept()-driven - * listener_accept() did once accept(2) returned: construct struct - * session, format remote_addr, log, and either begin the TLS - * handshake or send the plaintext greeting -- just once, since - * there's only ever one connection for this process to serve. - */ +/* Builds and starts this process's one and only session from the IMSG_LISTENER_SESSION_INIT payload drained at boot (SS7), doing what the old accept()-driven listener_accept() did: build struct session, format remote_addr, log, then begin the TLS handshake or send the plaintext greeting. */ static void listener_start_session(uint32_t session_id, int client_fd, int implicit_tls, const struct sockaddr_storage *ss, socklen_t sslen) @@ -821,18 +685,7 @@ listener_start_session(uint32_t session_id, int client if (s == NULL) { log_warn("calloc"); close(client_fd); - /* - * SS7: nothing else will ever run in this process -- it - * exists to serve this one session, and this is the only - * call to this function. Returning would park it in - * event_dispatch() forever with no client, holding a - * parent-side open_session entry that counts against - * MaxStartups for the rest of the daemon's uptime. - * session_teardown()'s exit(0) is unreachable from here - * (there is no session to tear down), so exit directly; - * the implicit-TLS branch just below reaches the same - * outcome through session_teardown(). - */ + /* Exit directly rather than return: nothing else will ever run in this process, and returning would park it in event_dispatch() forever holding a MaxStartups slot with no client and no session to tear down. */ exit(1); } session_idle_poll_init(s); /* before anything can tear s down */ @@ -845,14 +698,7 @@ listener_start_session(uint32_t session_id, int client { char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV]; - /* - * NI_NUMERIC*: no resolver call, no network I/O of any - * kind -- pure formatting of the already-numeric address - * bytes in ss/sslen. This is the fact listener_main()'s - * pledge() comment (SS8.1) rests dropping "inet" on; if - * that turns out wrong, this call is where pledge(2) - * would kill the process. - */ + /* NI_NUMERIC*: pure formatting of already-numeric address bytes, no resolver or network I/O -- the fact listener_main()'s pledge() comment rests dropping "inet" on. */ if (getnameinfo((const struct sockaddr *)ss, sslen, hbuf, sizeof(hbuf), sbuf, sizeof(sbuf), NI_NUMERICHOST | NI_NUMERICSERV) == 0) @@ -974,14 +820,7 @@ static int session_enqueue_cmd(struct session *, const /* RFC 9051 SS4.3 hard cap on a non-synchronizing literal. */ #define IMAP_NONSYNC_LITERAL_MAX 4096 -/* - * True if `line` (CRLF already stripped) ends in a NON-synchronizing - * literal announcement, "{n+}" (RFC 9051 SS4.3). Those octets are already - * in flight when the line arrives -- unlike a synchronizing "{n}", whose - * octets only follow our "+" continuation -- so the reader has to account - * for them no matter what becomes of the command that announced them. - * Octet count returned in *lenp; 0 is a legal announcement ("{0+}"). - */ +/* True if `line` ends in a non-synchronizing literal announcement "{n+}" (RFC 9051 SS4.3), whose octets are already in flight and must be accounted for regardless of the command's fate; octet count returned in *lenp. */ static int line_nonsync_literal(const char *line, uint64_t *lenp) { @@ -1012,13 +851,7 @@ line_nonsync_literal(const char *line, uint64_t *lenp) return (1); } -/* - * RFC 9051 SS9: tag = 1*, i.e. no CTLs, no SP, no - * 8-bit, and none of ( ) { % * DQUOTE backslash. Only the length was - * checked before, so any other byte reached session_reply()'s "%s %s %s" - * verbatim; a tag of "+" in particular turns our own reply into what the - * client reads as a command continuation request. - */ +/* RFC 9051 SS9: tag = 1*; previously only length was checked, so a tag of "+" could turn session_reply()'s own reply into a command continuation request. */ static int tag_is_valid(const char *tag) { @@ -1042,11 +875,7 @@ session_dispatch_client(int fd, short event, void *arg struct session *s = arg; ssize_t n; char *crlf; - size_t tls_want = 0; /* bytes offered to tls_read(); see the - * re-arm at the end of this function. - * Not "want": the literal-assembly loop - * below has its own uint64_t want, and - * shadowing it draws -Wshadow. */ + size_t tls_want = 0; /* bytes offered to tls_read(); re-armed at the end of this function -- named tls_want, not want, to avoid shadowing the literal-assembly loop's own uint64_t want (-Wshadow). */ (void)event; @@ -1101,13 +930,7 @@ session_dispatch_client(int fd, short event, void *arg size_t consumed, linelen; int alive; - /* - * Octets of a non-synchronizing literal whose command was - * refused (bad syntax, over the size cap, arrived while the - * session was busy, or simply wasn't APPEND). They are on - * the wire regardless, so swallow them rather than let the - * message body be parsed as further IMAP commands. - */ + /* Octets of a refused non-synchronizing literal (bad syntax, over cap, session busy, or not APPEND) are already on the wire, so swallow them instead of parsing the message body as further IMAP commands. */ if (s->literal_discard > 0) { uint64_t take; @@ -1121,17 +944,7 @@ session_dispatch_client(int fd, short event, void *arg } if (s->literal_discard > 0) break; /* need more data */ - /* - * The command line that announced this literal - * still ends in CRLF (RFC 9051 SS9). Nothing - * downstream wants it, and leaving it makes the - * line parser below see a zero-length line and - * answer "* BAD Empty command line" after every - * refused literal. Deliberately tolerant: if what - * follows is not CRLF then the command had more - * arguments after the literal, and the line parser - * should still get a crack at them. - */ + /* Swallow the announcing command line's trailing CRLF so the line parser doesn't see a spurious zero-length line and answer "* BAD Empty command line" after every refused literal; anything besides CRLF is left for the parser. */ if (s->inbuflen >= 2 && s->inbuf[0] == '\r' && s->inbuf[1] == '\n') { memmove(s->inbuf, s->inbuf + 2, @@ -1189,17 +1002,7 @@ session_dispatch_client(int fd, short event, void *arg linelen = (size_t)(crlf - s->inbuf); consumed = linelen + 2; - /* - * RFC 9051 SS2.2/SS9: CR and LF appear in a command only as the - * CRLF terminator, and NUL is not an ASTRING-CHAR. This is the - * one chokepoint where that can be enforced, and enforcing it - * here is what keeps every downstream site that echoes client - * text back (FETCH's BODY[