fix(libsinsp): fix some path handling in fs.path #1571

incertum · 2023-12-13T03:26:15Z

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

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

fs.path* still had some minor bugs in handling path resolution and ensuring no trailing / is in the output fields.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(libsinsp): fix some path handling in fs.path

incertum · 2023-12-13T03:26:32Z

/milestone 0.14.0

incertum

To the best of my testing abilities fd.name and fs.path.name now behave consistently the same way.

incertum · 2023-12-13T03:30:35Z

userspace/libsinsp/sinsp_filtercheck_fspath.cpp

-		m_tstr = sinsp_utils::concatenate_paths(tinfo->get_cwd(), m_tstr);
+		if(m_tstr.front() != '/')
+		{
+			m_tstr = sinsp_utils::concatenate_paths(tinfo->get_cwd(), m_tstr.c_str());


@federico-sysdig and @LucaGuerra hope you are ready for the next round 🤦‍♀️
Similar to the last PR we also have some issues here with the string. The possible trailing slash would not be removed otherwise. I have adjusted the unit tests to test this as well. Plus we also need to call concatenate_paths when the path is absolute. Absolute paths with path traversal happen on real-life servers.

incertum · 2023-12-13T03:31:12Z

userspace/libsinsp/test/events_fspath.ut.cpp


 	const char *rel_filename_complex = "../\\.../../tmp/filename_complex";
 	const char *resolved_rel_filename_complex = "/tmp/filename_complex";

 	const char *rel_filename_nopath = "nopath";
 	const char *resolved_rel_filename_nopath = "/root/nopath";

-	const char *name = "/tmp/name";
+	const char *name = "/tmp/random/dir...///../../name/";


Test for possible trailing / and path traversal for absolute paths.

incertum · 2023-12-13T03:32:18Z

userspace/libsinsp/test/events_fspath.ut.cpp

-		verify_fields(evt, expected_name, expected_name, NULL, NULL, NULL, NULL);
-	}
-
-	void test_exit_path_raw(const char *expected_name,


We could convert test_exit_path_raw to test_exit_path. That way the unit tests are clearer and more functional.

federico-sysdig

Wrote just a few comments.

userspace/libsinsp/utils.cpp

federico-sysdig · 2023-12-13T07:03:13Z

userspace/libsinsp/sinsp_filtercheck_fspath.cpp

+		} else
+		{
+			// concatenate_paths takes care of resolving the path
+			m_tstr = sinsp_utils::concatenate_paths("", m_tstr.c_str());


std::string should be automatically cast to std::string_view, I don't think you need the c_str() call (which needs an automatic cast from const char* to std::string_view anyway). A similar simplification can be done a few lines above.

We had that discussion before, nothing else worked here ...

Please, forgive me, I forgot. I'm aging. I'm noticing that we do pass a straight std::string to this function elsewhere. Do you mean that in this specific call instance it is not working? As in not compiling or not doing the right concatenation?

It's not doing the right concatenation. I developed and tested on my fedora Linux box.

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum · 2023-12-13T17:34:20Z

Rebased and squashed with the solution we all want, but I am now still getting some errors locally:

[  PASSED  ] 417 tests.
[  FAILED  ] 4 tests, listed below:
[  FAILED  ] fspath.unlinkat_2
[  FAILED  ] fspath.open
[  FAILED  ] fspath.openat_2
[  FAILED  ] fspath.openat2

all similar like:

Expected equality of these values:
  get_field_as_string(evt, fs_path_name).c_str()
    Which is: "/root/tmp/name/"
  expected_name
    Which is: "/root/tmp/name"

incertum · 2023-12-13T17:39:44Z

CI is confirming these failures.

LucaGuerra · 2023-12-13T22:50:34Z

@incertum the problem lies in the m_tstr.assign() calls in that file. As you can see the are taking a raw buffer (incl. NUL terminator) and converting it into a string by giving it the total length so they will always include the NUL. We don't want that, so we should change those assignments to build the string only up to its length to e.g.:

m_tstr.assign((const char*) extract_values[0].ptr, strnlen((const char*) extract_values[0].ptr, extract_values[0].len));

At this point I think we can safely remove the part that says:

	// If m_tstr ends in a c-style \0, remove it to be
	// consistent. Generally, the evt.rawarg fields above *will*
	// end in c-style \0 characters, while the ones that extract
	// from the enter event or have values populated by parsers
	// (e.g. fchmod/fchown) will not.
	if(m_tstr.back() == '\0')
	{
		m_tstr.pop_back();
	}

This becomes redundant because we already deal with the problem earlier as we should, I think it's good to maintain the invariant that m_tstr is a well formed string.

LucaGuerra · 2023-12-13T22:51:29Z

userspace/libsinsp/sinsp_filtercheck_fspath.cpp

@@ -349,7 +348,14 @@ uint8_t* sinsp_filter_check_fspath::extract(sinsp_evt* evt, OUT uint32_t* len, b
 			return NULL;
 		}

-		m_tstr = sinsp_utils::concatenate_paths(tinfo->get_cwd(), m_tstr);
+		if(m_tstr.front() != '/')


once the problem explained below is fixed I believe we can safely revert this change.

Follow up: I think we still need this as in this case we concatenate cwd and m_tstr, and else we still use the helper to possibly resolve the path.

Can we use std::filesystem::path::is_absolute instead? Much more readable IMHO.
If i got this correctly, we are stating that if m_tstr is relative, we attach the current working directory, else we use it as is, right?

else we use it as is, right?

almost correct we still need to resolve the absolute path as absolute paths can still contain path traversal. Statement based on real data observed.

…ecks Co-authored-by: Luca Guerra <luca@guerra.sh> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum · 2023-12-13T23:09:00Z

Re #1571 (comment) thanks @LucaGuerra 🚀 -- suggestions incorporated plus I learned dos and donts with string handling for the future, thanks for that!

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

LucaGuerra

LGTM

poiana · 2023-12-14T16:25:52Z

LGTM label has been added.

Git tree hash: 49f3589de256b0d33ac5ae69763f9ead2be2293d

FedeDP

/approve

poiana · 2023-12-14T16:28:23Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [FedeDP,LucaGuerra,incertum]

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

poiana added release-note kind/bug Something isn't working dco-signoff: yes kind/cleanup area/libsinsp labels Dec 13, 2023

poiana added the size/L label Dec 13, 2023

poiana added this to the 0.14.0 milestone Dec 13, 2023

poiana requested review from hbrueckner and Molter73 December 13, 2023 03:26

poiana added the approved label Dec 13, 2023

incertum commented Dec 13, 2023

View reviewed changes

federico-sysdig reviewed Dec 13, 2023

View reviewed changes

fix(libsinsp): fix some path handling in fs.path + cleanup

3a03b22

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the cleanup-fs-path branch from 4771585 to 3a03b22 Compare December 13, 2023 17:32

LucaGuerra reviewed Dec 13, 2023

View reviewed changes

fix(libsinsp): fix null termination string issues in fs.path filterch…

f37cf5a

…ecks Co-authored-by: Luca Guerra <luca@guerra.sh> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libsinsp): apply reviewers suggestions

