On Fri, Jul 26, 2024 at 07:43:28PM GMT, Song Liu wrote:
> Hi Christian,
>
> Thanks a lot for your comments.
>
> > On Jul 26, 2024, at 4:51 AM, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > On Fri, Jul 26, 2024 at 09:19:54AM GMT, Song Liu wrote:
> >> Hi Christian,
> >>
> >>> On Jul 26, 2024, at 12:06 AM, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >>
> >> [...]
> >>
> >>>> +
> >>>> + for (i = 0; i < 10; i++) {
> >>>> + ret = bpf_get_dentry_xattr(dentry, "user.kfunc", &value_ptr);
> >>>> + if (ret == sizeof(expected_value) &&
> >>>> + !bpf_strncmp(value, ret, expected_value))
> >>>> + matches++;
> >>>> +
> >>>> + prev_dentry = dentry;
> >>>> + dentry = bpf_dget_parent(prev_dentry);
> >>>
> >>> Why do you need to walk upwards and instead of reading the xattr values
> >>> during security_inode_permission()?
> >>
> >> In this use case, we would like to add xattr to the directory to cover
> >> all files under it. For example, assume we have the following xattrs:
> >>
> >> /bin xattr: user.policy_A = value_A
> >> /bin/gcc-6.9/ xattr: user.policy_A = value_B
> >> /bin/gcc-6.9/gcc xattr: user.policy_A = value_C
> >>
> >> /bin/gcc-6.9/gcc will use value_C;
> >> /bin/gcc-6.9/<other_files> will use value_B;
> >> /bin/<other_folder_or_file> will use value_A;
> >>
> >> By walking upwards from security_file_open(), we can finish the logic
> >> in a single LSM hook:
> >>
> >> repeat:
> >> if (dentry have user.policy_A) {
> >> /* make decision based on value */;
> >> } else {
> >> dentry = bpf_dget_parent();
> >> goto repeat;
> >> }
> >>
> >> Does this make sense? Or maybe I misunderstood the suggestion?
> >
> > Imho, what you're doing belongs into inode_permission() not into
> > security_file_open(). That's already too late and it's somewhat clear
> > from the example you're using that you're essentially doing permission
> > checking during path lookup.
>
> I am not sure I follow the suggestion to implement this with
> security_inode_permission()? Could you please share more details about
> this idea?
Given a path like /bin/gcc-6.9/gcc what that code currently does is:
* walk down to /bin/gcc-6.9/gcc
* walk up from /bin/gcc-6.9/gcc and then checking xattr labels for:
gcc
gcc-6.9/
bin/
/
That's broken because someone could've done
mv /bin/gcc-6.9/gcc /attack/ and when this walks back and it checks xattrs on
/attack even though the path lookup was for /bin/gcc-6.9. IOW, the
security_file_open() checks have nothing to do with the permission checks that
were done during path lookup.
Why isn't that logic:
* walk down to /bin/gcc-6.9/gcc and check for each component:
security_inode_permission(/)
security_inode_permission(gcc-6.9/)
security_inode_permission(bin/)
security_inode_permission(gcc)
security_file_open(gcc)
I think that dget_parent() logic also wouldn't make sense for relative path
lookups:
dfd = open("/bin/gcc-6.9", O_RDONLY | O_DIRECTORY | O_CLOEXEC);
This walks down to /bin/gcc-6.9 and then walks back up (subject to the
same problem mentioned earlier) and check xattrs for:
gcc-6.9
bin/
/
then that dfd is passed to openat() to open "gcc":
fd = openat(dfd, "gcc", O_RDONLY);
which again walks up to /bin/gcc-6.9 and checks xattrs for:
gcc
gcc-6.9
bin/
/
Which means this code ends up charging relative lookups twice. Even if one
irons that out in the program this encourages really bad patterns.
Path lookup is iterative top down. One can't just randomly walk back up and
assume that's equivalent.
