diff options
| author | Owen Jacobson <owen@grimoire.ca> | 2025-07-18 00:08:39 -0400 |
|---|---|---|
| committer | Owen Jacobson <owen@grimoire.ca> | 2025-07-18 01:21:42 -0400 |
| commit | dc240ca270f86552e999c81d864b4cb0c687a88e (patch) | |
| tree | 191b55b0213219d9a3499afc8de8a84d2ef9816d | |
| parent | 9ad439d16e797d04804b5d80706fd0e86041b161 (diff) | |
Add a `--umask` option to determine what permissions new files/databases get.
The new `--umask` option takes one of three values:
* `--umask masked`, the default, takes the inherited umask and forces o+rwx on.
* `--umask inherit` takes the inherited umask as-is.
* `--umask OCTAL` sets the umask to exactly `OCTAL` and is broadly equivalent to `umask OCTAL && pilcrow --umask inherit`.
This fell out of a conversation with @wlonk, who is working on notifications. Since notifications may require [VAPID] keys, the server will need a way to store those keys. That would generally be "in the pilcrow database," which lead me to the observation that Pilcrow creates that database as world-readable by default. "World-readable" and "encryption/signing keys" are not things that belong in the same sentence.
[VAPID]: https://datatracker.ietf.org/doc/html/rfc8292
The most "obvious" solution would be to set the permissions used for the sqlite database when it's created. That's harder than it sounds: sqlite has no built-in facility for doing this. The closest thing that exists today is the [`modeof`] query parameter, which copies the permissions (and ownership) from some other file. We also can't reliably set the permissions ourselves, as sqlite may - depending on build options and configuration - [create multiple files][wal].
[`modeof`]: https://www.sqlite.org/uri.html
[wal]: https://www.sqlite.org/wal.html
Using `umask` is a whole-process solution to this. As Pilcrow doesn't attempt to create other files, there's little issue with doing it this way, but this is a design risk for future work if it creates files that are _intended_ to be readable by more than just the Pilcrow daemon user.
| -rw-r--r-- | Cargo.lock | 23 | ||||
| -rw-r--r-- | Cargo.toml | 1 | ||||
| -rw-r--r-- | docs/ops.md | 12 | ||||
| -rw-r--r-- | src/cli.rs | 17 | ||||
| -rw-r--r-- | src/lib.rs | 1 | ||||
| -rw-r--r-- | src/umask.rs | 116 |
6 files changed, 166 insertions, 4 deletions
@@ -301,6 +301,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" [[package]] +name = "cfg_aliases" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "613afe47fcd5fac7ccf1db93babcb082c5994d996f20b8b159f2ad1658eb5724" + +[[package]] name = "chrono" version = "0.4.39" source = "registry+https://github.com/rust-lang/crates.io-index" @@ -1167,9 +1173,9 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.169" +version = "0.2.174" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b5aba8db14291edd000dfcc4d620c7ebfb122c613afb886ca8803fa4e128a20a" +checksum = "1171693293099992e19cddea4e8b849964e9846f4acee11b3948bcc337be8776" [[package]] name = "libm" @@ -1275,6 +1281,18 @@ dependencies = [ ] [[package]] +name = "nix" +version = "0.30.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "74523f3a35e05aba87a1d978330aef40f67b0304ac79c1c00b294c9830543db6" +dependencies = [ + "bitflags", + "cfg-if", + "cfg_aliases", + "libc", +] + +[[package]] name = "num-bigint-dig" version = "0.8.4" source = "registry+https://github.com/rust-lang/crates.io-index" @@ -1418,6 +1436,7 @@ dependencies = [ "hex-literal", "itertools", "mime", + "nix", "password-hash", "pin-project", "rand", @@ -32,6 +32,7 @@ headers = "0.4.0" hex-literal = "0.4.1" itertools = "0.14.0" mime = "0.3.17" +nix = { version = "0.30.1", features = ["fs"] } password-hash = { version = "0.5.0", features = ["std"] } rand = "0.8.5" rand_core = { version = "0.6.4", features = ["getrandom"] } diff --git a/docs/ops.md b/docs/ops.md index 4dfec49..a622c04 100644 --- a/docs/ops.md +++ b/docs/ops.md @@ -9,3 +9,15 @@ To avoid destroying backups that may still be needed, `pilcrow` will not start i - Start the server with **a copy** of the backup database, and determine if any data has been lost. If not, shut it down, replace your main database by copying the backup, and carry on. The `pilcrow` database is an ordinary file. While the server is not running, it can be freely copied or renamed without invalidating the data in it. + +## Permissions + +The Pilcrow database (`pilcrow.db` by default) should have restricted permissions, as it contains data that is either confidential (such as users' conversations), sensitive (such as the service's configuration), or both. At minimum, it should not be world-readable on the server where Pilcrow is installed. + +By default, the `pilcrow` command will set the process' umask to a value that prevents the creation of world-readable or world-writeable files, so that the Pilcrow database will be created with appropriate filesystem permissions. This behaviour can be controlled using the `--umask` startup option, which takes the following values: + +- `masked`, the default, which takes the inherited umask and additionally sets the world-readable, world-writeable, and world-executable bits; +- `inherit`, which takes the inherited umask as-is, requiring the operator to set a sensible value here; or +- any octal value corresponding to a valid umask, such as `0027`. + +Pilcrow does not check or change the permissions of the database after creation. Changing the umask of the server after the database has been created has no effect on the database's filesystem permissions. @@ -3,7 +3,7 @@ //! This module supports running `pilcrow` as a freestanding program, via the //! [`Args`] struct. -use std::{future, io}; +use std::{future, io, str}; use axum::{ http::header, @@ -14,7 +14,11 @@ use clap::{CommandFactory, Parser}; use sqlx::sqlite::SqlitePool; use tokio::net; -use crate::{app::App, clock, db, routes}; +use crate::{ + app::App, + clock, db, routes, + umask::{self, Umask}, +}; /// Command-line entry point for running the `pilcrow` server. /// @@ -51,6 +55,10 @@ pub struct Args { #[arg(short, long, env, default_value_t = 64209)] port: u16, + /// The umask pilcrow should run under (octal, `inherit`, or `masked`) + #[arg(short = 'U', long, default_value_t = Umask::Masked)] + umask: Umask, + /// Sqlite URL or path for the `pilcrow` database #[arg(short, long, env, default_value = "sqlite://pilcrow.db")] database_url: String, @@ -66,6 +74,7 @@ impl Args { /// /// This will perform the following tasks: /// + /// * Set the process' umask (as specified by `--umask`). /// * Migrate the `pilcrow` database (at `--database-url`). /// * Start an HTTP server (on the interface and port controlled by /// `--address` and `--port`). @@ -78,6 +87,8 @@ impl Args { /// prematurely. The specific [`Error`] variant will expose the cause /// of the failure. pub async fn run(self) -> Result<(), Error> { + self.umask.set(); + let pool = self.pool().await?; let app = App::from(pool); @@ -135,4 +146,6 @@ pub enum Error { Io(#[from] io::Error), /// Failure due to a database initialization error. See [`db::Error`]. Database(#[from] db::Error), + /// Failure due to invalid umask-related options. + Umask(#[from] umask::Error), } @@ -23,4 +23,5 @@ mod setup; mod test; mod token; mod ui; +mod umask; mod user; diff --git a/src/umask.rs b/src/umask.rs new file mode 100644 index 0000000..32a82ad --- /dev/null +++ b/src/umask.rs @@ -0,0 +1,116 @@ +use std::{fmt, str}; + +use nix::{ + libc::mode_t, + sys::stat::{self, Mode}, +}; + +#[derive(Clone, Copy)] +pub enum Umask { + Masked, + Inherit, + Set(Mode), +} + +impl Umask { + pub fn set(self) { + match self { + Self::Masked => Self::masked(), + Self::Inherit => Self::inherit(), + Self::Set(mode) => Self::set_to(mode), + } + } + + fn masked() { + // In the year 2025, this is _still_ the only reasonable way to check the + // process' current umask. This is also why we don't calculate a default + // umask and stick it in Clap's `default_value_t` - I'd prefer not to touch + // the process' umask before we know what we're doing with it, but there's + // no other way to look at the current one. + // + // The choice of `all` here is a complicated compromise. In theory, nothing + // will ever observe this umask, as we immediately overwrite it immediately + // below. As of this writing, there is nothing in this service that could + // create a file in this interval, which could then be affected by the + // temporary umask. However, future code changes _could_ introduce something + // that races with this, such as a signal handler that does more than just + // exiting the process. + // + // Using `all` makes sure that _any_ file created in that interval is + // created with "deny all" file permission bits, which will make it useless + // even for this program. Hopefully, that failure will be immediate enough + // to attract attention. The other alternatives which might work are zero + // (don't mask off any permissions, things might be globally readable) + // or 0o0027 (the permissions we'd want to use on most Linux distros, which + // could then silently hide the above race condition). + let current = stat::umask(Mode::all()); + + // In addition to whatever we inherit, by default, we ensure that files + // created by pilcrow are not world-readable or world-writeable. However, + // we respect the inherited group and user permissions on the assumption + // that the user, or their administrator, has either made a decision about + // group permissions, or are relying on the distro's defaults. + // + // The main thing `pilcrow` creates are its database and its backup + // database, which can contain both confidential information (users' + // conversations) and sensitive information (the service's configuration + // and any API tokens, keys, &c used by the service). + // + // There is no way to tell `sqlite` to restrict the permissions of database + // files directly, which is otherwise what we'd prefer. + let desired = current | Mode::S_IRWXO; + + stat::umask(desired); + } + + fn inherit() { + // Here's a complete list of steps required to inherit the umask from the calling process: + } + + fn set_to(mode: Mode) { + stat::umask(mode); + } +} + +impl str::FromStr for Umask { + type Err = Error; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + let umask = match s { + "masked" => Self::Masked, + "inherit" => Self::Inherit, + octal => { + let mode = mode_t::from_str_radix(octal, 8)?; + let mode = Mode::from_bits(mode).ok_or(Error::UnknownBits)?; + Self::Set(mode) + } + }; + + Ok(umask) + } +} + +impl fmt::Display for Umask { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Masked => "masked".fmt(f), + Self::Inherit => "inherit".fmt(f), + Self::Set(mode) => write!(f, "{mode:o}"), + } + } +} + +/// Errors occurring during umask option parsing. +#[derive(Debug, thiserror::Error)] +pub enum Error { + /// Failed to parse a umask value from the input. + #[error(transparent)] + Parse(#[from] std::num::ParseIntError), + + /// The provided umask contained invalid bits. (See the constants associated with [`Mode`] for + /// valid umask bits.) + // We dont need to hold onto the actual umask value here - Clap does that for us, and prints + // the value as the user input it, which beats anything we could do here. + #[error("unknown bits in umask")] + UnknownBits, +} |
