| Message ID | 20201116232536.1752908-2-kpsingh@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2,1/2] bpf: Add bpf_lsm_set_bprm_opts helper | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | fail | Was 0 now: 1 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis CHECK: Blank lines aren't necessary after an open brace '{' WARNING: 'occured' may be misspelled - perhaps 'occurred'? WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Nov 16, 2020 at 11:25:36PM +0000, KP Singh wrote: > From: KP Singh <kpsingh@google.com> > > The test forks a child process, updates the local storage to set/unset > the securexec bit. > > The BPF program in the test attaches to bprm_creds_for_exec which checks > the local storage of the current task to set the secureexec bit on the > binary parameters (bprm). > > The child then execs a bash command with the environment variable > TMPDIR set in the envp. The bash command returns a different exit code > based on its observed value of the TMPDIR variable. > > Since TMPDIR is one of the variables that is ignored by the dynamic > loader when the secureexec bit is set, one should expect the > child execution to not see this value when the secureexec bit is set. > > Signed-off-by: KP Singh <kpsingh@google.com> > --- > .../selftests/bpf/prog_tests/test_bprm_opts.c | 124 ++++++++++++++++++ > tools/testing/selftests/bpf/progs/bprm_opts.c | 34 +++++ > 2 files changed, 158 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > create mode 100644 tools/testing/selftests/bpf/progs/bprm_opts.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > new file mode 100644 > index 000000000000..cba1ef3dc8b4 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > @@ -0,0 +1,124 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Copyright (C) 2020 Google LLC. > + */ > + > +#include <asm-generic/errno-base.h> > +#include <sys/stat.h> Is it needed? > +#include <test_progs.h> > +#include <linux/limits.h> > + > +#include "bprm_opts.skel.h" > +#include "network_helpers.h" > + > +#ifndef __NR_pidfd_open > +#define __NR_pidfd_open 434 > +#endif > + > +static const char * const bash_envp[] = { "TMPDIR=shouldnotbeset", NULL }; > + > +static inline int sys_pidfd_open(pid_t pid, unsigned int flags) > +{ > + return syscall(__NR_pidfd_open, pid, flags); > +} > + > +static int update_storage(int map_fd, int secureexec) > +{ > + int task_fd, ret = 0; > + > + task_fd = sys_pidfd_open(getpid(), 0); > + if (task_fd < 0) > + return errno; > + > + ret = bpf_map_update_elem(map_fd, &task_fd, &secureexec, BPF_NOEXIST); > + if (ret) > + ret = errno; > + > + close(task_fd); > + return ret; > +} > + > +static int run_set_secureexec(int map_fd, int secureexec) > +{ > + > + int child_pid, child_status, ret, null_fd; > + > + child_pid = fork(); > + if (child_pid == 0) { > + null_fd = open("/dev/null", O_WRONLY); > + if (null_fd == -1) > + exit(errno); > + dup2(null_fd, STDOUT_FILENO); > + dup2(null_fd, STDERR_FILENO); > + close(null_fd); > + > + /* Ensure that all executions from hereon are > + * secure by setting a local storage which is read by > + * the bprm_creds_for_exec hook and sets bprm->secureexec. > + */ > + ret = update_storage(map_fd, secureexec); > + if (ret) > + exit(ret); > + > + /* If the binary is executed with securexec=1, the dynamic > + * loader ingores and unsets certain variables like LD_PRELOAD, > + * TMPDIR etc. TMPDIR is used here to simplify the example, as > + * LD_PRELOAD requires a real .so file. > + * > + * If the value of TMPDIR is set, the bash command returns 10 > + * and if the value is unset, it returns 20. > + */ > + ret = execle("/bin/bash", "bash", "-c", > + "[[ -z \"${TMPDIR}\" ]] || exit 10 && exit 20", > + NULL, bash_envp); > + if (ret) It should never reach here? May be just exit() unconditionally instead of having a chance to fall-through and then return -EINVAL. > + exit(errno); > + } else if (child_pid > 0) { > + waitpid(child_pid, &child_status, 0); > + ret = WEXITSTATUS(child_status); > + > + /* If a secureexec occured, the exit status should be 20. > + */ > + if (secureexec && ret == 20) > + return 0; > + > + /* If normal execution happened the exit code should be 10. > + */ > + if (!secureexec && ret == 10) > + return 0; > + > + return ret; Any chance that ret may be 0? > + } > + > + return -EINVAL; > +} > + > +void test_test_bprm_opts(void) > +{ > + int err, duration = 0; > + struct bprm_opts *skel = NULL; > + > + skel = bprm_opts__open_and_load(); > + if (CHECK(!skel, "skel_load", "skeleton failed\n")) > + goto close_prog; > + > + err = bprm_opts__attach(skel); > + if (CHECK(err, "attach", "attach failed: %d\n", err)) > + goto close_prog; > + > + /* Run the test with the secureexec bit unset */ > + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map), > + 0 /* secureexec */); > + if (CHECK(err, "run_set_secureexec:0", "err = %d", err)) nit. err = %d"\n" > + goto close_prog; > + > + /* Run the test with the secureexec bit set */ > + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map), > + 1 /* secureexec */); > + if (CHECK(err, "run_set_secureexec:1", "err = %d", err)) Same here. Others LGTM.
On Tue, Nov 17, 2020 at 1:43 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Mon, Nov 16, 2020 at 11:25:36PM +0000, KP Singh wrote: > > From: KP Singh <kpsingh@google.com> > > > > The test forks a child process, updates the local storage to set/unset > > the securexec bit. > > > > The BPF program in the test attaches to bprm_creds_for_exec which checks > > the local storage of the current task to set the secureexec bit on the > > binary parameters (bprm). > > > > The child then execs a bash command with the environment variable > > TMPDIR set in the envp. The bash command returns a different exit code > > based on its observed value of the TMPDIR variable. > > > > Since TMPDIR is one of the variables that is ignored by the dynamic > > loader when the secureexec bit is set, one should expect the > > child execution to not see this value when the secureexec bit is set. > > > > Signed-off-by: KP Singh <kpsingh@google.com> > > --- > > .../selftests/bpf/prog_tests/test_bprm_opts.c | 124 ++++++++++++++++++ > > tools/testing/selftests/bpf/progs/bprm_opts.c | 34 +++++ > > 2 files changed, 158 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > > create mode 100644 tools/testing/selftests/bpf/progs/bprm_opts.c > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > > new file mode 100644 > > index 000000000000..cba1ef3dc8b4 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > > @@ -0,0 +1,124 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* > > + * Copyright (C) 2020 Google LLC. > > + */ > > + > > +#include <asm-generic/errno-base.h> > > +#include <sys/stat.h> > Is it needed? No, Good catch, removed. > > > +#include <test_progs.h> [...] > > + * If the value of TMPDIR is set, the bash command returns 10 > > + * and if the value is unset, it returns 20. > > + */ > > + ret = execle("/bin/bash", "bash", "-c", > > + "[[ -z \"${TMPDIR}\" ]] || exit 10 && exit 20", > > + NULL, bash_envp); > > + if (ret) > It should never reach here? May be just exit() unconditionally > instead of having a chance to fall-through and then return -EINVAL. Agreed. changed it to exit(errno); here. > > > + exit(errno); > > + } else if (child_pid > 0) { > > + waitpid(child_pid, &child_status, 0); > > + ret = WEXITSTATUS(child_status); > > + > > + /* If a secureexec occured, the exit status should be 20. > > + */ > > + if (secureexec && ret == 20) > > + return 0; > > + > > + /* If normal execution happened the exit code should be 10. > > + */ > > + if (!secureexec && ret == 10) > > + return 0; > > + > > + return ret; > Any chance that ret may be 0? I think it's safer to just let it fall through and return -EINVAL, so I removed the return ret here. > > > + } [...] > > + 0 /* secureexec */); > > + if (CHECK(err, "run_set_secureexec:0", "err = %d", err)) > nit. err = %d"\n" Fixed. > > > + goto close_prog; > > + > > + /* Run the test with the secureexec bit set */ > > + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map), > > + 1 /* secureexec */); > > + if (CHECK(err, "run_set_secureexec:1", "err = %d", err)) > Same here. Fixed. - KP > > Others LGTM.
diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c new file mode 100644 index 000000000000..cba1ef3dc8b4 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2020 Google LLC. + */ + +#include <asm-generic/errno-base.h> +#include <sys/stat.h> +#include <test_progs.h> +#include <linux/limits.h> + +#include "bprm_opts.skel.h" +#include "network_helpers.h" + +#ifndef __NR_pidfd_open +#define __NR_pidfd_open 434 +#endif + +static const char * const bash_envp[] = { "TMPDIR=shouldnotbeset", NULL }; + +static inline int sys_pidfd_open(pid_t pid, unsigned int flags) +{ + return syscall(__NR_pidfd_open, pid, flags); +} + +static int update_storage(int map_fd, int secureexec) +{ + int task_fd, ret = 0; + + task_fd = sys_pidfd_open(getpid(), 0); + if (task_fd < 0) + return errno; + + ret = bpf_map_update_elem(map_fd, &task_fd, &secureexec, BPF_NOEXIST); + if (ret) + ret = errno; + + close(task_fd); + return ret; +} + +static int run_set_secureexec(int map_fd, int secureexec) +{ + + int child_pid, child_status, ret, null_fd; + + child_pid = fork(); + if (child_pid == 0) { + null_fd = open("/dev/null", O_WRONLY); + if (null_fd == -1) + exit(errno); + dup2(null_fd, STDOUT_FILENO); + dup2(null_fd, STDERR_FILENO); + close(null_fd); + + /* Ensure that all executions from hereon are + * secure by setting a local storage which is read by + * the bprm_creds_for_exec hook and sets bprm->secureexec. + */ + ret = update_storage(map_fd, secureexec); + if (ret) + exit(ret); + + /* If the binary is executed with securexec=1, the dynamic + * loader ingores and unsets certain variables like LD_PRELOAD, + * TMPDIR etc. TMPDIR is used here to simplify the example, as + * LD_PRELOAD requires a real .so file. + * + * If the value of TMPDIR is set, the bash command returns 10 + * and if the value is unset, it returns 20. + */ + ret = execle("/bin/bash", "bash", "-c", + "[[ -z \"${TMPDIR}\" ]] || exit 10 && exit 20", + NULL, bash_envp); + if (ret) + exit(errno); + } else if (child_pid > 0) { + waitpid(child_pid, &child_status, 0); + ret = WEXITSTATUS(child_status); + + /* If a secureexec occured, the exit status should be 20. + */ + if (secureexec && ret == 20) + return 0; + + /* If normal execution happened the exit code should be 10. + */ + if (!secureexec && ret == 10) + return 0; + + return ret; + } + + return -EINVAL; +} + +void test_test_bprm_opts(void) +{ + int err, duration = 0; + struct bprm_opts *skel = NULL; + + skel = bprm_opts__open_and_load(); + if (CHECK(!skel, "skel_load", "skeleton failed\n")) + goto close_prog; + + err = bprm_opts__attach(skel); + if (CHECK(err, "attach", "attach failed: %d\n", err)) + goto close_prog; + + /* Run the test with the secureexec bit unset */ + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map), + 0 /* secureexec */); + if (CHECK(err, "run_set_secureexec:0", "err = %d", err)) + goto close_prog; + + /* Run the test with the secureexec bit set */ + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map), + 1 /* secureexec */); + if (CHECK(err, "run_set_secureexec:1", "err = %d", err)) + goto close_prog; + +close_prog: + bprm_opts__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/bprm_opts.c b/tools/testing/selftests/bpf/progs/bprm_opts.c new file mode 100644 index 000000000000..f353b4fdc0b7 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/bprm_opts.c @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright 2020 Google LLC. + */ + +#include "vmlinux.h" +#include <errno.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +char _license[] SEC("license") = "GPL"; + +struct { + __uint(type, BPF_MAP_TYPE_TASK_STORAGE); + __uint(map_flags, BPF_F_NO_PREALLOC); + __type(key, int); + __type(value, int); +} secure_exec_task_map SEC(".maps"); + +SEC("lsm/bprm_creds_for_exec") +int BPF_PROG(secure_exec, struct linux_binprm *bprm) +{ + int *secureexec; + + secureexec = bpf_task_storage_get(&secure_exec_task_map, + bpf_get_current_task_btf(), 0, + BPF_LOCAL_STORAGE_GET_F_CREATE); + + if (secureexec && *secureexec) + bpf_lsm_set_bprm_opts(bprm, BPF_LSM_F_BPRM_SECUREEXEC); + + return 0; +}