* [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