* [PATCH bpf-next v2] libbpf: Fix the case of running as non-root with capabilities
@ 2022-09-21 8:40 Jon Doron
2022-09-23 21:10 ` Andrii Nakryiko
0 siblings, 1 reply; 2+ messages in thread
From: Jon Doron @ 2022-09-21 8:40 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.
From the access 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.***
"
What that means is that for non-root user the access API will not do the
proper validation if the process really has permission to a file or not.
To resolve this this patch replaces all the access API calls with stat.
Signed-off-by: Jon Doron <jond@wiz.io>
---
tools/lib/bpf/btf.c | 3 ++-
tools/lib/bpf/libbpf.c | 11 ++++++++---
tools/lib/bpf/usdt.c | 4 +++-
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 2d14f1a52d7a..33ad4792d9e8 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -4663,13 +4663,14 @@ struct btf *btf__load_vmlinux_btf(void)
struct utsname buf;
struct btf *btf;
int i, err;
+ struct stat sb;
uname(&buf);
for (i = 0; i < ARRAY_SIZE(locations); i++) {
snprintf(path, PATH_MAX, locations[i].path_fmt, buf.release);
- if (access(path, R_OK))
+ if (stat(path, &sb))
continue;
if (locations[i].raw_btf)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 50d41815f431..c7fbce4225b5 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;
}
@@ -10681,6 +10683,7 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
for (s = search_paths[i]; s != NULL; s = strchr(s, ':')) {
char *next_path;
int seg_len;
+ struct stat sb;
if (s[0] == ':')
s++;
@@ -10690,7 +10693,9 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
continue;
snprintf(result, result_sz, "%.*s/%s", seg_len, s, file);
/* ensure it is an executable file/link */
- if (access(result, R_OK | X_OK) < 0)
+ if (stat(result, &sb) < 0)
+ continue;
+ if ((sb.st_mode & (S_IROTH | S_IXOTH)) != (S_IROTH | S_IXOTH))
continue;
pr_debug("resolved '%s' to '%s'\n", file, result);
return 0;
diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
index d18e37982344..19a6fbcfe9c0 100644
--- a/tools/lib/bpf/usdt.c
+++ b/tools/lib/bpf/usdt.c
@@ -7,6 +7,7 @@
#include <libelf.h>
#include <gelf.h>
#include <unistd.h>
+#include <sys/stat.h>
#include <linux/ptrace.h>
#include <linux/kernel.h>
@@ -257,6 +258,7 @@ struct usdt_manager *usdt_manager_new(struct bpf_object *obj)
static const char *ref_ctr_sysfs_path = "/sys/bus/event_source/devices/uprobe/format/ref_ctr_offset";
struct usdt_manager *man;
struct bpf_map *specs_map, *ip_to_spec_id_map;
+ struct stat sb;
specs_map = bpf_object__find_map_by_name(obj, "__bpf_usdt_specs");
ip_to_spec_id_map = bpf_object__find_map_by_name(obj, "__bpf_usdt_ip_to_spec_id");
@@ -282,7 +284,7 @@ struct usdt_manager *usdt_manager_new(struct bpf_object *obj)
* If this is not supported, USDTs with semaphores will not be supported.
* Added in: a6ca88b241d5 ("trace_uprobe: support reference counter in fd-based uprobe")
*/
- man->has_sema_refcnt = access(ref_ctr_sysfs_path, F_OK) == 0;
+ man->has_sema_refcnt = stat(ref_ctr_sysfs_path, &sb) == 0;
return man;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Fix the case of running as non-root with capabilities
2022-09-21 8:40 [PATCH bpf-next v2] libbpf: Fix the case of running as non-root with capabilities Jon Doron
@ 2022-09-23 21:10 ` Andrii Nakryiko
0 siblings, 0 replies; 2+ messages in thread
From: Andrii Nakryiko @ 2022-09-23 21:10 UTC (permalink / raw)
To: Jon Doron; +Cc: bpf, ast, andrii, Jon Doron
On Wed, Sep 21, 2022 at 1:40 AM 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.
>
> From the access 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.***
> "
>
> What that means is that for non-root user the access API will not do the
> proper validation if the process really has permission to a file or not.
>
> To resolve this this patch replaces all the access API calls with stat.
>
> Signed-off-by: Jon Doron <jond@wiz.io>
> ---
> tools/lib/bpf/btf.c | 3 ++-
> tools/lib/bpf/libbpf.c | 11 ++++++++---
> tools/lib/bpf/usdt.c | 4 +++-
> 3 files changed, 13 insertions(+), 5 deletions(-)
>
This patch doesn't apply cleanly onto bpf-next, please rebase. But
also see comments below.
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 2d14f1a52d7a..33ad4792d9e8 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -4663,13 +4663,14 @@ struct btf *btf__load_vmlinux_btf(void)
> struct utsname buf;
> struct btf *btf;
> int i, err;
> + struct stat sb;
>
> uname(&buf);
>
> for (i = 0; i < ARRAY_SIZE(locations); i++) {
> snprintf(path, PATH_MAX, locations[i].path_fmt, buf.release);
>
> - if (access(path, R_OK))
> + if (stat(path, &sb))
> continue;
>
> if (locations[i].raw_btf)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 50d41815f431..c7fbce4225b5 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;
> }
> @@ -10681,6 +10683,7 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
> for (s = search_paths[i]; s != NULL; s = strchr(s, ':')) {
> char *next_path;
> int seg_len;
> + struct stat sb;
>
> if (s[0] == ':')
> s++;
> @@ -10690,7 +10693,9 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
> continue;
> snprintf(result, result_sz, "%.*s/%s", seg_len, s, file);
> /* ensure it is an executable file/link */
> - if (access(result, R_OK | X_OK) < 0)
> + if (stat(result, &sb) < 0)
> + continue;
> + if ((sb.st_mode & (S_IROTH | S_IXOTH)) != (S_IROTH | S_IXOTH))
So this bit look a bit suspicious. Why are we only checking others
bits? And in general, it seems pretty unfortunate to check access bits
manually.
So two thoughts.
1. Have you tried using faccessat2() with AT_EACCESS flag? It seems
like that (apart from all the stuff in BUGS section in man page,
sigh...) delegate the actual permission checks using effective user to
kernel. This seems like a bit better and safer way?
2. Whichever way we go, I think let's add an internal wrapper that
will hide struct stat (which we almost never use) and will simulate
access just with proper checks? Like libbpf_faccess() or something
along those lines? You can expose it in libbpf_internal.h
> continue;
> pr_debug("resolved '%s' to '%s'\n", file, result);
> return 0;
> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> index d18e37982344..19a6fbcfe9c0 100644
> --- a/tools/lib/bpf/usdt.c
> +++ b/tools/lib/bpf/usdt.c
> @@ -7,6 +7,7 @@
> #include <libelf.h>
> #include <gelf.h>
> #include <unistd.h>
> +#include <sys/stat.h>
> #include <linux/ptrace.h>
> #include <linux/kernel.h>
>
> @@ -257,6 +258,7 @@ struct usdt_manager *usdt_manager_new(struct bpf_object *obj)
> static const char *ref_ctr_sysfs_path = "/sys/bus/event_source/devices/uprobe/format/ref_ctr_offset";
> struct usdt_manager *man;
> struct bpf_map *specs_map, *ip_to_spec_id_map;
> + struct stat sb;
>
> specs_map = bpf_object__find_map_by_name(obj, "__bpf_usdt_specs");
> ip_to_spec_id_map = bpf_object__find_map_by_name(obj, "__bpf_usdt_ip_to_spec_id");
> @@ -282,7 +284,7 @@ struct usdt_manager *usdt_manager_new(struct bpf_object *obj)
> * If this is not supported, USDTs with semaphores will not be supported.
> * Added in: a6ca88b241d5 ("trace_uprobe: support reference counter in fd-based uprobe")
> */
> - man->has_sema_refcnt = access(ref_ctr_sysfs_path, F_OK) == 0;
> + man->has_sema_refcnt = stat(ref_ctr_sysfs_path, &sb) == 0;
>
> return man;
> }
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-09-23 21:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-21 8:40 [PATCH bpf-next v2] libbpf: Fix the case of running as non-root with capabilities Jon Doron
2022-09-23 21:10 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox