Problems with source tampering resistance

[khanhtranngoccva]

I'm trying to develop the ability of easy_fuser to resist upstream file or directory name tampering. This would happen if the source directory is exposed to the user, or is mounted somewhere else as a bind mount, usually from a user's attempts to rename an existing directory or file inode.

So far, it is slightly easier for the FUSE filesystem to deal with a tampered file if TTL=0 (which is what I use in my application to ensure attributes are not cached and will appear differently per process) and I am opening the file by path, because there is always a lookup call before anything else happens.

However, when TTL != 0 or when opening a persistent directory handle using a path like /fs/path/to/A (which resolves to FUSE inode 3, for example), and the directory or file is renamed to something like /fs/path/to/B, then any further opens related to inode 3 or descendants of inode 3 (including lookup on inode 3) will fail because /fs/path/to/B is not recognized by the mapper yet.

I just wrote a test case on my end-user application to simulate this scenario, and the test, which fails with the current implementation of InodeMultiMapper is as follows:

#[test_log::test(rstest)]
fn test_should_open_inner_file_by_directory_ref_when_directory_name_is_tampered(
    #[from(fixtures::globals)] _globals: &(),
) -> Result<(), anyhow::Error> {
    let test_dir = fixtures::generate_test_dir();
    let mounted_file_system = fixtures::file_system(test_dir.path());
    let target_directory_path = mounted_file_system.target_dir.join("path/to/test_dir");
    fs::create_dir_all(&target_directory_path).expect("should create directory");

    // Stable reference to the target directory
    let target_dir = Dir::open(&target_directory_path).expect("should open target directory");
    let mut inner_file = target_dir
        .write_file("test.txt", 0o644)
        .expect("should create file in target directory");
    inner_file
        .write_all(b"Hello, world!")
        .expect("should write to test file");
    drop(inner_file);

    // Rename the target directory on the source filesystem
    let target_directory_source_path = mounted_file_system.source_dir.join("path/to/test_dir");
    let target_directory_new_source_path = mounted_file_system.source_dir.join("path/to/test_dir_new");
    std::fs::rename(
        &target_directory_source_path,
        &target_directory_new_source_path,
    )
    .expect("should rename directory");

    // The FUSE implementation should try to open file using the `target_dir` handle as a fallback
    let mut inner_file = target_dir
        .open_file("test.txt")
        .expect("should open test file using the directory handle");
    let mut buffer = Vec::new();
    inner_file
        .read_to_end(&mut buffer)
        .expect("should read from test file");
    assert_eq!(buffer, b"Hello, world!");
    drop(inner_file);
    Ok(())
}

My idea is to let the user define a custom cloneable BackingHandle structure (usually an Arc of an underlying type) or another cloneable data structure that they can return alongside the backing ID and the metadata. This structure is then stored in the mapper, and can be retrieved from a method like backing_handle() in the HybridId API.
The HybridId would take another generic parameter so that it is now HybridId<BackingId, BackingHandle>, and backing_handle() shall return Option<BackingHandle>. The concern is that a lot of mapper internals may have to be altered in ways that I have not figured out yet, for example, the FileIdType trait has to add another trait like type BackingHandle = Option<BackingHandle>.

Depending on application, the user can try to exhaust the available paths that the mapper supplies first, then fall back to atomic operations with BackingHandle. Alternatively, the user may exclusively rely on BackingHandle and ignore the path-based APIs.

[Alogani]

Ok, i likely won't support your idea. Because it modifies the behaviour in a non common way (and add complexity).

When looking at other solutions like bindfs, they can suffer the same problem and using TTL=0 resolve it. So for me that is not really a problem.

Other solutions

Using inotify

i don't oppose the addition of a thread using inotify on source directory. It must own the inode_mapper with a lock and must add/remove inotify for all new directories registered in InodeMapperTrait. inotify syscalls speed should be monitored to see if this is viable

Multiple target mountpoint directory

The possibility to have multiple target directory : it would resolve somewhat the problem because you would have different multiple entry point to modify the same source file (which has similarity with what you want).

• They won't share the same inode mapper.
• When fuse action happens that modify the system it would reflect on other filesystem by triggerig the same fuse operations in those other systems
• It should be implemented using Publish-Subscribe pattern (using channels) that is checked before each fuse_operations (non blocking)
• However the complication is defining the MessageType (both what function to call and with what arguments need to be passed)

=> It will be in long term feature goals (depending on my time) to support this multiple mountpoint feature

[khanhtranngoccva]

With TTL = 0, any path-based file API works normally on my current set of tests. However, my goal is to ensure the filesystem still works as much as possible if the user or an advanced application is doing unexpected things with the upstream (underlying) file tree like renaming (which the FUSE handlers cannot be notified about), even if the capacity is severely reduced because inferring paths might be difficult or impossible altogether. I did experiment with such a change this afternoon, and it vaguely works like this:

