FR: TOCTOU mitigation without userspace enter events

[tuminoid]

Motivation

TOCTOU mitigation should be handled on the kernel side to enable disabling of userspace enter events as proposed here.

Feature

As described in the proposal:

The mitigation implemented in this PR (#235) uses enter events. How can we achieve the same result without sending these events to userspace?

For the connect syscall, we can use the tuple parameter in the exit event to avoid issues with TOCTOU. So, the mitigation here is to populate the userspace with information directly from the kernel.

For what concerns the other 4 syscalls, the idea is to hook into the specific tracepoints (e.g., tracepoint/sys_enter_open), collect the necessary parameters, and save them in a map indexed by thread ID. So we use a bpf hash map with thread-id as a key and syscall arguments as a value. This is very similar to what we do today when RAW_TRACEPOINTS are not enabled: https://github.com/falcosecurity/libs/blob/0.19.0/driver/bpf/maps.h#L103 In the exit tracepoint, we can retrieve this information and send only one event to userspace. This drastically reduces the instrumentation time with respect to using the generic sys_enter tracepoint like today. Moreover, we won't send the enter event to userspace but we will merge the information directly in the kernel.

We have some options here, and I'm hoping to discuss the specific implementation design here, before embarking on journey to make it happen:

• use of hash maps per driver (bpf hash maps where relevant, LRU maps in kernel driver, gvisor ??)
• is the mentioned RAW_TRACEPOINT code a good example what is expected
• is some refactoring needed in case we extend the implementation

Alternatives

Alternative or additional work: We could have a configuration flag or a compile time flag to allow opting out of TOCTOU mitigation to allow user choose between performance gain and the mitigation.

Additional context

Linked to configuration flag feature request, if we choose to implement runtime configuration flag for disabling enter events. Link to issue.

[tuminoid]

Ping @terror96, @leogr, @ekoops

[terror96]

These are details, but currently stash_map is defined only when BPF_SUPPORTS_RAW_TRACEPOINTS is not defined. Should we define a new stash_map or just use the already defined hash map also for storing args in creat, open, openat and openat2 entry handlers? However, stash_map is only defined for "legacy" bpf driver and we need to use something similar to other drivers as well in which case it could make sense to define a new, common structure name for all drivers just for storing entry parameters in these special case. Even that the implementation/definition & usage of the structure is most likely a bit different e.g. kmod vs "modern" bpf.

edit: from memory consumption point of view, it would be better to utilize existing structures if possible such as stash_map (bpf), auxmap (modern bpf)... asfaik, for kmod we would need to define a new temp structure.

[leogr]

We could have a configuration flag or a compile time flag to allow opting out of TOCTOU mitigation to allow user choose between performance gain and the mitigation.

I would allow users to opt out only if performance concerns justify this need. So, I'd say to postpone the decision once we have some working implementation.

[leogr]

/milestone 0.22

[poiana]

@leogr: The provided milestone is not valid for this repository. Milestones in this repository: [0.22.0, TBD, next-driver]

Use /milestone clear to clear the milestone.

Details

In response to this:

/milestone 0.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[leogr]

/milestone 0.22.0

[ekoops]

/assign

[terror96]

/assign

[ekoops]

As we are approaching the end of #2427, I would like to revamp this discussion with some thoughts.

As briefly discussed in the mentioned issue and on this issue, the way to keep TOCTTOU mitigation while removing the sys_enter raw tracepoint programs is adding ad-hoc sys_enter tracepoint programs collecting the needed syscall parameters.

Actually, as stated in the proposal, long-term mitigation would involve attaching to internal kernel hooks rather than using the syscall enter hooks, but currently, we only support tracepoints as hook points in our drivers.

Parameters collected on these ad-hoc tracepoints need to be sent to userspace. We have two possible ways of doing so:

1. using some kind of hash map storing the association between the current thread TID and the parameters - in this scenario, the sys_enter tracepoint program would save the parameters it wants to share on this map, and the sys_exit raw_tracepoint program would retrieve them and use to build the whole exit event which will send to userspace
2. keep pushing enter events from the sys_enter tracepoint programs

The first solution would simplify userspace handling logic, but I have some concern on the performance penalties introduced by the operation on the shared hash map and the additional copies to bring the parameters to userspace through the ring buffer on the exit programs side.
The second solution would be easier to implement, but would not benefit of any userspace handling logic simplification.

One final thought is related to the connect events couple, for which we currently have TOCTTOU mitigation.
The current connect exit fillers/programs leverage the user-provided sockaddr to build part of the tuple parameter if these information cannot be gathered from kernel data structures (because they are not present). If we decide to not leverage the user-provided sockaddr anymore, we can avoid introducing the corresponding sys_enter tracepoint program to mitigate the TOCTTOU vulnerability, as we would have all the required trustable information on the exit event.
As we still need to agree on the fact that not using anymore the user-provided sockaddr is the right choice, in my last PR I didn't move forward with this, and I temporarily kept the enter event handling logic.

What are your thoughts on this? @terror96 @falcosecurity/libs-maintainers