* [PATCH bpf v1] Fix the case of running rootless with capabilities
@ 2022-09-20 5:20 Jon Doron
2022-09-21 0:15 ` Andrii Nakryiko
0 siblings, 1 reply; 3+ messages in thread
From: Jon Doron @ 2022-09-20 5:20 UTC (permalink / raw)
To: bpf, ast, andrii; +Cc: Jon Doron
From: Jon Doron <jond@wiz.io>
When running rootless with special capabilities like:
FOWNER / DAC_OVERRIDE / DAC_READ_SEARCH
The access API will not make the proper check if there is really
access to a file or not.
Signed-off-by: Jon Doron <jond@wiz.io>
---
tools/lib/bpf/libbpf.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 50d41815f431..df804fd65493 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -875,8 +875,9 @@ __u32 get_kernel_version(void)
const char *ubuntu_kver_file = "/proc/version_signature";
__u32 major, minor, patch;
struct utsname info;
+ struct stat sb;
- if (access(ubuntu_kver_file, R_OK) == 0) {
+ if (stat(ubuntu_kver_file, &sb) == 0) {
FILE *f;
f = fopen(ubuntu_kver_file, "r");
@@ -9877,9 +9878,10 @@ static int append_to_file(const char *file, const char *fmt, ...)
static bool use_debugfs(void)
{
static int has_debugfs = -1;
+ struct stat sb;
if (has_debugfs < 0)
- has_debugfs = access(DEBUGFS, F_OK) == 0;
+ has_debugfs = stat(DEBUGFS, &sb) == 0;
return has_debugfs == 1;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf v1] Fix the case of running rootless with capabilities
2022-09-20 5:20 [PATCH bpf v1] Fix the case of running rootless with capabilities Jon Doron
@ 2022-09-21 0:15 ` Andrii Nakryiko
2022-09-21 0:17 ` Andrii Nakryiko
0 siblings, 1 reply; 3+ messages in thread
From: Andrii Nakryiko @ 2022-09-21 0:15 UTC (permalink / raw)
To: Jon Doron; +Cc: bpf, ast, andrii, Jon Doron
On Mon, Sep 19, 2022 at 10:21 PM Jon Doron <arilou@gmail.com> wrote:
>
> From: Jon Doron <jond@wiz.io>
>
> When running rootless with special capabilities like:
> FOWNER / DAC_OVERRIDE / DAC_READ_SEARCH
>
> The access API will not make the proper check if there is really
> access to a file or not.
>
This is very succinct and doesn't explain why access() doesn't work. I
had to read the man page for access() to (hopefully) understand what's
going on. Please elaborate a bit more and maybe quote man page:
The check is done using the calling process's real UID and GID,
rather than the effective IDs as is done when actually attempting
an operation (e.g., open(2)) on the file. Similarly, for the
root user, the check uses the set of permitted capabilities
rather than the set of effective capabilities; and for non-root
users, the check uses an empty set of capabilities.
This allows set-user-ID programs and capability-endowed programs
to easily determine the invoking user's authority. In other
words, access() does not answer the "can I read/write/execute
this file?" question. It answers a slightly different question:
"(assuming I'm a setuid binary) can the user who invoked me
read/write/execute this file?", which gives set-user-ID programs
the possibility to prevent malicious users from causing them to
read files which users shouldn't be able to read.
So if I understand correctly, access() is self-limiting itself
artificially, while in practice target file can be totally readable
due to caps or effective user ID differences.
Please try to summarize this in the commit message.
> Signed-off-by: Jon Doron <jond@wiz.io>
> ---
> tools/lib/bpf/libbpf.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 50d41815f431..df804fd65493 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -875,8 +875,9 @@ __u32 get_kernel_version(void)
> const char *ubuntu_kver_file = "/proc/version_signature";
> __u32 major, minor, patch;
> struct utsname info;
> + struct stat sb;
>
> - if (access(ubuntu_kver_file, R_OK) == 0) {
> + if (stat(ubuntu_kver_file, &sb) == 0) {
> FILE *f;
>
> f = fopen(ubuntu_kver_file, "r");
> @@ -9877,9 +9878,10 @@ static int append_to_file(const char *file, const char *fmt, ...)
> static bool use_debugfs(void)
> {
> static int has_debugfs = -1;
> + struct stat sb;
>
> if (has_debugfs < 0)
> - has_debugfs = access(DEBUGFS, F_OK) == 0;
> + has_debugfs = stat(DEBUGFS, &sb) == 0;
>
I found in total 5 access() uses in libbpf, can you please update the
other 3 as well? Thanks!
> return has_debugfs == 1;
> }
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf v1] Fix the case of running rootless with capabilities
2022-09-21 0:15 ` Andrii Nakryiko
@ 2022-09-21 0:17 ` Andrii Nakryiko
0 siblings, 0 replies; 3+ messages in thread
From: Andrii Nakryiko @ 2022-09-21 0:17 UTC (permalink / raw)
To: Jon Doron; +Cc: bpf, ast, andrii, Jon Doron
On Tue, Sep 20, 2022 at 5:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Sep 19, 2022 at 10:21 PM Jon Doron <arilou@gmail.com> wrote:
> >
> > From: Jon Doron <jond@wiz.io>
> >
> > When running rootless with special capabilities like:
> > FOWNER / DAC_OVERRIDE / DAC_READ_SEARCH
> >
> > The access API will not make the proper check if there is really
> > access to a file or not.
> >
>
> This is very succinct and doesn't explain why access() doesn't work. I
> had to read the man page for access() to (hopefully) understand what's
> going on. Please elaborate a bit more and maybe quote man page:
>
> The check is done using the calling process's real UID and GID,
> rather than the effective IDs as is done when actually attempting
> an operation (e.g., open(2)) on the file. Similarly, for the
> root user, the check uses the set of permitted capabilities
> rather than the set of effective capabilities; and for non-root
> users, the check uses an empty set of capabilities.
>
> This allows set-user-ID programs and capability-endowed programs
> to easily determine the invoking user's authority. In other
> words, access() does not answer the "can I read/write/execute
> this file?" question. It answers a slightly different question:
> "(assuming I'm a setuid binary) can the user who invoked me
> read/write/execute this file?", which gives set-user-ID programs
> the possibility to prevent malicious users from causing them to
> read files which users shouldn't be able to read.
>
> So if I understand correctly, access() is self-limiting itself
> artificially, while in practice target file can be totally readable
> due to caps or effective user ID differences.
>
> Please try to summarize this in the commit message.
>
Oh, and I think it's fine to target bpf-next tree with this. Also
please prefix libbpf patches with "libbpf: ", so your patch subject
should start with "[PATCH v2 bpf-next] libbpf: ..."
> > Signed-off-by: Jon Doron <jond@wiz.io>
> > ---
> > tools/lib/bpf/libbpf.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 50d41815f431..df804fd65493 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -875,8 +875,9 @@ __u32 get_kernel_version(void)
> > const char *ubuntu_kver_file = "/proc/version_signature";
> > __u32 major, minor, patch;
> > struct utsname info;
> > + struct stat sb;
> >
> > - if (access(ubuntu_kver_file, R_OK) == 0) {
> > + if (stat(ubuntu_kver_file, &sb) == 0) {
> > FILE *f;
> >
> > f = fopen(ubuntu_kver_file, "r");
> > @@ -9877,9 +9878,10 @@ static int append_to_file(const char *file, const char *fmt, ...)
> > static bool use_debugfs(void)
> > {
> > static int has_debugfs = -1;
> > + struct stat sb;
> >
> > if (has_debugfs < 0)
> > - has_debugfs = access(DEBUGFS, F_OK) == 0;
> > + has_debugfs = stat(DEBUGFS, &sb) == 0;
> >
>
> I found in total 5 access() uses in libbpf, can you please update the
> other 3 as well? Thanks!
>
>
> > return has_debugfs == 1;
> > }
> > --
> > 2.37.3
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-21 0:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-20 5:20 [PATCH bpf v1] Fix the case of running rootless with capabilities Jon Doron
2022-09-21 0:15 ` Andrii Nakryiko
2022-09-21 0:17 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox