v2:
* abstract walk_to_visible_parent() helper
* chmod and chown rights only take affect on directory's context
* add testcase for fchmodat/lchown/fchownat
* fix other review issues
Xiu Jianfeng (6):
landlock: expand access_mask_t to u32 type
landlock: abstract walk_to_visible_parent() helper
landlock: add chmod and chown support
landlock/selftests: add selftests for chmod and chown
landlock/samples: add chmod and chown support
landlock: update chmod and chown support in document
Documentation/userspace-api/landlock.rst | 9 +-
include/uapi/linux/landlock.h | 10 +-
samples/landlock/sandboxer.c | 13 +-
security/landlock/fs.c | 110 ++++++--
security/landlock/limits.h | 2 +-
security/landlock/ruleset.h | 2 +-
security/landlock/syscalls.c | 2 +-
tools/testing/selftests/landlock/base_test.c | 2 +-
tools/testing/selftests/landlock/fs_test.c | 267 ++++++++++++++++++-
9 files changed, 386 insertions(+), 31 deletions(-)
--
2.17.1
Please add a description and include links to the previous versions.
On 27/08/2022 13:12, Xiu Jianfeng wrote:
> v2:
> * abstract walk_to_visible_parent() helper
> * chmod and chown rights only take affect on directory's context
> * add testcase for fchmodat/lchown/fchownat
> * fix other review issues
>
> Xiu Jianfeng (6):
> landlock: expand access_mask_t to u32 type
> landlock: abstract walk_to_visible_parent() helper
> landlock: add chmod and chown support
> landlock/selftests: add selftests for chmod and chown
> landlock/samples: add chmod and chown support
> landlock: update chmod and chown support in document
>
> Documentation/userspace-api/landlock.rst | 9 +-
> include/uapi/linux/landlock.h | 10 +-
> samples/landlock/sandboxer.c | 13 +-
> security/landlock/fs.c | 110 ++++++--
> security/landlock/limits.h | 2 +-
> security/landlock/ruleset.h | 2 +-
> security/landlock/syscalls.c | 2 +-
> tools/testing/selftests/landlock/base_test.c | 2 +-
> tools/testing/selftests/landlock/fs_test.c | 267 ++++++++++++++++++-
> 9 files changed, 386 insertions(+), 31 deletions(-)
>
Hi Mickael,
Sorry about the long silence on this work, As we known this work depends
on another work about changing argument from struct dentry to struct
path for some attr/xattr related lsm hooks, I'm stuck with this thing,
because IMA/EVM is a special security module which is not LSM-based
currently, and severely coupled with the file system. so I am waiting
for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
be ready, I think it can make my work more easy. you can find
Roberto'work here,
https://lwn.net/ml/linux-kernel/[email protected]/
Any good idea are welcome, thanks.
On 2022/8/27 19:12, Xiu Jianfeng wrote:
> v2:
> * abstract walk_to_visible_parent() helper
> * chmod and chown rights only take affect on directory's context
> * add testcase for fchmodat/lchown/fchownat
> * fix other review issues
>
> Xiu Jianfeng (6):
> landlock: expand access_mask_t to u32 type
> landlock: abstract walk_to_visible_parent() helper
> landlock: add chmod and chown support
> landlock/selftests: add selftests for chmod and chown
> landlock/samples: add chmod and chown support
> landlock: update chmod and chown support in document
>
> Documentation/userspace-api/landlock.rst | 9 +-
> include/uapi/linux/landlock.h | 10 +-
> samples/landlock/sandboxer.c | 13 +-
> security/landlock/fs.c | 110 ++++++--
> security/landlock/limits.h | 2 +-
> security/landlock/ruleset.h | 2 +-
> security/landlock/syscalls.c | 2 +-
> tools/testing/selftests/landlock/base_test.c | 2 +-
> tools/testing/selftests/landlock/fs_test.c | 267 ++++++++++++++++++-
> 9 files changed, 386 insertions(+), 31 deletions(-)
>
On 18/04/2023 12:53, xiujianfeng wrote:
> Hi Mickael,
>
> Sorry about the long silence on this work, As we known this work depends
> on another work about changing argument from struct dentry to struct
> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
> because IMA/EVM is a special security module which is not LSM-based
> currently, and severely coupled with the file system. so I am waiting
> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
> be ready, I think it can make my work more easy. you can find
> Roberto'work here,
> https://lwn.net/ml/linux-kernel/[email protected]/
>
> Any good idea are welcome, thanks.
Thanks for the update Xiu.
Which part would be needed from Roberto's patch series?
>
>
> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>> v2:
>> * abstract walk_to_visible_parent() helper
>> * chmod and chown rights only take affect on directory's context
>> * add testcase for fchmodat/lchown/fchownat
>> * fix other review issues
>>
>> Xiu Jianfeng (6):
>> landlock: expand access_mask_t to u32 type
>> landlock: abstract walk_to_visible_parent() helper
>> landlock: add chmod and chown support
>> landlock/selftests: add selftests for chmod and chown
>> landlock/samples: add chmod and chown support
>> landlock: update chmod and chown support in document
>>
>> Documentation/userspace-api/landlock.rst | 9 +-
>> include/uapi/linux/landlock.h | 10 +-
>> samples/landlock/sandboxer.c | 13 +-
>> security/landlock/fs.c | 110 ++++++--
>> security/landlock/limits.h | 2 +-
>> security/landlock/ruleset.h | 2 +-
>> security/landlock/syscalls.c | 2 +-
>> tools/testing/selftests/landlock/base_test.c | 2 +-
>> tools/testing/selftests/landlock/fs_test.c | 267 ++++++++++++++++++-
>> 9 files changed, 386 insertions(+), 31 deletions(-)
>>
On 2023/4/21 1:40, Mickaël Salaün wrote:
>
> On 18/04/2023 12:53, xiujianfeng wrote:
>> Hi Mickael,
>>
>> Sorry about the long silence on this work, As we known this work depends
>> on another work about changing argument from struct dentry to struct
>> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
>> because IMA/EVM is a special security module which is not LSM-based
>> currently, and severely coupled with the file system. so I am waiting
>> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
>> be ready, I think it can make my work more easy. you can find
>> Roberto'work here,
>> https://lwn.net/ml/linux-kernel/[email protected]/
>>
>> Any good idea are welcome, thanks.
>
> Thanks for the update Xiu.
>
> Which part would be needed from Roberto's patch series?
>
As we discussed before, the two access rights that need to be added and
their usage is as below:
LANDLOCK_ACCESS_FS_WRITE_METADATA controls
1.inode_setattr
2.inode_setxattr
3.inode_removexattr
4.inode_set_acl
5.inode_remove_acl
LANDLOCK_ACCESS_FS_READ_METADATA controls
1.inode_getattr
2.inode_get_acl
3.inode_getxattr
4.inode_listxattr
all these APIs should be changed to use struct path instead of dentry,
and then several vfs APIs as follows are invovled:
notify_change,
__vfs_setxattr_locked,
__vfs_removexattr_locked,
__vfs_setxattr_noperm
vfs_set_acl
vfs_remove_acl
vfs_getxattr
vfs_listxattr
vfs_get_acl
and also include some LSM hooks such as inode_post_setxattr and
inode_setsecctx.
Since the original places where pass dentry to security_inode_xxx may
not have any struct path, we have to pass it from the top caller, so
this also touches lots of filesystems(e.g. cachefiles, ecryptfs, ksmbd,
nfsd, overlayfs...).
Other LSMs such as selinux, smack can be easy to refator because they
are LSM-based, and if VFS passes path to security_inode_xxx and they can
just use path->dentry instead inside they own modules.
AS for IMA/EVM, unfortunately they are not LSM-based and coupled with
the file system. To make things worse, there is a recursive dependency
situation during the update of extended attribute which happen as follows:
__vfs_setxattr_noperm
=> security_inode_post_setxattr
=> evm_inode_post_setxattr
=> evm_update_evmxattr
=> __vfs_setxattr_noperm
To change the argument of __vfs_setxattr_noperm from a dentry to the
path structure, the two EVM functions would have to be altered as well.
However, evm_update_evmxattr is called by 3 other EVM functions who
lives in the very heart of the complicated EVM framework. Any change to
them would cause a nasty chain reaction in EVM and, as IMA would trigger
EVM directly, in IMA as well.
There is another callchain as follow:
ima_appraise_measurement
=>evm_verifyxattr
=>evm_verifyxattr
=>evm_verify_hmac
=>evm_calc_hash
=>evm_calc_hmac_or_hash
=>vfs_getxattr
Passing struct path into vfs_getxattr() would also affect this
callchain. Currently ima_appraise_measurment accepts a struct file, and
dentry is generated from file_dentry(file) in order to mitigate a
deadlock issue involving overlayfs(commit e71b9dff0634ed). Once
&file->f_path is passed through this callchain, and someone wants the
dentry, it will be using file->f_path.dentry, which is different from
file_dentry(file). In the overlayfs scenario, may this cause an issue?
The patchset of moving IMA and EVM into the LSM infrastructe would be
helpfull but still can not completely resolve this situation. more
refactor would be needed in EVM. That's all that's happening right now.
>
>>
>>
>> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>>> v2:
>>> * abstract walk_to_visible_parent() helper
>>> * chmod and chown rights only take affect on directory's context
>>> * add testcase for fchmodat/lchown/fchownat
>>> * fix other review issues
>>>
>>> Xiu Jianfeng (6):
>>> landlock: expand access_mask_t to u32 type
>>> landlock: abstract walk_to_visible_parent() helper
>>> landlock: add chmod and chown support
>>> landlock/selftests: add selftests for chmod and chown
>>> landlock/samples: add chmod and chown support
>>> landlock: update chmod and chown support in document
>>>
>>> Documentation/userspace-api/landlock.rst | 9 +-
>>> include/uapi/linux/landlock.h | 10 +-
>>> samples/landlock/sandboxer.c | 13 +-
>>> security/landlock/fs.c | 110 ++++++--
>>> security/landlock/limits.h | 2 +-
>>> security/landlock/ruleset.h | 2 +-
>>> security/landlock/syscalls.c | 2 +-
>>> tools/testing/selftests/landlock/base_test.c | 2 +-
>>> tools/testing/selftests/landlock/fs_test.c | 267 ++++++++++++++++++-
>>> 9 files changed, 386 insertions(+), 31 deletions(-)
>>>
On 24/04/2023 10:52, xiujianfeng wrote:
>
>
> On 2023/4/21 1:40, Mickaël Salaün wrote:
>>
>> On 18/04/2023 12:53, xiujianfeng wrote:
>>> Hi Mickael,
>>>
>>> Sorry about the long silence on this work, As we known this work depends
>>> on another work about changing argument from struct dentry to struct
>>> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
>>> because IMA/EVM is a special security module which is not LSM-based
>>> currently, and severely coupled with the file system. so I am waiting
>>> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
>>> be ready, I think it can make my work more easy. you can find
>>> Roberto'work here,
>>> https://lwn.net/ml/linux-kernel/[email protected]/
>>>
>>> Any good idea are welcome, thanks.
>>
>> Thanks for the update Xiu.
>>
>> Which part would be needed from Roberto's patch series?
>>
> As we discussed before, the two access rights that need to be added and
> their usage is as below:
> LANDLOCK_ACCESS_FS_WRITE_METADATA controls
> 1.inode_setattr
> 2.inode_setxattr
> 3.inode_removexattr
> 4.inode_set_acl
> 5.inode_remove_acl
> LANDLOCK_ACCESS_FS_READ_METADATA controls
> 1.inode_getattr
> 2.inode_get_acl
> 3.inode_getxattr
> 4.inode_listxattr
>
> all these APIs should be changed to use struct path instead of dentry,
> and then several vfs APIs as follows are invovled:
> notify_change,
> __vfs_setxattr_locked,
> __vfs_removexattr_locked,
> __vfs_setxattr_noperm
> vfs_set_acl
> vfs_remove_acl
> vfs_getxattr
> vfs_listxattr
> vfs_get_acl
> and also include some LSM hooks such as inode_post_setxattr and
> inode_setsecctx.
>
> Since the original places where pass dentry to security_inode_xxx may
> not have any struct path, we have to pass it from the top caller, so
> this also touches lots of filesystems(e.g. cachefiles, ecryptfs, ksmbd,
> nfsd, overlayfs...).
>
> Other LSMs such as selinux, smack can be easy to refator because they
> are LSM-based, and if VFS passes path to security_inode_xxx and they can
> just use path->dentry instead inside they own modules.
>
> AS for IMA/EVM, unfortunately they are not LSM-based and coupled with
> the file system. To make things worse, there is a recursive dependency
> situation during the update of extended attribute which happen as follows:
>
> __vfs_setxattr_noperm
> => security_inode_post_setxattr
> => evm_inode_post_setxattr
> => evm_update_evmxattr
> => __vfs_setxattr_noperm
>
> To change the argument of __vfs_setxattr_noperm from a dentry to the
> path structure, the two EVM functions would have to be altered as well.
> However, evm_update_evmxattr is called by 3 other EVM functions who
> lives in the very heart of the complicated EVM framework. Any change to
> them would cause a nasty chain reaction in EVM and, as IMA would trigger
> EVM directly, in IMA as well.
>
> There is another callchain as follow:
> ima_appraise_measurement
> =>evm_verifyxattr
> =>evm_verifyxattr
> =>evm_verify_hmac
> =>evm_calc_hash
> =>evm_calc_hmac_or_hash
> =>vfs_getxattr
> Passing struct path into vfs_getxattr() would also affect this
> callchain. Currently ima_appraise_measurment accepts a struct file, and
> dentry is generated from file_dentry(file) in order to mitigate a
> deadlock issue involving overlayfs(commit e71b9dff0634ed). Once
> &file->f_path is passed through this callchain, and someone wants the
> dentry, it will be using file->f_path.dentry, which is different from
> file_dentry(file). In the overlayfs scenario, may this cause an issue?
I might be OK, but this need to be tested.
>
> The patchset of moving IMA and EVM into the LSM infrastructe would be
> helpfull but still can not completely resolve this situation. more
> refactor would be needed in EVM. That's all that's happening right now.
OK, thanks for the detailed explanation!
I guess you could start with easier hooks (e.g. inode_getattr and
inode_setattr) to see if there is potentially other implications, and
incrementally build on that.
>
>>
>>>
>>>
>>> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>>>> v2:
>>>> * abstract walk_to_visible_parent() helper
>>>> * chmod and chown rights only take affect on directory's context
>>>> * add testcase for fchmodat/lchown/fchownat
>>>> * fix other review issues
>>>>
>>>> Xiu Jianfeng (6):
>>>> landlock: expand access_mask_t to u32 type
>>>> landlock: abstract walk_to_visible_parent() helper
>>>> landlock: add chmod and chown support
>>>> landlock/selftests: add selftests for chmod and chown
>>>> landlock/samples: add chmod and chown support
>>>> landlock: update chmod and chown support in document
>>>>
>>>> Documentation/userspace-api/landlock.rst | 9 +-
>>>> include/uapi/linux/landlock.h | 10 +-
>>>> samples/landlock/sandboxer.c | 13 +-
>>>> security/landlock/fs.c | 110 ++++++--
>>>> security/landlock/limits.h | 2 +-
>>>> security/landlock/ruleset.h | 2 +-
>>>> security/landlock/syscalls.c | 2 +-
>>>> tools/testing/selftests/landlock/base_test.c | 2 +-
>>>> tools/testing/selftests/landlock/fs_test.c | 267 ++++++++++++++++++-
>>>> 9 files changed, 386 insertions(+), 31 deletions(-)
>>>>
On 2023/4/26 21:58, Mickaël Salaün wrote:
>
>
> On 24/04/2023 10:52, xiujianfeng wrote:
>>
>>
>> On 2023/4/21 1:40, Mickaël Salaün wrote:
>>>
>>> On 18/04/2023 12:53, xiujianfeng wrote:
>>>> Hi Mickael,
>>>>
>>>> Sorry about the long silence on this work, As we known this work
>>>> depends
>>>> on another work about changing argument from struct dentry to struct
>>>> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
>>>> because IMA/EVM is a special security module which is not LSM-based
>>>> currently, and severely coupled with the file system. so I am waiting
>>>> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
>>>> be ready, I think it can make my work more easy. you can find
>>>> Roberto'work here,
>>>> https://lwn.net/ml/linux-kernel/[email protected]/
>>>>
>>>> Any good idea are welcome, thanks.
>>>
>>> Thanks for the update Xiu.
>>>
>>> Which part would be needed from Roberto's patch series?
>>>
>> As we discussed before, the two access rights that need to be added and
>> their usage is as below:
>> LANDLOCK_ACCESS_FS_WRITE_METADATA controls
>> 1.inode_setattr
>> 2.inode_setxattr
>> 3.inode_removexattr
>> 4.inode_set_acl
>> 5.inode_remove_acl
>> LANDLOCK_ACCESS_FS_READ_METADATA controls
>> 1.inode_getattr
>> 2.inode_get_acl
>> 3.inode_getxattr
>> 4.inode_listxattr
>>
>> all these APIs should be changed to use struct path instead of dentry,
>> and then several vfs APIs as follows are invovled:
>> notify_change,
>> __vfs_setxattr_locked,
>> __vfs_removexattr_locked,
>> __vfs_setxattr_noperm
>> vfs_set_acl
>> vfs_remove_acl
>> vfs_getxattr
>> vfs_listxattr
>> vfs_get_acl
>> and also include some LSM hooks such as inode_post_setxattr and
>> inode_setsecctx.
>>
>> Since the original places where pass dentry to security_inode_xxx may
>> not have any struct path, we have to pass it from the top caller, so
>> this also touches lots of filesystems(e.g. cachefiles, ecryptfs, ksmbd,
>> nfsd, overlayfs...).
>>
>> Other LSMs such as selinux, smack can be easy to refator because they
>> are LSM-based, and if VFS passes path to security_inode_xxx and they can
>> just use path->dentry instead inside they own modules.
>>
>> AS for IMA/EVM, unfortunately they are not LSM-based and coupled with
>> the file system. To make things worse, there is a recursive dependency
>> situation during the update of extended attribute which happen as
>> follows:
>>
>> __vfs_setxattr_noperm
>> => security_inode_post_setxattr
>> => evm_inode_post_setxattr
>> => evm_update_evmxattr
>> => __vfs_setxattr_noperm
>>
>> To change the argument of __vfs_setxattr_noperm from a dentry to the
>> path structure, the two EVM functions would have to be altered as well.
>> However, evm_update_evmxattr is called by 3 other EVM functions who
>> lives in the very heart of the complicated EVM framework. Any change to
>> them would cause a nasty chain reaction in EVM and, as IMA would trigger
>> EVM directly, in IMA as well.
>>
>> There is another callchain as follow:
>> ima_appraise_measurement
>> =>evm_verifyxattr
>> =>evm_verifyxattr
>> =>evm_verify_hmac
>> =>evm_calc_hash
>> =>evm_calc_hmac_or_hash
>> =>vfs_getxattr
>> Passing struct path into vfs_getxattr() would also affect this
>> callchain. Currently ima_appraise_measurment accepts a struct file, and
>> dentry is generated from file_dentry(file) in order to mitigate a
>> deadlock issue involving overlayfs(commit e71b9dff0634ed). Once
>> &file->f_path is passed through this callchain, and someone wants the
>> dentry, it will be using file->f_path.dentry, which is different from
>> file_dentry(file). In the overlayfs scenario, may this cause an issue?
>
> I might be OK, but this need to be tested.
>
>>
>> The patchset of moving IMA and EVM into the LSM infrastructe would be
>> helpfull but still can not completely resolve this situation. more
>> refactor would be needed in EVM. That's all that's happening right now.
>
> OK, thanks for the detailed explanation!
>
> I guess you could start with easier hooks (e.g. inode_getattr and
> inode_setattr) to see if there is potentially other implications, and
> incrementally build on that.
I was thinking to get everything done at once, but it looks like we are
having some troubles, so it's good advice to separate it into small
works and upstream them on by one, Thanks.
Currently inode_getattr already takes a struct path argument in the
mainline, I would send the patches about inode_setattr refactor and
discuss the problem of file_dentry() on that thread.
>
>
>>
>>>
>>>>
>>>>
>>>> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>>>>> v2:
>>>>> * abstract walk_to_visible_parent() helper
>>>>> * chmod and chown rights only take affect on directory's context
>>>>> * add testcase for fchmodat/lchown/fchownat
>>>>> * fix other review issues
>>>>>
>>>>> Xiu Jianfeng (6):
>>>>> landlock: expand access_mask_t to u32 type
>>>>> landlock: abstract walk_to_visible_parent() helper
>>>>> landlock: add chmod and chown support
>>>>> landlock/selftests: add selftests for chmod and chown
>>>>> landlock/samples: add chmod and chown support
>>>>> landlock: update chmod and chown support in document
>>>>>
>>>>> Documentation/userspace-api/landlock.rst | 9 +-
>>>>> include/uapi/linux/landlock.h | 10 +-
>>>>> samples/landlock/sandboxer.c | 13 +-
>>>>> security/landlock/fs.c | 110 ++++++--
>>>>> security/landlock/limits.h | 2 +-
>>>>> security/landlock/ruleset.h | 2 +-
>>>>> security/landlock/syscalls.c | 2 +-
>>>>> tools/testing/selftests/landlock/base_test.c | 2 +-
>>>>> tools/testing/selftests/landlock/fs_test.c | 267
>>>>> ++++++++++++++++++-
>>>>> 9 files changed, 386 insertions(+), 31 deletions(-)
>>>>>