Skip to content

Refuse shared storage directories and don't follow symlinks - #1179

Draft
ajaysehwal wants to merge 1 commit into
lightningdevkit:mainfrom
ajaysehwal:fix-storage-dir-hardening
Draft

ajaysehwal wants to merge 1 commit into
lightningdevkit:mainfrom
ajaysehwal:fix-storage-dir-hardening

Conversation

@ajaysehwal

Copy link
Copy Markdown
Contributor

By default, we store all node data, including the ChannelMonitors, under /tmp/ldk_node. create_dir_all_private accepts a directory that already exists as-is, without looking at who owns it or who can write to it. If another
local user pre-creates the directory (e.g., mkdir -m 0777 /tmp/ldk_node) before our first start, they can read our state, replace or roll back the database, or plant a symlink (e.g., ldk_node.log -> ~/.bashrc) so that we append log lines
to a file of their choosing.

Here, we harden how we handle the storage directory:

  • Add create_dir_all_private_checked, which on Unix additionally refuses to use an existing directory if it is owned by a user other than the current user or root, or if it is writable by all users (mode & 0o002). It's used for the configured storage_dir_path when building the SQLite- and fs-store-based Node, and for the default log directory. Failures surface as BuildError::StoragePathAccessFailed, with the cause logged.
  • Open the log file with O_NOFOLLOW, and refuse to open a SQLite database file that is a symlink.
  • Document that the default location is only suitable for testing, and set a storage directory in the README and crate-level example.

Notes for reviewers

  • Compatibility. This is a behavior change, noted in CHANGELOG.md. Group-writable directories (e.g., from fsGroup) and 0755 directories from older versions are still accepted. Directories that are world-writable, or owned by another non-root user, are now rejected. The main case I'm aware of that this could break is a root-owned 0777 directory used by a non-root process, such as a Kubernetes emptyDir. If that's a concern, I can relax the world-writable check to apply only when the directory isn't owned by us.
  • Default unchanged. The default remains /tmp/ldk_node. Moving it or refusing it on mainnet would be an API change, so I left that for a separate discussion.
  • Scope. The check only applies to the configured storage directory, not to arbitrary user-supplied log or seed paths (their parents may legitimately be shared directories such as /tmp or /var/log), and it doesn't change the
    VSS/Postgres setup. It is also racy by nature (check vs. use), but the check closes the realistic pre-created-directory case.
  • No new dependencies: libc was already a direct dependency (used for geteuid).

Testing

  • New unit tests: shared-directory rejection in create_dir_all_private_checked (world-writable rejected, group-writable and own directories accepted), a symlinked SQLite database, a symlinked log file, and Builder::build refusing a world-writable storage directory.
  • cargo test --lib passes (336 tests). I did not run the integration tests, and I haven't built with the uniffi feature.

AI disclosure: this change was prepared with the assistance of AI tools (Claude Code).

By default we store all node data, including the `ChannelMonitor`s, under
`/tmp/ldk_node`. If that directory had been pre-created by another local
user, we'd happily use it (`create_dir_all_private` accepts existing
directories as-is), allowing them to read, replace or roll back our state,
or to redirect our log or database file via a symlink.

Here, we add `create_dir_all_private_checked`, which on Unix additionally
refuses to use an existing directory if it is owned by a user other than
us or `root`, or if it is writable by all users. We use it for the
configured storage directory when building the SQLite- and fs-store-based
`Node`, and for the default log directory. Further, we open the log file
with `O_NOFOLLOW` and refuse to open a symlinked SQLite database file.

We also document that the default location is only suitable for testing
and add `set_storage_dir_path` to the README/crate-level example.

This change was prepared with the assistance of AI tools (Claude Code).
@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants