On Fri, Sep 01, 2023 at 03:35:59PM +0200, Günther Noack wrote: > Hello! > > On Fri, Aug 25, 2023 at 07:07:01PM +0200, Mickaël Salaün wrote: > > On Fri, Aug 25, 2023 at 05:51:09PM +0200, Günther Noack wrote: > > > Hello! > > > > > > On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote: > > > > On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote: > > > > > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate) > > > > > }; > > > > > int fd, ruleset_fd; > > > > > > > > > > - /* Enable Landlock. */ > > > > > + /* Enables Landlock. */ > > > > > ruleset_fd = create_ruleset(_metadata, variant->handled, rules); > > > > > ASSERT_LE(0, ruleset_fd); > > > > > enforce_ruleset(_metadata, ruleset_fd); > > > > > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate) > > > > > ASSERT_EQ(0, close(fd)); > > > > > } > > > > > > > > We should also check with O_PATH to make sure the correct error is > > > > returned (and not EACCES). > > > > > > Is this remark referring to the code before it or after it? > > > > > > My interpretation is that you are asking to test that test_fioqsize_ioctl() will > > > return errnos correctly? Do I understand that correctly? (I think that would > > > be a little bit overdone, IMHO - it's just a test utility of ~10 lines after > > > all, which is below the threshold where it can be verified by staring at it for > > > a bit. :)) > > > > I was refering to the previous memfd_ftruncate test, which is changed > > with a next patch. We should check the access rights tied (and checkd) > > to FD (i.e. truncate and ioctl) opened with O_PATH. > > OK, I added a test that checks ioctl(2) and ftruncate(2) on files that > were opened with O_PATH, both before and after enabling Landlock. > ftruncate() and ioctl() always give an EBADF error, both before and > after enabling Landlock (as described in open(2) in the section about > O_PATH). Good! > > A bit outside of the IOCTL path set scope: > > I was surprised that it is even possible to successfully open a file > with O_PATH, even after Landlock is enabled and restricts all it can > in that file hierarchy. This lets you detect that a file exists, even > when that file is in a directory whose contents you are otherwise not > permitted to list due to Landlock. This is indeed intended and tested with the effective_access test. O_PATH is handled the same way as chdir (and similar path-based syscalls) and always allowed. However, I just realized that the O_PATH case is not explicitly mentioned in the documentation. > > The logic for that is in the get_required_file_open_access() function. > Should we add a "LANDLOCK_ACCESS_FS_PATH_FILE" right, which would work > similar to LANDLOCK_ACCESS_FS_READ_FILE and > LANDLOCK_ACCESS_FS_WRITE_FILE, so that this can be restricted? Having a file descriptor opened with O_PATH should not give any specific access (hence the need to test with IOCTLs). O_PATH is designed to get an explicit reference to the filesystem (without the issues of paths) and use the related FD with *at() syscalls, including landlock_add_rule() with a path_beneath rule. FDs opened with O_PATH should not be an issue by themselves for a sandbox, but the real issue is to discover paths, i.e. the directory's execute access right. This is why I think it would make more sense to have something like a LANDLOCK_ACCESS_FS_WALK right instead. This would enable to definitely deny any access to a file hierarchy. For chdir-like syscalls, we could rely on path_permission(), but for a more holistic approach we need to properly handle the execute permission on directories. See Christian's comment on chdir/path_permission and the limit of what an LSM can infer from paths: https://lore.kernel.org/r/20230530-mietfrei-zynisch-8b63a8566f66@brauner We could rely on the last walked (leaf) dentry to allow or deny such walk, but that would still enable malicious processes to infer path because of intermediate walk that may return ENOENT. We could also return ENOENT instead of EACCES, but this would not handle the case of other access controls returning EACCES. With this in mind, I think the better solution is to properly check each intermediate directory during a path walk. This is not currently possible with path-based LSM hooks but I think that we could add a new LSM hook in filename_lookup(), close to the audit_inode() call, to get the necessary information about the currently walked directory. Simply using this new hook with Landlock would add a significant performance impact because of the way Landlock identifies paths (i.e. walk back). An interesting approach to efficiently check Landlock access right would be to store the collected access rights of a path in a cache, and use it when checking a "child" of this path. According to task_struct, there is only one path walk at a time per thread, which would enable us to have only one cached path per task and opportunistically use it in Landlock's is_access_to_paths_allowed() as a backstop to end the walk. With this trick, I think in most cases no walk back would be required, which would be great for performance. The main challenge would be to efficiently handle the ".." directories. See an initial approach of caching for Landlock: https://lore.kernel.org/r/20210630224856.1313928-1-mic@xxxxxxxxxxx Being able to control path walks would be a way to use (most?) inode-based LSM hooks for Landlock, especially the security_inode_permission(). Indeed. we could then tie an inode to a path (resolution) thanks to the cache (and potentially other caches according to the number of checked inodes). This would be great for new access rights such as LANDLOCK_ACCESS_FS_{READ,WRITE}_METADATA. Xiu, that would be a good opportunity to continue your work, and probably without waiting for IMA to be converted to a proper LSM. What do you think?
