Skip to content

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

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

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

Conversation

@ajaysehwal

@ajaysehwal ajaysehwal commented Oct 18, 2026 •

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 default log file (ldk_node.log in the storage directory) with O_NOFOLLOW, and refuse to open a SQLite database file that is a symlink. Explicitly configured log file paths still follow symlinks, as before.
  • 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. 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.
  • Symlinks. O_NOFOLLOW is only applied to the default log file. For explicitly configured log paths, we keep following symlinks, as users may legitimately symlink their log file, and failing there would also hit the logger's expect on every write.
  • unsafe. This adds a single unsafe { libc::geteuid() } call (it can't fail). No new dependencies: libc was already a direct dependency.

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 default log file (rejected) vs. an explicitly configured one (accepted), and Builder::build refusing a world-writable storage directory.
  • cargo test --lib passes (340 tests), and cargo check --lib compiles with the uniffi feature and with storage-sqlite disabled. I did not run the integration tests.

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

@ldk-reviews-bot

ldk-reviews-bot commented Oct 18, 2026 •

Copy link
Copy Markdown

I've assigned @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ajaysehwal
ajaysehwal force-pushed the fix-storage-dir-hardening branch 2 times, most recently from a4c34f6 to 6f661e4 Compare October 20, 2026 15:52
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 default
log file (in the storage directory) with `O_NOFOLLOW` and refuse to open a
symlinked SQLite database file. Explicitly configured log file paths still
follow symlinks, as before.

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).
@ajaysehwal
ajaysehwal force-pushed the fix-storage-dir-hardening branch from 6f661e4 to e15d123 Compare October 20, 2026 16:04
@ajaysehwal
ajaysehwal marked this pull request as ready for review October 20, 2026 16:10
@ldk-reviews-bot
ldk-reviews-bot requested a review from tnull October 20, 2026 16:11
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