-
Notifications
You must be signed in to change notification settings - Fork 164
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
new(libsinsp,driver): add evt.is_open_create syscall event field #1299
Conversation
Welcome @mrgian! It looks like this is your first PR to falcosecurity/libs 🎉 |
Please double check driver/SCHEMA_VERSION file. See versioning. /hold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrgian this is a really nice PR 🤩 thank you!
Left few minor comments.
In addition, proposing a slightly different approach. At the same time it would be good to first gather feedback from the second reviewer before jumping into it. @Andreagit97 would you be available since many of us are on vacation at the moment?
How about instead of removing a valid flag we add the new / more robust flag to the flags?
Line 72 in b607089
{"O_TMPFILE", PPM_O_TMPFILE}, |
{"FMODE_CREATED", PPM_FMODE_CREATED},
and perform all operations within the existing open_flags_to_scap
?
Subsequently we could benefit from updating the relevant Falco upstream rules that check for evt.arg.flags contains O_CREAT
https://github.com/falcosecurity/rules/blob/main/rules/
And we now always get to do the fun three times, the modern_bpf
implementation would be needed as well. Thank you!
One more comment: No worries we will help with the testing and such once we have made a decision about the final approach! |
Now that I think about it, it can be useful knowing both if open() was called with Like you suggested, I'm waiting for further feedback before proceeding. |
ei @mrgian thank you for this! A side note, as far as i know @oheifetz is working on the same issue -> oheifetz@6dccf13 Curious thing about this |
Oh I did not know as it wasn't an open PR yet, @oheifetz and @mrgian could you somehow work together and merge these changes into one PR with co-authored commits? If ok with you @oheifetz we could use this current open PR and add your changes as co-authored on top?
Thanks @Andreagit97 yes we need to investigate better. |
A good solution may be something like this:
static inline void get_fd_modes(int64_t fd, unsigned long* modes)
{
...
file = fdt->fd[fd];
*modes = file->f_mode;
...
}
int f_sys_open_x(struct event_filler_arguments *args)
{
...
get_fd_modes(&modes);
syscall_get_arguments_deprecated(args, 2, 1, &flags);
res = val_to_ring(args, open_flags_to_scap(flags, modes), 0, false, 0);
if (unlikely(res != PPM_SUCCESS))
return res;
...
}
static __always_inline u32 open_flags_to_scap(unsigned long flags, unsigned long modes)
{
...
if (modes & FMODE_CREATED)
res |= PPM_FMODE_CREATED;
...
} And then obviously implement the same for I still have to investigate the behavior of Please let me know what you think. |
@mrgian yes splitting the lookups is something we do in other places and ok from my perspective. Please co-author those changes with @oheifetz oheifetz@6dccf13 Thanks in advance for looking more into the behavior of Keep flags to original type though u32 ... |
Hi, I worked on this for some time and had also added a test to check the new flag ( @mrgian, you can look at my commit and take any code that makes sense to you. I gather that there is no need for two PRs. |
Perfect thanks for replying @oheifetz and agreeing with porting your changes as co-authored commits into this PR 🙏 ! |
Hi @oheifetz Thank you! |
@mrgian I wanted to update the README for a long time and never got to it. Would you mind checking the new instructions out https://github.com/falcosecurity/libs/blob/0f041148624e1c3c1c401fa58a04cae0e4897571/README.md and giving all the tests a try? Currently a few things are still missing, that is why CI is failing. Proposing you take a look first and then I'll review again and also happy to help! Thanks! |
Oops, I forgot to store updated flags to the map. Should be ok now. I didn't do much research about the behavior of |
Fixed build on older kernels where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work thanks @mmat11, @oheifetz 🎉 Some additional points:
- we need tests in the drivers for this feature (@oheifetz already implemented them IIRC)
- if we also want the filtercheck in this PR we probably need some tests in sinsp ( this is an example
TEST_F(sinsp_with_test_input, open_by_handle_at)
- BTW what do we want by this
filtercheck
? do we need to involve also theCREAT
syscall and theO_TMPFILE
flag? @incertum @terylt
- Please take a look at this function
parse_open_openat_creat_exit
... due to TOCTOU attacks we have to replace exit event flags with the enter event ones so we have 2 choices otherwise ourPPM_O_F_CREATED
will be overwritten- add this flag in enter fillers in the drivers, right now you have added it only in the exit ones (open_by_handle_at doesn't need this workaround, it's already ok)
- check directly in this function
parse_open_openat_creat_exit
the presence ofPPM_O_F_CREATED
and preserve it before overwriting the flags
Solution 1 is more expensive in terms of perf but for sure is the clearest and future proof so probably I would go with this!
The last point is probably the reason why e2e testing are failing
Ehi @Andreagit97 , I've added helper functions for getting file mode created flag. Code is much cleaner now :) As for the failed e2e tests, I'm not sure it's a problem with
BTW, I don't know if @mmat11 is involved too, but I suppose you got my github handle wrong 🙄 |
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com> Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
- Signed-off-by: Gianmatteo Palmieri <mail@gian.im> Co-authored-by: Ofer Heifetz <oheifetz@gmail.com> Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com> Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com> Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
22eb54b
to
8296493
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 8164b3f38b87219c4f7c7d1121b80f421c133807
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, incertum, mrgian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area libsinsp
/area tests
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Introduces a new event field in the evt class:
evt.is_open_create
is set to true if file was created on open/openat/openat2 syscallThe check is based on syscall flags argument, however you can still call open() with
O_CREAT
flag enabled.So I added an additional check in the driver that unsets this flag if the file was not actually created.
Which issue(s) this PR fixes:
Fixes #1079
Special notes for your reviewer:
Please note that I've only tested the kmod driver.
Does this PR introduce a user-facing change?: