update(driver): handle processes sending open file descriptors via SCM_RIGHTS #1400

loresuso · 2023-10-10T13:09:10Z

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:
When using unix sockets, processes can pass open file descriptors to one another by using sendmsg and recmsg syscalls. This PR attempts to reflect this in the sinsp state. Whenever the recvmsg contains ancillary data, it checks for SCM_RIGHTS. If so, we proceed with a proc scan of the file descriptors of the receiving process, since it will have new open file descriptors that we missed.

Since we only want to update the file descriptors, this PR introduces a new platform API to scan only that and avoid parsing information about the receiving process that we already have in the sinsp state. This, plus the fact that we can consider the frequency of SCM_RIGHTS messages pretty low, should mean that we are not introducing a big performance penalty.

Which issue(s) this PR fixes:
#1364

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

github-actions · 2023-10-10T13:09:36Z

Please double check driver/API_VERSION file. See versioning.

/hold

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

…ern bpf Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

…e descriptors of a process from procfs Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

…meter in recvmsg Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

loresuso · 2023-10-11T13:03:42Z

userspace/libsinsp/parsers.cpp

+			if(etype == PPME_SOCKET_RECVMSG_X && evt->get_num_params() >= 5)
+			{
+				parinfo = evt->get_param(4);
+				if(parinfo->m_len > sizeof(struct cmsghdr))


Ensure there is room for the header and some more payload. In that case, SCM_RIGHTS could effectively have passed open file descriptors from one process to another

loresuso · 2023-10-11T13:06:06Z

userspace/libsinsp/parsers.cpp

+				if(parinfo->m_len > sizeof(struct cmsghdr))
+				{
+					struct cmsghdr *cmsg = (struct cmsghdr *)parinfo->m_val;
+					if(cmsg->cmsg_type == SCM_RIGHTS)


When using SCM_RIGHTS, we expect to have just one header, like in the example at the end of this page: https://linux.die.net/man/3/cmsg

struct msghdr msg = {0}; struct cmsghdr *cmsg; int myfds[NUM_FD]; /* Contains the file descriptors to pass. */ char buf[CMSG_SPACE(sizeof myfds)]; /* ancillary data buffer */ int *fdptr; msg.msg_control = buf; msg.msg_controllen = sizeof buf; cmsg = CMSG_FIRSTHDR(&msg); cmsg->cmsg_level = SOL_SOCKET; cmsg->cmsg_type = SCM_RIGHTS; cmsg->cmsg_len = CMSG_LEN(sizeof(int) * NUM_FD); /* Initialize the payload: */ fdptr = (int *) CMSG_DATA(cmsg); memcpy(fdptr, myfds, NUM_FD * sizeof(int)); /* Sum of the length of all control messages in the buffer: */ msg.msg_controllen = cmsg->cmsg_len;

This is because SCM_RIGHTS offers natively the possibility to pass an array of file descriptors, so we won't need multiple headers. A quick check on the kernel source code also confirmed this.

loresuso · 2023-10-11T13:16:02Z

test/drivers/test_suites/syscall_exit_suite/recvmsg_x.cpp

+
+	/* Parameter 2: size (type: PT_UINT32) */
+	evt_test->assert_numeric_param(2, (uint32_t)FULL_MESSAGE_LEN);
+


I would add the check for parameter 4 here. Something like this:

evt_test->assert_tuple_unix_param(4, PPM_AF_UNIX, UNIX_SERVER);

The problem is that this only works with the modern bpf probe at the moment. I would be curious to solve this for the other two drivers. I think this is part of a bigger discussion, but the way we are constructing the tuple for UNIX sockets it's kind of wrong in my opinion:

libs/driver/modern_bpf/helpers/store/auxmap_store_params.h

Line 665 in a884db5

/* Pack the tuple info:

as you can see, we are pushing two kernel pointers which are meaningless in userspace IMO. Plus, looks like the modern probe and the old drivers push a different tuple in this particular case.

loresuso · 2023-10-11T13:17:02Z

userspace/libscap/linux/scap_procs.c

+
+	snprintf(proc_dir, sizeof(proc_dir), "%s/proc/%d/", scap_get_host_root(), tinfo->pid);
+
+	return scap_fd_scan_fd_dir(linux_platform, &platform->m_proclist, proc_dir, tinfo, &sockets_by_ns, &num_fds_ret, lasterr);


This is the function that is responsible for calling the sinsp proc callbacks for the tinfo and each scanned fd

jasondellaluce

LGTM

FedeDP · 2023-10-12T09:14:33Z

userspace/libscap/scap_platform_impl.h

@@ -65,6 +66,7 @@ struct scap_platform_vtable
 	bool (*is_thread_alive)(struct scap_platform*, int64_t pid, int64_t tid, const char* comm);
 	int32_t (*get_global_pid)(struct scap_platform*, int64_t *pid, char *error);
 	int32_t (*get_threadlist)(struct scap_platform* platform, struct ppm_proclist_info **procinfo_p, char *lasterr);
+	int32_t (*get_fdlist)(struct scap_platform* platform, struct scap_threadinfo *tinfo, char *lasterr);


cc @gnosek about scap platform new method

FedeDP · 2023-10-12T09:22:16Z

I triggered a run of kernel testing framework against HEAD of the branch: https://github.com/falcosecurity/libs/actions/runs/6493764036

EDIT: ops, wrong; i need to push an upstream branch first.

FedeDP · 2023-10-12T09:32:14Z

Pushed upstream branch to test: https://github.com/falcosecurity/libs/commits/PR1400; new run: https://github.com/falcosecurity/libs/actions/runs/6493859950

loresuso · 2023-10-12T09:33:03Z

Thanks Fede!

FedeDP · 2023-10-12T09:43:06Z

No new issues (same as on master!)

Arm64:

KERNEL	CMAKE-CONFIGURE	KMOD BUILD	KMOD SCAP-OPEN	BPF-PROBE BUILD	BPF-PROBE SCAP-OPEN	MODERN-BPF SCAP-OPEN
amazonlinux2-5.4	🟢	🟢	🟢	🟢	🟢	🟡
amazonlinux2022-5.15	🟢	🟢	🟢	🟢	🟢	🟢
fedora-6.2	🟢	🟢	🟢	🟢	🟢	🟢
oraclelinux-4.14	🟢	🟢	🟢	🟡	🟡	🟡
oraclelinux-5.15	🟢	🟢	🟢	🟢	🟢	🟢
ubuntu-6.3	🟢	🟢	🟢	🟢	🟢	🟢

amd64:

KERNEL	CMAKE-CONFIGURE	KMOD BUILD	KMOD SCAP-OPEN	BPF-PROBE BUILD	BPF-PROBE SCAP-OPEN	MODERN-BPF SCAP-OPEN
amazonlinux2-4.19	🟢	🟢	🟢	🟢	🟢	🟡
amazonlinux2-5.10	🟢	🟢	🟢	🟢	🟢	🟢
amazonlinux2-5.15	🟢	🟢	🟢	🟢	🟢	🟢
amazonlinux2-5.4	🟢	🟢	🟢	🟢	🟢	🟡
amazonlinux2022-5.15	🟢	🟢	🟢	🟢	🟢	🟢
amazonlinux2023-6.1	🟢	🟢	🟢	🟢	🟢	🟢
archlinux-6.0	🟢	🟢	🟢	🟢	🟢	🟢
centos-3.10	🟢	🟢	🟢	🟡	🟡	🟡
centos-4.18	🟢	❌	❌	🟢	🟢	🟢
centos-5.14	🟢	🟢	🟢	🟢	🟢	🟢
fedora-5.17	🟢	❌	❌	🟢	🟢	🟢
fedora-5.8	🟢	🟢	🟢	🟢	❌	🟢
fedora-6.2	🟢	🟢	🟢	🟢	🟢	🟢
oraclelinux-3.10	🟢	🟢	🟢	🟡	🟡	🟡
oraclelinux-4.14	🟢	🟢	🟢	🟢	🟢	🟡
oraclelinux-5.15	🟢	🟢	🟢	🟢	🟢	🟢
oraclelinux-5.4	🟢	🟢	🟢	🟢	🟢	🟡
ubuntu-4.15	🟢	🟢	🟢	🟢	🟢	🟡
ubuntu-6.3	🟢	🟢	🟢	🟢	🟢	🟢

Andreagit97

Amazing work!

test/drivers/test_suites/syscall_exit_suite/recvmsg_x.cpp

userspace/libscap/linux/scap_procs.c

userspace/libscap/scap_platform_api.h

userspace/libscap/scap_platform_impl.h

userspace/libsinsp/parsers.cpp

…remove useless forward decl Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

FedeDP · 2023-10-12T13:13:40Z

/milestone next-driver

loresuso · 2023-10-12T13:44:55Z

Looks like the test on ancillary_data is flaky. Sometimes the recvmsg event can't be found, which is really weird
EDIT: I have now tried to put the exact pid in the wait call. Let's run this a couple of times to see if that was the issue.
RE-EDIT: looks like it works now

… tests Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

Andreagit97

/approve

poiana · 2023-10-13T10:00:32Z

LGTM label has been added.

Git tree hash: 867e8603ae65d8adbb184f842155c5f79276ee57

FedeDP

/approve

poiana · 2023-10-13T10:01:39Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

FedeDP · 2023-10-13T10:04:00Z

/hold cancel

poiana added kind/feature New feature or request release-note-none dco-signoff: yes area/driver-kmod area/driver-bpf area/driver-modern-bpf area/libsinsp labels Oct 10, 2023

poiana requested review from hbrueckner and Molter73 October 10, 2023 13:09

poiana added the size/L label Oct 10, 2023

poiana added the do-not-merge/hold label Oct 10, 2023

loresuso added 4 commits October 10, 2023 13:11

update(driver): add msgcontrol parameter to recvmsg in kmod

b8843a3

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

update(driver/modern_bpf): add msgcontrol parameter to recvmsg in mod…

c3252e2

…ern bpf Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

update(driver/bpf): add msgcontrol parameter to recvmsg in bpf

82d41ee

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

update(userspace/libscap): introduce new platform api to retrieve fil…

700d80e

…e descriptors of a process from procfs Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

loresuso force-pushed the update/shm-rights branch from 7d9bb8d to f3082b7 Compare October 10, 2023 13:11

loresuso changed the title ~~update: handle processes sending open file descriptors via SCM_RIGHTS~~ wip: update: handle processes sending open file descriptors via SCM_RIGHTS Oct 10, 2023

poiana added the do-not-merge/work-in-progress label Oct 10, 2023

loresuso force-pushed the update/shm-rights branch 2 times, most recently from 4fca071 to 537b173 Compare October 10, 2023 14:12

poiana added size/XL and removed size/L labels Oct 10, 2023

loresuso added 3 commits October 10, 2023 21:31

update(userspace/libsinsp): add new parsing logic for SCM_RIGTHS

5e382a8

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

update(test/drivers): update test suite with the new msg_control para…

11cd5d5

…meter in recvmsg Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

update(driver): bump SCHEMA_VERSION to 2.12.1

b611781

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

loresuso force-pushed the update/shm-rights branch 3 times, most recently from 70c3c92 to 2c318a4 Compare October 11, 2023 08:46

new(test/driver): add test for ancillary data in recvmsg

b3119d2

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

poiana removed the do-not-merge/work-in-progress label Oct 11, 2023

loresuso commented Oct 11, 2023

View reviewed changes

jasondellaluce reviewed Oct 11, 2023

View reviewed changes

FedeDP reviewed Oct 12, 2023

View reviewed changes

Andreagit97 reviewed Oct 12, 2023

View reviewed changes

update(userspace/libsinsp): check correct allocation of thread info, …

ed5ff00

…remove useless forward decl Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

poiana added this to the next-driver milestone Oct 12, 2023

update(test/drivers): handle msgcontrol parameters also in socketcall…

73ad7c5

… tests Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>

loresuso force-pushed the update/shm-rights branch 3 times, most recently from 0dd985c to 73ad7c5 Compare October 13, 2023 08:54

Andreagit97 approved these changes Oct 13, 2023

View reviewed changes

poiana assigned Andreagit97 Oct 13, 2023

poiana added the lgtm label Oct 13, 2023

poiana added the approved label Oct 13, 2023

FedeDP approved these changes Oct 13, 2023

View reviewed changes

poiana assigned FedeDP Oct 13, 2023

poiana removed the do-not-merge/hold label Oct 13, 2023

poiana merged commit 9daa2db into falcosecurity:master Oct 13, 2023
69 checks passed

FedeDP mentioned this pull request Dec 25, 2023

Add FDs to a process' FD table when transferred via SCM_RIGHTS #1364

Closed

Andreagit97 changed the title ~~update: handle processes sending open file descriptors via SCM_RIGHTS~~ update(driver): handle processes sending open file descriptors via SCM_RIGHTS Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update(driver): handle processes sending open file descriptors via SCM_RIGHTS #1400

update(driver): handle processes sending open file descriptors via SCM_RIGHTS #1400

loresuso commented Oct 10, 2023

github-actions bot commented Oct 10, 2023 •

edited

loresuso Oct 11, 2023

loresuso Oct 11, 2023 •

edited

loresuso Oct 11, 2023

loresuso Oct 11, 2023 •

edited

jasondellaluce left a comment

FedeDP Oct 12, 2023

FedeDP commented Oct 12, 2023 •

edited

FedeDP commented Oct 12, 2023

loresuso commented Oct 12, 2023

FedeDP commented Oct 12, 2023 •

edited

Andreagit97 left a comment

FedeDP commented Oct 12, 2023

loresuso commented Oct 12, 2023 •

edited

Andreagit97 left a comment

poiana commented Oct 13, 2023

FedeDP left a comment

poiana commented Oct 13, 2023

FedeDP commented Oct 13, 2023


		/* Parameter 2: size (type: PT_UINT32) */
		evt_test->assert_numeric_param(2, (uint32_t)FULL_MESSAGE_LEN);


		snprintf(proc_dir, sizeof(proc_dir), "%s/proc/%d/", scap_get_host_root(), tinfo->pid);

		return scap_fd_scan_fd_dir(linux_platform, &platform->m_proclist, proc_dir, tinfo, &sockets_by_ns, &num_fds_ret, lasterr);

update(driver): handle processes sending open file descriptors via SCM_RIGHTS #1400

update(driver): handle processes sending open file descriptors via SCM_RIGHTS #1400

Conversation

loresuso commented Oct 10, 2023

github-actions bot commented Oct 10, 2023 • edited

loresuso Oct 11, 2023

Choose a reason for hiding this comment

loresuso Oct 11, 2023 • edited

Choose a reason for hiding this comment

loresuso Oct 11, 2023

Choose a reason for hiding this comment

loresuso Oct 11, 2023 • edited

Choose a reason for hiding this comment

jasondellaluce left a comment

Choose a reason for hiding this comment

FedeDP Oct 12, 2023

Choose a reason for hiding this comment

FedeDP commented Oct 12, 2023 • edited

FedeDP commented Oct 12, 2023

loresuso commented Oct 12, 2023

FedeDP commented Oct 12, 2023 • edited

Andreagit97 left a comment

Choose a reason for hiding this comment

FedeDP commented Oct 12, 2023

loresuso commented Oct 12, 2023 • edited

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Oct 13, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 13, 2023

FedeDP commented Oct 13, 2023

github-actions bot commented Oct 10, 2023 •

edited

loresuso Oct 11, 2023 •

edited

loresuso Oct 11, 2023 •

edited

FedeDP commented Oct 12, 2023 •

edited

FedeDP commented Oct 12, 2023 •

edited

loresuso commented Oct 12, 2023 •

edited