new(libsinsp,driver): add evt.is_open_create syscall event field

[mrgian]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

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 syscall

The 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?:

NONE

[poiana]

Welcome @mrgian! It looks like this is your first PR to falcosecurity/libs 🎉

[github-actions]

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

[incertum]

@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?

libs/driver/flags_table.c

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!

[incertum]

One more comment: No worries we will help with the testing and such once we have made a decision about the final approach!

[mrgian]

How about instead of removing a valid flag we add the new / more robust flag to the flags?

Now that I think about it, it can be useful knowing both if open() was called with O_CREAT AND if the file was actually created or not.

Like you suggested, I'm waiting for further feedback before proceeding.

[Andreagit97]

ei @mrgian thank you for this!
yeah i agree with @incertum, having a new flag seems the best way to go in this way we don't lose information about the O_CREAT flag. Maybe we could create a new flag like PPM_FMODE_CREATED or similar...
And yes we need also the implementation in the modern probe and probably also in the open_by_handle_at syscall since it behaves like the others open/openat/openat2

A side note, as far as i know @oheifetz is working on the same issue -> oheifetz@6dccf13

Curious thing about this FMODE_CREATED flag is that this seems to be set only when the open is called with the O_CREAT flag while if we create a new file with O_TMPFILE this flag is not set in the kernel, so probably we need to take this fact in consideration when we will write rules in userspace... to truly understand if a new file is created we probably need both FMODE_CREATED and O_TMPFILE

[incertum]

A side note, as far as i know @oheifetz is working on the same issue -> oheifetz@6dccf13

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?

Curious thing about this FMODE_CREATED flag is that this seems to be set only when the open is called with the O_CREAT flag while if we create a new file with O_TMPFILE this flag is not set in the kernel, so probably we need to take this fact in consideration when we will write rules in userspace... to truly understand if a new file is created we probably need both FMODE_CREATED and O_TMPFILE

Thanks @Andreagit97 yes we need to investigate better.

[mrgian]

A good solution may be something like this:

• getting mode flags from the kernel:

static inline void get_fd_modes(int64_t fd, unsigned long* modes)
{
        ...

	file = fdt->fd[fd];
	*modes = file->f_mode;

        ...
}

• passing modes to open_flags_to_scap

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;
        ...
}

• setting new flag PPM_FMODE_CREATED if modes has FMODE_CREATED:

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 ebf and modern_ebf

I still have to investigate the behavior of FMODE_CREATED and change this logic if needed.

Please let me know what you think.

[incertum]

@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 FMODE_CREATED, that is the most crucial part even :)

Keep flags to original type though u32 ...

[oheifetz]

Hi,

I worked on this for some time and had also added a test to check the new flag (PPM_FMODE_CREATED), it passed several code reviews and reached a point that the remaining code that is needed is to add was the open friends syscalls in the same manner that the open was added, so all syscalls that create a file as a by product will be in the same PR.

@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.

[incertum]

Perfect thanks for replying @oheifetz and agreeing with porting your changes as co-authored commits into this PR 🙏 !

[mrgian]

Hi @oheifetz
I took a look at your commit and your solution is better than mine.
I will integrate your changes as co-authored commits when I have time.

Thank you!

[incertum]

@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!

[mrgian]

Oops, I forgot to store updated flags to the map. Should be ok now.

I didn't do much research about the behavior of FMODE_CREATED, only a few tests.
I can only confirm what @Andreagit97 already said, calling open with O_TMPFILE creates a new temp file but doesn't set FMODE_CREATED in the kernel.
However, from my understanding, calling open with O_TMPFILE creates an unnamed temp file in the specified directory, so calling open with this flag on a file returns an error. Maybe we can rely on this to check if the file was actually created.

[mrgian]

Fixed build on older kernels where FMODE_CREATED had not yet been introduced.
However only the eBPF probe seems to pass e2e tests and I can't figure out why. I might need some help here...

[Andreagit97]

Amazing work thanks @mmat11, @oheifetz 🎉 Some additional points:

1. we need tests in the drivers for this feature (@oheifetz already implemented them IIRC)
2. if we also want the filtercheck in this PR we probably need some tests in sinsp ( this is an example

   libs/userspace/libsinsp/test/events_file.ut.cpp

   Line 109 in 6caaa3c

   TEST_F(sinsp_with_test_input, open_by_handle_at)

   )

• BTW what do we want by this filtercheck? do we need to involve also the CREAT syscall and the O_TMPFILE flag? @incertum @terylt

3. 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 our PPM_O_F_CREATED will be overwritten
   1. 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)
   2. check directly in this function parse_open_openat_creat_exit the presence of PPM_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

[mrgian]

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 libsinsp.
test_file_writes tests are passing with bpf, but not with modern-bfp and kmod, so this makes me guess it's a problem related to the drivers.

Amazing work thanks @mmat11, @oheifetz 🎉 Some additional points:

BTW, I don't know if @mmat11 is involved too, but I suppose you got my github handle wrong 🙄

[incertum]

/approve

[poiana]

LGTM label has been added.

Git tree hash: 8164b3f38b87219c4f7c7d1121b80f421c133807

[FedeDP]

Great job!
/approve

[poiana]

[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:

• OWNERS [Andreagit97,FedeDP,incertum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Andreagit97]

/unhold