fix(driver, userspace): fix `loginuid`, `euid` and `tty` types to uint32_t by incertum · Pull Request #1192 · falcosecurity/libs

incertum · 2023-07-10T04:29:04Z

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:

loginuid shall be treated as uint32_t throughout to avoid wonky audit uids (which is currently the case in production settings). In order to keep backward compatibility change user.loginuid filtercheck field to int64_t and return -1 if the uid is invalid.

TODO: Need to bump the schema version once we have decided when to merge this PR.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(driver, userspace): fix loginuid, euid and tty types to uint32_t

github-actions · 2023-07-10T04:29:53Z

Please double check driver/API_VERSION file. See versioning.

incertum · 2023-07-10T04:33:38Z

@FedeDP inspected the other PR again #526 and I think we cannot port changes to ERRNO or other fields into this PR as we would need to create so many new events ... almost wonder if it is time to introduce compatibility versioning for scap files since it is a large overhead for all of us to maintain compatibility and/or work around limitations.

Forgot about test running the e2e tests on my box, will address tomorrow ...

incertum · 2023-07-14T15:11:21Z

Edited:

Yes let's also address tty mentioned in #515 @Andreagit97 and re the cwd I am seeing that relying on the syscalls that do send cwd is suboptimal in production, this is why for example we introduced euid recently as to not needing to rely on setuid. Therefore, I would rather propose we go back and actually send cwd, because execve* syscalls are too important and I mentioned multiple times that in the grand schema of things those are not the frequent and noisy ones we need to worry about in most modern infrastructures.

Note that the loginuid stands out as very critical for the Falco use cases.

FedeDP · 2023-07-17T08:32:23Z

/milestone driver-backlog

Andreagit97

uhm I have a major question, why do we craft new events? Apart from the hard-coded rules we imposed on ourselves do we have some real reasons? In the end, it would be enough to handle both loginuid=-1 and loginuid=UINT32_MAX in userspace, or am I missing something @FedeDP @gnosek? Let's say I would love to not change all the events every time we need to change the type of a param...

Re cwd: I would be on board to add it but not sure about the cost of potentially long string in the buffer 🤔 The issue is always the same we have no idea about perf consequence :/

driver/bpf/fillers.h

Andreagit97 · 2023-07-18T08:32:33Z

driver/bpf/fillers.h

 #define UID_GID_MAP_MAX_BASE_EXTENTS 5
 #endif

+#ifndef INVALID_UID


I would put it somewhere shared between all the drivers, something like ppm_events_public.h

probably to avoid every possible collision with COS definition I would call it something like INVALID_LOGINUID

We should probably just very specifically use UINT32_MAX everywhere, do we actually need below fallback check?

#ifndef UINT32_MAX #define UINT32_MAX (4294967295U) #endif

Alternatively we can go back to consistently just case -1 to uint32_t, not much of an opinion here.

driver/bpf/fillers.h

Andreagit97 · 2023-07-18T08:55:55Z

userspace/libpman/src/events_prog_names.h

 	[PPME_SYSCALL_MEMFD_CREATE_X] =  "memfd_create_x",
 	[PPME_SYSCALL_PIDFD_GETFD_E] = "pidfd_getfd_e",
 	[PPME_SYSCALL_PIDFD_GETFD_X] = "pidfd_getfd_x",
+	[PPME_SYSCALL_EXECVE_20_E] = "execve_e",


here we need to replace the old execve events instead of adding new entries

incertum · 2023-07-18T21:53:20Z

@Andreagit97 we need new events because of the scap files, see #526 ... read my comment above I think we should introduce scap files versioning so that we can for once fix all types without needing to redefine each event which is also a significant code review overhead ... #1192 (comment)

We may need the execve fix earlier as the loginuid fix is more urgent for Falco use cases, but for all other outstanding fixes I would also much more prefer not to create new events.

incertum · 2023-07-28T23:49:44Z

@Andreagit97 this is ready for a fresh look now, also thanks again for the drivers tests, caught one little error while redoing it :)

Andreagit97

Apart from the INVALID_UID fix we should be fine

driver/bpf/fillers.h

test/e2e/README.md

userspace/libsinsp/test/events_proc.ut.cpp

Andreagit97 · 2023-08-01T10:17:59Z

driver/bpf/fillers.h

 #endif /* COS_73_WORKAROUND */
 #else
-		loginuid.val = UINT32_MAX;
+		loginuid.val = INVALID_UID;


actually loginuid.val = UINT32_MAX was right, loginuid is a kuid_t so when we use just loginuid we need to assign INVALID_UID while when we use loginuid.val we need to assign an uint32 so UINT32_MAX

