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(ebpf): use ts as fd_arg_path_map key #3674

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Nov 6, 2023

Close: #3609

1. Explain what the PR does

9588651 fix(ebpf): use ts as fd_arg_path_map key

A race condition has been reported when sequential open/close calls
cause the same FD (part of the map key) to be reused, therefore causing
value corruption.

The fd_arg_path_map continues as BPF_LRU_HASH just to avoid the need to
delete entries in userland even though the map is not used as a LRU
anymore - just as "oldest inserted", since there are no subsequent
accesses to update the LRU status of any entry.

P.S.: Another solution would be to use the inode number as part of the
key, but this value would not be available in the event at decode stage.

2. Explain how to test it

sudo ./dist/tracee -s comm=openclose -e 'openat,close' -o option:parse-arguments-fds --capabilities bypass=true

sudo ./dist/tracee -s comm=openclose -e 'openat,close' -o option:parse-arguments-fds --capabilities bypass=true
TIME             UID    COMM             PID     TID     RET              EVENT                     ARGS
15:06:30:629712  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: /etc/ld.so.cache, flags: O_RDONLY|O_CLOEXEC, mode: 0
15:06:30:629748  1000   openclose        131340  131340  0                close                     fd: 3=/etc/ld.so.cache
15:06:30:629783  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: /usr/lib/libc.so.6, flags: O_RDONLY|O_CLOEXEC, mode: 0
15:06:30:629876  1000   openclose        131340  131340  0                close                     fd: 3=/usr/lib/libc.so.6
15:06:30:630163  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_2088142222.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630269  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_2088142222.txt
15:06:30:630298  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_1423438580.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630331  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_1423438580.txt
15:06:30:630344  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_1290108987.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630372  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_1290108987.txt
15:06:30:630384  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_1633928993.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630413  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_1633928993.txt
15:06:30:630426  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_939221181.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630459  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_939221181.txt
15:06:30:630472  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_73418372.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630500  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_73418372.txt
15:06:30:630513  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_1706344463.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630542  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_1706344463.txt
15:06:30:630558  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_1477295567.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630588  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_1477295567.txt
15:06:30:630601  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_1347454283.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630629  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_1347454283.txt
15:06:30:630642  1000   openclose        131340  131340  3                openat                    dirfd: -100, pathname: file_1857739312.txt, flags: O_WRONLY|O_CREAT|O_TRUNC, mode: 438
15:06:30:630674  1000   openclose        131340  131340  0                close                     fd: 3=/home/gg/oc/file_1857739312.txt
^C
End of events stream
Stats: {EventCount:{value:24} EventsFiltered:{value:0} NetCapCount:{value:0} BPFLogsCount:{value:0} ErrorCount:{value:0} LostEvCount:{value:0} LostWrCount:{value:0} LostNtCapCount:{value:0} LostBPFLogsCount:{value:0}}

Trigger the events by the PoC provided by @lclin56 in: #3609 (comment)

3. Other comments

A race condition has been reported when sequential open/close calls
cause the same FD (part of the map key) to be reused, therefore causing
value corruption.

The fd_arg_path_map continues as BPF_LRU_HASH just to avoid the need to
delete entries in userland even though the map is not used as a LRU
anymore - just as "oldest inserted", since there are no subsequent
accesses to update the LRU status of any entry.

P.S.: Another solution would be to use the inode number as part of the
key, but this value would not be available in the event at decode stage.
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

LGTM. Much better indexing to avoid collisions. I liked the time reversion function as well.

@rafaeldtinoco rafaeldtinoco merged commit ca5d7eb into aquasecurity:main Nov 14, 2023
31 checks passed
@geyslan geyslan deleted the 3609-fix-race-fd branch November 28, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The file paths parsed by the parse-arguments-fds option are incorrect.
2 participants