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.

Sign up for GitHub

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(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

Merged
merged 5 commits into from
Oct 13, 2022
+18 −16
Merged

fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9d796ac
cleanup(driver-modern-bpf): minor refactor of push__bytebuf to reflec…
incertum Sep 26, 2022
df17037
cleanup(driver-modern-bpf): enforce upper bounds for push__bytebuf
incertum Sep 26, 2022
a94d950
fix(driver-modern-bpf): fix USER vs MEMORY attribution in execve filler
incertum Sep 26, 2022
7bf2082
cleanup(driver-modern-bpf): skip push__bytebuf reads when no valid le…
incertum Sep 26, 2022
1a388da
cleanup(driver-modern-bpf): start moving bound checks to auxmap__store*
incertum Oct 4, 2022
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
44 changes: 30 additions & 14 deletions driver/modern_bpf/helpers/base/push_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ enum read_memory
/* Paramater maximum size */
#define MAX_PARAM_SIZE MAX_EVENT_SIZE - 1

/* ARGS or ENV maximum size */
#define ARGS_ENV_SIZE_MAX 4096
Copy link
Member

Choose a reason for hiding this comment

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

not completely understood why we need this 🤔

Copy link
Member

Choose a reason for hiding this comment

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

SAFE_ACCESS should already protect us from the worst case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been part of the old probe and it may be less for protection and rather more for you don't wanna ever log super super long unbounded strings in your logs. @leogr @FedeDP do you have more insights re what the motivation was in the old probe? Only safe reads or also some guarantees on limits? Lastly for push__charbuf limits are enforced so it would also be about consistency.

Copy link
Member

Choose a reason for hiding this comment

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

It has been part of the old probe and it may be less for protection and rather more for you don't wanna ever log super super long unbounded strings in your logs.

That's a good point for this reason we use approaches like the snaplen to let users understand the max dimension they want to send, you can see an example here

libs/driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/sendto.bpf.c

Lines 89 to 98 in 0c280ca

unsigned long bytes_to_read = maps__get_snaplen();
if(bytes_to_read > ret)
{
bytes_to_read = ret;
}
/* Parameter 2: data (type: PT_BYTEBUF) */
unsigned long sent_data_pointer = extract__syscall_argument(regs, 1);
auxmap__store_bytebuf_param(auxmap, sent_data_pointer, bytes_to_read, USER);

The idea of this snaplen concept is to limit at an higher level so you can chose when to limit rather then implementing it directly at low level

For what concerns the old probe we use this same concept of snaplen all the others checks are to please the verifier AFAIK 🤔

Copy link
Contributor Author

@incertum incertum Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Those are the two relevant max bounds from old probe

#define ARGS_ENV_SIZE_MAX 4096
#define MAX_PATH_LENGTH 4096

These values are battle proven, so wouldn't go higher before testing modern_bpf in the field.

Probably have a few options here and we can go step-wise, no need to change all now.

  • Could start with uniformly enforcing bounds to existing MAX_PARAM_SIZE, if I can say so, the value appears extremely large, I think 2^12 is reasonable and enough and as mentioned battle proven. If we go this route can change this quickly and reuse the existing macro.
  • If we need more granular upper bounds can introduce a new enum, similarly to the read_memory. For example, for the cmd args I can see that maybe 2^13 could be a good choice for an enforced upper bound. Would however perform all bound checks and selecting the corresponding upper bound here and not in the fillers. In addition, fillers can always also enforce a lower value than the max enforced bound.

These bound checks are not unique to Falco or libs, seeing them everywhere and I would be very hesitant deploying anything to production without them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry typo meant the MAX_PARAM_SIZE you also use in SAFE_ACCESS

Copy link
Member

Choose a reason for hiding this comment

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

uhm I got your point maybe MAX_PARAM_SIZE is big enough, it's the same value we use in the old probe but it could be dangerous...

/* Event maximum size */
#define MAX_EVENT_SIZE 64 * 1024

/* Paramater maximum size */
#define MAX_PARAM_SIZE MAX_EVENT_SIZE - 1

BTW putting a hard-coded value here doesn't seem the right solution, or better let's say it could be the right solution if we were in production and we have to rush, but this is not the case 🤣 let's take our time to think of something more dynamic or configurable

Copy link
Member

Choose a reason for hiding this comment

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

I've added some dedicated checks for execve here https://github.com/falcosecurity/libs/pull/648/files#diff-23e1e00c90562960e7e4110fcc5c62cd89d2ea8a8df3198d7c04512084c7c378R52-R54 :)


/* Helper used to please the verifier during reading
* operations like `bpf_probe_read_str()`.
*/
Expand Down Expand Up @@ -253,27 +256,40 @@ static __always_inline u16 push__charbuf(u8 *data, u64 *payload_pos, unsigned lo
*/
static __always_inline u16 push__bytebuf(u8 *data, u64 *payload_pos, unsigned long bytebuf_pointer, u16 len_to_read, enum read_memory mem)
{

int written_bytes = 0;

if(mem == KERNEL)
u16 total_len = 0;
if (len_to_read < 1)
{
return 0;
}
else if (len_to_read > ARGS_ENV_SIZE_MAX)
{
written_bytes = bpf_probe_read_kernel(&data[SAFE_ACCESS(*payload_pos)],
len_to_read,
(char *)bytebuf_pointer);
total_len = ARGS_ENV_SIZE_MAX;
}
else
{
written_bytes = bpf_probe_read_user(&data[SAFE_ACCESS(*payload_pos)],
len_to_read,
(char *)bytebuf_pointer);
total_len = len_to_read;
}

if(written_bytes != 0)
if(mem == KERNEL)
{
return 0;
if(bpf_probe_read_kernel(&data[SAFE_ACCESS(*payload_pos)],
total_len,
(void *)bytebuf_pointer) != 0)
{
return 0;
}

}
else
{
if(bpf_probe_read_user(&data[SAFE_ACCESS(*payload_pos)],
total_len,
(void *)bytebuf_pointer) != 0)
{
return 0;
}
}

*payload_pos += len_to_read;
return len_to_read;
*payload_pos += total_len;
return total_len;
}
6 changes: 3 additions & 3 deletions driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,15 @@ int BPF_PROG(execve_x,
/* We need to extract the len of `exe` arg so we can undestand
* the overall length of the remaining args.
*/
u16 exe_arg_len = auxmap__store_charbuf_param(auxmap, arg_start_pointer, KERNEL);
u16 exe_arg_len = auxmap__store_charbuf_param(auxmap, arg_start_pointer, USER);

/* Parameter 3: args (type: PT_CHARBUFARRAY) */
/* Here we read the whole array starting from the pointer to the first
* element. We could also read the array element per element but
* since we know the total len we read it as a `bytebuf`.
* The `\0` after every argument are preserved.
*/
auxmap__store_bytebuf_param(auxmap, arg_start_pointer + exe_arg_len, total_args_len - exe_arg_len, KERNEL);
auxmap__store_bytebuf_param(auxmap, arg_start_pointer + exe_arg_len, total_args_len - exe_arg_len, USER);
}
else
{
Expand Down Expand Up @@ -203,7 +203,7 @@ int BPF_PROG(t1_execve_x,
* since we know the total len we read it as a `bytebuf`.
* The `\0` after every argument are preserved.
*/
auxmap__store_bytebuf_param(auxmap, env_start_pointer, total_env_len, KERNEL);
auxmap__store_bytebuf_param(auxmap, env_start_pointer, total_env_len, USER);
}
else
{
Expand Down