Same in the other loginuid.val usage

Andreagit97

/approve

poiana · 2023-08-01T16:20:45Z

LGTM label has been added.

Details

Git tree hash: 54658f25e6073fefcb8221f654daac3e72e83f52

userspace/libsinsp/filterchecks.cpp

FedeDP · 2023-08-02T07:27:49Z

/hold

* loginuid, tty and euid now consistently uint32_t * not a breaking UX change since we still expose -1 in case of an invalid uid in the filterchecks, user.loginuid now int64_t Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

FedeDP

/approve

FedeDP · 2023-08-02T15:16:02Z

/unhold

poiana · 2023-08-02T15:16:04Z

LGTM label has been added.

Details

Git tree hash: c75a0f78e2af355ae3a881f806e9df4e52054a1b

Andreagit97

/approve

poiana · 2023-08-02T15:19:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, incertum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

poiana requested a review from hbrueckner July 10, 2023 04:29

poiana added the area/libpman label Jul 10, 2023

poiana requested a review from jasondellaluce July 10, 2023 04:29

poiana added area/libsinsp area/tests size/XL labels Jul 10, 2023

incertum mentioned this pull request Jul 14, 2023

[Tracking] Params inconsistencies in our drivers #515

Closed

59 tasks

poiana added size/XXL and removed size/XL labels Jul 14, 2023

incertum changed the title ~~fix(driver, userspace): fix loginuid types, new execve* events~~ fix(driver, userspace): fix loginuid, euid and tty types, new execve* events Jul 14, 2023

poiana added this to the driver-backlog milestone Jul 17, 2023

Andreagit97 reviewed Jul 18, 2023

View reviewed changes

poiana added the size/L label Jul 28, 2023

incertum changed the title ~~fix(driver, userspace): fix loginuid, euid and tty types, new execve* events~~ fix(driver, userspace): fix loginuid, euid and tty types to uint32_t Jul 28, 2023

Andreagit97 reviewed Jul 31, 2023

View reviewed changes

driver/bpf/fillers.h Outdated Show resolved Hide resolved

driver/bpf/fillers.h Outdated Show resolved Hide resolved

test/e2e/README.md Show resolved Hide resolved

userspace/libsinsp/test/events_proc.ut.cpp Show resolved Hide resolved

Andreagit97 reviewed Aug 1, 2023

View reviewed changes

Andreagit97 previously approved these changes Aug 1, 2023

View reviewed changes

poiana assigned Andreagit97 Aug 1, 2023

poiana added the lgtm label Aug 1, 2023

FedeDP reviewed Aug 2, 2023

View reviewed changes

userspace/libsinsp/filterchecks.cpp Show resolved Hide resolved

poiana added the do-not-merge/hold label Aug 2, 2023

incertum dismissed Andreagit97’s stale review via 38993eb August 2, 2023 15:10

poiana removed the lgtm label Aug 2, 2023

poiana requested a review from Andreagit97 August 2, 2023 15:10

incertum and others added 5 commits August 2, 2023 15:14

fix(driver): fix INVALID_UID for loginuid in bpf driver

472884a

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(test): more libsinsp unit test coverage for loginuid

c4b8982

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

fix(driver): revert/use UINT32_MAX for loginuid.val in bpf driver

c88685f

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup: fix remaining tty uint32_t change inconsistencies

edb2cd6

Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

FedeDP approved these changes Aug 2, 2023

View reviewed changes

poiana assigned FedeDP Aug 2, 2023

poiana added the lgtm label Aug 2, 2023

poiana removed the do-not-merge/hold label Aug 2, 2023

Andreagit97 approved these changes Aug 2, 2023

View reviewed changes

poiana merged commit a3d92ab into falcosecurity:master Aug 2, 2023

incertum deleted the loginuid-type branch December 8, 2023 20:37

Conversation

incertum commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incertum commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incertum commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FedeDP commented Jul 17, 2023

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andreagit97 Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

Andreagit97 Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

incertum Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andreagit97 Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

incertum commented Jul 18, 2023

Uh oh!

incertum commented Jul 28, 2023

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Andreagit97 Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

Andreagit97 Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Aug 1, 2023

Uh oh!

Uh oh!

FedeDP commented Aug 2, 2023

Uh oh!

FedeDP left a comment

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Aug 2, 2023

Uh oh!

poiana commented Aug 2, 2023

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Aug 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

incertum commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 10, 2023 •

edited

Loading

incertum commented Jul 10, 2023 •

edited

Loading

incertum commented Jul 14, 2023 •

edited

Loading