Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(drivers): add always a null terminator after args and envs #1800

Merged
merged 5 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions driver/bpf/fillers.h
Original file line number Diff line number Diff line change
Expand Up @@ -1929,6 +1929,13 @@ static __always_inline int bpf_accumulate_argv_or_env(struct filler_data *data,
*args_len = 0;
off = data->state->tail_ctx.curoff;

if(argv == NULL)
{
// we need to put a `\0` otherwise we could read junk data
data->buf[off & SCRATCH_SIZE_HALF] = '\0';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why the CI was failing, if we called execve with argv or 'env NULLwe ended up reading random memory indata->buf` written by the previous process on this CPU.

return PPM_SUCCESS;
}

#pragma unroll
for (j = 0; j < FAILED_ARGS_ENV_ITEMS_MAX; ++j) {
arg = _READ_USER(argv[j]);
Expand Down
18 changes: 18 additions & 0 deletions driver/modern_bpf/helpers/store/auxmap_store_params.h
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,18 @@ static __always_inline void auxmap__store_exe_args_failure(struct auxiliary_map
unsigned long charbuf_pointer = 0;
uint16_t exe_len = 0;

if(array == NULL)
{
/* We need to store both the exe and the args.
* To be compliant with other drivers we send an empty string as exe not a param with len==0.
*/
push__new_character(auxmap->data, &auxmap->payload_pos, '\0');
push__param_len(auxmap->data, &auxmap->lengths_pos, sizeof(char));

auxmap__store_empty_param(auxmap);
return;
}

/* Here we read the pointer to `exe` and we store it */
if(bpf_probe_read_user(&charbuf_pointer, sizeof(charbuf_pointer), &array[0]))
{
Expand Down Expand Up @@ -452,6 +464,12 @@ static __always_inline void auxmap__store_env_failure(struct auxiliary_map *auxm
uint16_t total_len = 0;
uint64_t initial_payload_pos = auxmap->payload_pos;

if(array == NULL)
{
auxmap__store_empty_param(auxmap);
return;
}

for(uint8_t index = 0; index < MAX_CHARBUF_POINTERS; ++index)
{
if(bpf_probe_read_user(&charbuf_pointer, sizeof(charbuf_pointer), &array[index]))
Expand Down
22 changes: 18 additions & 4 deletions driver/ppm_fillers.c
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ int accumulate_argv_or_env(const void __user * argv,
{
int len = 0;
int ret = 0;
const char __user *p;
const char __user * p = NULL;

for (;;) {

Expand Down Expand Up @@ -768,7 +768,14 @@ int accumulate_argv_or_env(const void __user * argv,
argv += sizeof(argv);
}

str_storage[len-1] = '\0';
if(len>0)
{
str_storage[len-1] = '\0';
}
else
{
str_storage[0] = '\0';
}
LucaGuerra marked this conversation as resolved.
Show resolved Hide resolved
return len;
}

Expand All @@ -779,7 +786,7 @@ int compat_accumulate_argv_or_env(compat_uptr_t argv,
{
int len = 0;
int ret = 0;
const char __user *p;
const char __user *p = NULL;
for (;;) {
compat_uptr_t compat_p = 0;

Expand Down Expand Up @@ -816,7 +823,14 @@ int compat_accumulate_argv_or_env(compat_uptr_t argv,
argv += sizeof(argv);
}

str_storage[len-1] = '\0';
if(len>0)
{
str_storage[len-1] = '\0';
}
else
{
str_storage[0] = '\0';
}
return len;
}
#endif
Expand Down
95 changes: 95 additions & 0 deletions test/drivers/test_suites/syscall_exit_suite/execve_x.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,101 @@ TEST(SyscallExit, execveX_failure)
evt_test->assert_num_params_pushed(28);
}

TEST(SyscallExit, execveX_failure_args_env_NULL)
{
auto evt_test = get_syscall_event_test(__NR_execve, EXIT_EVENT);

evt_test->enable_capture();

/*=============================== TRIGGER SYSCALL ===========================*/

char pathname[] = "//args_env_NULL//";
assert_syscall_state(SYSCALL_FAILURE, "execve", syscall(__NR_execve, pathname, NULL, NULL));
int64_t errno_value = -errno;

/*=============================== TRIGGER SYSCALL ===========================*/

evt_test->disable_capture();

evt_test->assert_event_presence();

if(HasFatalFailure())
{
return;
}

evt_test->parse_event();

evt_test->assert_header();

/*=============================== ASSERT PARAMETERS ===========================*/

/* Parameter 1: res (type: PT_ERRNO)*/
evt_test->assert_numeric_param(1, (int64_t)errno_value);

/* Parameter 2: exe (type: PT_CHARBUF) */
/* exe is taken from the args and not from the pathname. */
evt_test->assert_charbuf_param(2, "");

/* Parameter 3: args (type: PT_CHARBUFARRAY) */
evt_test->assert_empty_param(3);

/* Parameter 16: env (type: PT_CHARBUFARRAY) */
evt_test->assert_empty_param(16);

/*=============================== ASSERT PARAMETERS ===========================*/

evt_test->assert_num_params_pushed(28);
}

TEST(SyscallExit, execveX_failure_path_NULL_but_not_args)
{
auto evt_test = get_syscall_event_test(__NR_execve, EXIT_EVENT);

evt_test->enable_capture();

/*=============================== TRIGGER SYSCALL ===========================*/

char pathname[] = "//path_NULL_but_not_args//";
const char *newargv[] = {"", NULL};
const char *newenviron[] = {"", NULL};
assert_syscall_state(SYSCALL_FAILURE, "execve", syscall(__NR_execve, pathname, newargv, newenviron));
int64_t errno_value = -errno;

/*=============================== TRIGGER SYSCALL ===========================*/

evt_test->disable_capture();

evt_test->assert_event_presence();

if(HasFatalFailure())
{
return;
}

evt_test->parse_event();

evt_test->assert_header();

/*=============================== ASSERT PARAMETERS ===========================*/

/* Parameter 1: res (type: PT_ERRNO)*/
evt_test->assert_numeric_param(1, (int64_t)errno_value);

/* Parameter 2: exe (type: PT_CHARBUF) */
evt_test->assert_charbuf_param(2, "");

/* Parameter 3: args (type: PT_CHARBUFARRAY) */
evt_test->assert_empty_param(3);

/* Parameter 16: env (type: PT_CHARBUFARRAY) */
evt_test->assert_charbuf_array_param(16, &newenviron[0]);

/*=============================== ASSERT PARAMETERS ===========================*/

evt_test->assert_num_params_pushed(28);
}

TEST(SyscallExit, execveX_success)
{
auto evt_test = get_syscall_event_test(__NR_execve, EXIT_EVENT);
Expand Down
Loading