Repository navigation
Refuse shared storage directories and don't follow symlinks - #1179
Open
ajaysehwal wants to merge 1 commit into
Open
ajaysehwal wants to merge 1 commit into
ajaysehwal wants to merge 1 commit into
Conversation
|
I've assigned @tnull as a reviewer! |
ajaysehwal
force-pushed
the
fix-storage-dir-hardening
branch
2 times, most recently
from
October 20, 2026 15:52
a4c34f6 to
6f661e4
Compare
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
force-pushed
the
fix-storage-dir-hardening
branch
from
October 20, 2026 16:04
6f661e4 to
e15d123
Compare
ajaysehwal
marked this pull request as ready for review
October 20, 2026 16:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
By default, we store all node data, including the
ChannelMonitors, under/tmp/ldk_node.create_dir_all_privateaccepts 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:
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 orroot, or if it is writable by all users (mode & 0o002). It's used for the configuredstorage_dir_pathwhen building the SQLite- and fs-store-basedNode, and for the default log directory. Failures surface asBuildError::StoragePathAccessFailed, with the cause logged.ldk_node.login the storage directory) withO_NOFOLLOW, and refuse to open a SQLite database file that is a symlink. Explicitly configured log file paths still follow symlinks, as before.Notes for reviewers
fsGroup) and0755directories 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-owned0777directory used by a non-root process, such as a KubernetesemptyDir. If that's a concern, I can relax the world-writable check to apply only when the directory isn't owned by us./tmp/ldk_node. Moving it or refusing it on mainnet would be an API change, so I left that for a separate discussion./tmpor/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.O_NOFOLLOWis 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'sexpecton every write.unsafe. This adds a singleunsafe { libc::geteuid() }call (it can't fail). No new dependencies:libcwas already a direct dependency.Testing
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), andBuilder::buildrefusing a world-writable storage directory.cargo test --libpasses (340 tests), andcargo check --libcompiles with theuniffifeature and withstorage-sqlitedisabled. I did not run the integration tests.AI disclosure: this change was prepared with the assistance of AI tools (Claude Code).