1. The HybridResolver not only manages a reference count, but also a generic Context field, which is guarded by a parking_lot::RwLock and starts at None.
2. The user must specify an optional boxed callback that modifies the inode's Context lock. The user can forego modifying the variable by using None as that return value.
3. The Context field stores the user-defined handle.

There are indeed a few problems with this approach:

• To ensure the best performance for multiple threads contending on an inode context field and avoid a race condition where a context is created twice, we are forced to unconditionally introduce the parking_lot::RwLock dependency, which lets the user take advantage of the rather contrived upgradable_read() API. Otherwise, checking would have to be done in the write phase instead of being done in parallel with other reads, which slows throughput. The signature for the callback would be

type ContextUpdater<Context> = Box<dyn FnOnce(&RwLock<Option<Context>>) -> Result<(), io::Error>>

• A lot of internal implementation has to be altered. For example, since the data field is managed at the FileIdResolver level but the ContextUpdater callback is called by the underlying it, making ContextUpdater fallible means that the InodeMultiMapper becomes more tightly coupled with the FileIdResolvers and forcing them to change the implementation. So far, the lookup method has to add a ContextUpdater parameter and return an optional error code.

[Alogani]

But how would the user know that source file system is modified ? Apart of using inotify ?

In either case, i don't like modifying inode_mapping more, but a publish-subscribe pattern :

• Split FuseDriver in two : one part in charge of making the calls to resolver, another in charge of calling FuseHandler
• If the user wants to make a change directly, it calls the publisher struct in the same way it would stimulate a fuse operation (rename, unlink, etc), and the struct will delegate to FuseDriver apart for the specific part related to calling FuseHandler
• make all functions of FuseHandler accept an supplementary arg call called dry_run to inform when this pattern is used to avoid making a real change

So simple, no big change in API, and will allow multi mount point easily !

[khanhtranngoccva]

I realized that even making such an interface can really complicate how users should integrate the crate. Because there is a second fallible callback that the user may supply in certain handlers, these callbacks will have to roll back all changes made in the first place by the handlers that supply them; otherwise, there will be resource and memory leaks.

Now I think the simpler solution is that the handlers can make a custom InodeContextTable field containing whatever data the filesystem wants.

By the way, I would like to refactor so that using an Arc reference of HybridResolver instead of InodeMultiMapper in HybridId, and use implement path resolution and backing ID methods there instead.

[Alogani]

This is for now theorical, i need some example usages of the final user, even with pseudo code to before being able to approve a custom InodeContextTable.

For my proposition, this would look like this :

let fuse_session = spawn_mount(...)
fuse_session:dummy_ops::rename(fuse_args)
// Which would delegate calls to a "dummy" fuse_driver, which would in turn call)
FuseHandler::rename(fuse_args, dry_run=True)

I find it pretty simple for end user, no complex concepts, dry_run will always be false apart when :

• end user calls a dummy operations
• there are multiple different mountpoints and we need to repercutate it inside different handlers (need to be refined to see feasibility and implementation details)

As for InodeMultiMapper, for now you are free to make any changes that pleases you. However i think one day i will look closer at it too see if it can be merged with current InodeMapper implementation with generics or traits and simplified.

[khanhtranngoccva]

I do not think that the InodeContextTable in question has to be adapted in the library code - since the previous updates already introduced deterministic file IDs and inodes. All the user really has to do is to query this file ID, and then use this key to make a reference-counted file context entry of their own in 6 methods create, mkdir, mknod, lookup, readdir, and readdirplus. After that, the HybridId has a stable backing_id property, which we can immediately use to retrieve this context. And finally, the reference count can be decremented in the forget handler

[khanhtranngoccva]

@Alogani

Two more down-the-line issues with my hybrid implementation that I discovered:

1. the file ID supplied can no longer be trivially creatable, unlike other implementations of IDs. It might be convenient to library users at first, but may impede unit testing because the user needs to pass a resolver reference to it to even create a file ID.

To solve this problem, the resolver needs to be decoupled from the ID, while the ID itself should have minimal data attached to it (just a single number). We can then give an Arc of TResolver to the user in a specific method called setup or similar, which is called before the actual mount operation takes place (before init). The user can then store this resolver and call methods like all_paths(id: HybridId) at any point. It also allows us to encapsulate path resolution code of the HybridId inside inode_mapping.

2. To ensure a FS's accuracy when dealing with underlying change, an inode needs to be bound to a stable file descriptor (preferably with the O_PATH flag), including the root inode 1.

My in progress FS application has a table that binds a stable file ID to reference counted context data including the file descriptor, while the library aims to bind inode numbers with the stable file IDs.

Correct lookups from the root inode (1) therefore needs the stable backing file ID and a live fd to the underlying directory, but the current implementation offers no opportunity to insert a stable ID or a valid FD for this inode.

I propose an optional lookup_root that works exclusively for inode 1. This method expects a metadata object from the user similar to normal lookup, and will set the backing_id property for inode 1. It is more proactive - it also runs before the mount takes place so that any failure will prevent the FS from being mounted by fuser at all. It is an optional method - there should be a warning message if it is not implemented.

Let me know what you think about these proposals.