linux-kernel - Re: [PATCH bpf-next 2/2] selftests/bpf: Add tests for bpf_get_dentry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240729-zollfrei-verteidigen-cf359eb36601@brauner>
Date: Mon, 29 Jul 2024 15:46:06 +0200
From: Christian Brauner <brauner@...nel.org>
To: Song Liu <songliubraving@...a.com>
Cc: Christian Brauner <brauner@...nel.org>,
	Song Liu <song@...nel.org>,
	bpf <bpf@...r.kernel.org>,
	Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Kernel Team <kernel-team@...a.com>,
	"andrii@...nel.org" <andrii@...nel.org>,
	"eddyz87@...il.com" <eddyz87@...il.com>,
	"ast@...nel.org" <ast@...nel.org>,
	"daniel@...earbox.net" <daniel@...earbox.net>,
	"martin.lau@...ux.dev" <martin.lau@...ux.dev>,
	"viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
	"jack@...e.cz" <jack@...e.cz>,
	"kpsingh@...nel.org" <kpsingh@...nel.org>,
	"mattbobrowski@...gle.com" <mattbobrowski@...gle.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Add tests for bpf_get_dentry_xattr

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@...nel.org> 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@...nel.org> 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.