2fce16e

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

LucaGuerra approved these changes Dec 14, 2023

View reviewed changes

poiana assigned LucaGuerra Dec 14, 2023

poiana added the lgtm label Dec 14, 2023

FedeDP approved these changes Dec 14, 2023

View reviewed changes

poiana assigned FedeDP Dec 14, 2023

poiana merged commit ef8ca60 into falcosecurity:master Dec 14, 2023
31 checks passed

incertum deleted the cleanup-fs-path branch December 14, 2023 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(libsinsp): fix some path handling in fs.path #1571

fix(libsinsp): fix some path handling in fs.path #1571

incertum commented Dec 13, 2023

incertum commented Dec 13, 2023

incertum left a comment

incertum Dec 13, 2023

incertum Dec 13, 2023

incertum Dec 13, 2023

federico-sysdig left a comment

federico-sysdig Dec 13, 2023

incertum Dec 13, 2023

federico-sysdig Dec 13, 2023

incertum Dec 13, 2023

incertum commented Dec 13, 2023

incertum commented Dec 13, 2023

LucaGuerra commented Dec 13, 2023

LucaGuerra Dec 13, 2023

incertum Dec 13, 2023

FedeDP Dec 14, 2023

incertum Dec 14, 2023

incertum commented Dec 13, 2023

LucaGuerra left a comment

poiana commented Dec 14, 2023

FedeDP left a comment

poiana commented Dec 14, 2023

fix(libsinsp): fix some path handling in fs.path #1571

fix(libsinsp): fix some path handling in fs.path #1571

Conversation

incertum commented Dec 13, 2023

incertum commented Dec 13, 2023

incertum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

federico-sysdig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented Dec 13, 2023

incertum commented Dec 13, 2023

LucaGuerra commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented Dec 13, 2023

LucaGuerra left a comment

Choose a reason for hiding this comment

poiana commented Dec 14, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Dec 14, 2023