fix(sinsp): fix fs.path filterchecks for relative paths (add dirfd concept)
#1993
Conversation
poiana
added
release-note-none
do-not-merge/work-in-progress
kind/bug
|
/milestone 0.18.0 |
| @@ -355,10 +355,100 @@ uint8_t* sinsp_filter_check_fspath::extract_single(sinsp_evt* evt, uint32_t* len | |||
|
|
|||
| if(!std::filesystem::path(m_tstr).is_absolute()) | |||
| { | |||
| m_tstr = sinsp_utils::concatenate_paths(tinfo->get_cwd(), m_tstr); | |||
| std::string sdir; // init | |||
| // Compare to `sinsp_filter_check_fd::extract_fdname_from_creator` logic | |||
There was a problem hiding this comment.
Sharing the relevant event I grepped out for easier review if the param number is correct
[PPME_SYSCALL_UNLINKAT_2_X] = {"unlinkat", EC_FILE | EC_SYSCALL, EF_NONE, 4, {{"res", PT_ERRNO, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"name", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"flags", PT_FLAGS32, PF_HEX, unlinkat_flags} } },
[PPME_SYSCALL_MKDIRAT_X] = {"mkdirat", EC_FILE | EC_SYSCALL, EF_NONE, 4, {{"res", PT_ERRNO, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"path", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"mode", PT_UINT32, PF_HEX} } },
[PPME_SYSCALL_OPENAT_2_X] = {"openat", EC_FILE | EC_SYSCALL, EF_CREATES_FD | EF_MODIFIES_STATE, 7, {{"fd", PT_FD, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"name", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"flags", PT_FLAGS32, PF_HEX, file_flags}, {"mode", PT_UINT32, PF_OCT}, {"dev", PT_UINT32, PF_HEX}, {"ino", PT_UINT64, PF_DEC} } },
[PPME_SYSCALL_FCHMODAT_X] = {"fchmodat", EC_FILE | EC_SYSCALL, EF_NONE, 4, {{"res", PT_ERRNO, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"filename", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"mode", PT_MODE, PF_OCT, chmod_mode} } },
[PPME_SYSCALL_OPENAT2_X] = {"openat2", EC_FILE | EC_SYSCALL, EF_CREATES_FD | EF_MODIFIES_STATE, 8, {{"fd", PT_FD, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"name", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"flags", PT_FLAGS32, PF_HEX, file_flags}, {"mode", PT_UINT32, PF_OCT}, {"resolve", PT_FLAGS32, PF_HEX, openat2_flags}, {"dev", PT_UINT32, PF_HEX}, {"ino", PT_UINT64, PF_DEC} } },
[PPME_SYSCALL_FCHOWNAT_X] = {"fchownat", EC_FILE | EC_SYSCALL, EF_NONE, 6, {{"res", PT_ERRNO, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"pathname", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"uid", PT_UINT32, PF_DEC}, {"gid", PT_UINT32, PF_DEC}, {"flags", PT_FLAGS32, PF_HEX, fchownat_flags}} },
[PPME_SYSCALL_MKNODAT_X] = {"mknodat", EC_OTHER | EC_SYSCALL, EF_USES_FD, 5, {{"res", PT_ERRNO, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"path", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)},{"mode", PT_MODE, PF_OCT, mknod_mode},{"dev", PT_UINT32, PF_DEC}}},
[PPME_SYSCALL_NEWFSTATAT_X] = {"newfstatat", EC_FILE | EC_SYSCALL, EF_USES_FD, 4, {{"res", PT_ERRNO, PF_DEC}, {"dirfd", PT_FD, PF_DEC}, {"path", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"flags", PT_FLAGS32, PF_HEX, newfstatat_flags}}},
[PPME_SYSCALL_OPENAT_2_E] = {"openat", EC_FILE | EC_SYSCALL, EF_CREATES_FD | EF_MODIFIES_STATE, 4, {{"dirfd", PT_FD, PF_DEC}, {"name", PT_FSRELPATH, PF_NA, DIRFD_PARAM(0)}, {"flags", PT_FLAGS32, PF_HEX, file_flags}, {"mode", PT_UINT32, PF_OCT} } },
[PPME_SYSCALL_OPENAT2_E] = {"openat2", EC_FILE | EC_SYSCALL, EF_CREATES_FD | EF_MODIFIES_STATE, 5, {{"dirfd", PT_FD, PF_DEC}, {"name", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"flags", PT_FLAGS32, PF_HEX, file_flags}, {"mode", PT_UINT32, PF_OCT}, {"resolve", PT_FLAGS32, PF_HEX, openat2_flags} } },
[PPME_SYSCALL_RENAMEAT2_X] = {"renameat2", EC_FILE | EC_SYSCALL, EF_NONE, 6, {{"res", PT_ERRNO, PF_DEC}, {"olddirfd", PT_FD, PF_DEC}, {"oldpath", PT_FSRELPATH, PF_NA, DIRFD_PARAM(1)}, {"newdirfd", PT_FD, PF_DEC}, {"newpath", PT_FSRELPATH, PF_NA, DIRFD_PARAM(3)}, {"flags", PT_FLAGS32, PF_HEX, renameat2_flags} } },
[PPME_SYSCALL_SYMLINKAT_X] = {"symlinkat", EC_FILE | EC_SYSCALL, EF_NONE, 4, {{"res", PT_ERRNO, PF_DEC}, {"target", PT_CHARBUF, PF_NA}, {"linkdirfd", PT_FD, PF_DEC}, {"linkpath", PT_FSRELPATH, PF_NA, DIRFD_PARAM(2)} } },
[PPME_SYSCALL_OPEN_BY_HANDLE_AT_X] = {"open_by_handle_at", EC_FILE | EC_SYSCALL, EF_CREATES_FD | EF_MODIFIES_STATE, 6, {{"fd", PT_FD, PF_DEC}, {"mountfd", PT_FD, PF_DEC}, {"flags", PT_FLAGS32, PF_HEX, file_flags}, {"path", PT_FSPATH, PF_NA}, {"dev", PT_UINT32, PF_HEX}, {"ino", PT_UINT64, PF_DEC} } },
As mentioned in the PR all still WIP since I need to expand the unit tests ...
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1993 +/- ##
==========================================
- Coverage 74.30% 74.10% -0.21%
==========================================
Files 253 254 +1
Lines 30966 31213 +247
Branches 5414 5442 +28
==========================================
+ Hits 23010 23130 +120
- Misses 7951 8082 +131
+ Partials 5 1 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
userspace/libsinsp/parsers.cpp
Outdated
| @@ -2732,8 +2732,8 @@ void sinsp_parser::parse_open_openat_creat_exit(sinsp_evt *evt) | |||
| } | |||
| } | |||
|
|
|||
| // since open_by_handle_at returns an absolute path we will always start at / | |||
| sdir = ""; | |||
| int64_t dirfd_mountfd = evt->get_param(1)->as<int64_t>(); // mountfd | |||
There was a problem hiding this comment.
Here in name we always return the full path from our drivers, so open_by_handle_at behaves differently, it doesn't use dirfd + relative path
name = evt->get_param(3)->as<std::string_view>();There was a problem hiding this comment.
Hah of course why not ... I wasn't aware how fragmented our open* approaches truly are and that we even do dpath traversals for open by handle at in the kernel ... one of these days I'll try to compile a few things in an issue, but it's not urgent for the upcoming release.
incertum
force-pushed
the
fix-fs-path-openat
branch
from
3c5d125 to
bfe211d
Compare
Perf diff from master - unit testsPerf diff from master - scap fileHeap diff from master - unit testsHeap diff from master - scap file |
| { | ||
| // todo the cached fd.name might be subject to some std::string_view lifetime issues as the second path of the path concatenation | ||
| // is missing when retrieving fd.name from the cache. Let's check if string_view and std::string handling is done correctly throughout. | ||
| // ASSERT_STREQ(get_field_as_string(evt, fs_path_name).c_str(), get_field_as_string(evt, "fd.name").c_str()); |
There was a problem hiding this comment.
@falcosecurity/libs-maintainers and also @federico-sysdig would appreciate some help to resolve some mystery (again) ....
So these tests (comparing the fd.name from the cache to the fs.path.name) are failing, possibly due to some std::string_view lifetime issues or other issues? Regardless am stuck debugging.
Sprinkled few print statements along the relevant code paths and we only loose the second part of the concatenated path when retrieving the name from the cache in the fd filterchecks. Before that it's all correct within the parsers logic.
[ RUN ] fspath.openat_2_relative
parser name exit tmp/random/dir...///../../name.txt
parser name enter event tmp/random/dir...///../../name.txt
parser name fullpath /tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8/tmp/name.txt
parser name fullpath from fdi /tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8/tmp/name.txt
parser name fullpath from table cache fdi /tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8/tmp/name.txt
FDNAME fd 3 CACHE FILTERCHECK /tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8
[Btw the fallback in the filtercheck extract_from_null_fd is working as expected and these tests would pass. Also note that today we do not test fd.name for many scenarios ...]
There was a problem hiding this comment.
I don't understand. Are you saying that you get a full path when using get_field_as_string(evt, fs_path_name) and only the first part of it when using get_field_as_string(evt, "fd.name")? Should they really be equal? Wouldn't it depend on the value of fs_path_name?
If there is an issue with a string view pointing at a string whose lifetime is over, that is indeed a serious issue, but it would be a problem within the machinery behind get_field_as_string.
Nit: The check can be a bit simpler:
ASSERT_EQ(get_field_as_string(evt, fs_path_name), get_field_as_string(evt, "fd.name"));There was a problem hiding this comment.
(1) Incorporated your nit suggestion and also made sure the list of events where these 2 should be same is complete. Re-pushed the last commit.
(2) Plus let me share the 2 locations in the source code where the cached string that represents the fd full file path is not equal.
-
which is
m_tstr = m_fdinfo->m_name;has the second part of the paths that were concatenated missing - Versus in the parsers
libs/userspace/libsinsp/parsers.cpp
Line 2776 in 874e573
m_nameis correct, also when directly retrieving it again after having written it to the fd table / cache.
See below a print output for how
m_tstr = m_fdinfo->m_name; looks like atm.
[ RUN ] fspath.open
FILTERCHECK FDNAME FROM FD CACHE /tmp/name
[ OK ] fspath.open (0 ms)
[ RUN ] fspath.openat
[ OK ] fspath.openat (0 ms)
[ RUN ] fspath.openat_2
FILTERCHECK FDNAME FROM FD CACHE /tmp/name
[ OK ] fspath.openat_2 (0 ms)
[ RUN ] fspath.openat_2_relative
FILTERCHECK FDNAME FROM FD CACHE /tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8
/home/m/Documents/OSS/oss-dev-master/libs/userspace/libsinsp/test/events_fspath.ut.cpp:114: Failure
Expected equality of these values:
get_field_as_string(evt, fs_path_name)
Which is: "/tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8/tmp/name.txt"
get_field_as_string(evt, "fd.name")
Which is: "/tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8"
[ FAILED ] fspath.openat_2_relative (0 ms)
[ RUN ] fspath.openat2
FILTERCHECK FDNAME FROM FD CACHE /tmp/name
[ OK ] fspath.openat2 (0 ms)
[ RUN ] fspath.openat2_relative
FILTERCHECK FDNAME FROM FD CACHE /tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8
/home/m/Documents/OSS/oss-dev-master/libs/userspace/libsinsp/test/events_fspath.ut.cpp:114: Failure
Expected equality of these values:
get_field_as_string(evt, fs_path_name)
Which is: "/tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8/tmp/name.txt"
get_field_as_string(evt, "fd.name")
Which is: "/tmp/dirfd1/dirfd2/dirfd3/dirfd4/dirfd5/dirfd6/dirfd7/dirfd8"
[ FAILED ] fspath.openat2_relative (0 ms)
[ RUN ] fspath.openat2_relative_dirfd_cwd
There was a problem hiding this comment.
I'm taking a look at this. I tried pulling your branch, rebasing it and running the tests but this one doesn't seem to be failing. Did you happen to remove the failing one?
There was a problem hiding this comment.
The failing test is commented out for now.
There was a problem hiding this comment.
So I took a deeper look at this. I believe the problem lies in the use of parse_dirfd(). Within that function it calls set_fd_info, replacing the current event fd with the dirfd one. By calling that function in the extract function it means that the thread state is affected at extract time, which is not what we want:
libs/userspace/libsinsp/parsers.cpp
Line 2544 in 5ed00b2
In fact, if you replace the verify_fields function with:
void verify_fields(ppm_event_code event_type, sinsp_evt *evt,
const char *expected_name,
const char *expected_nameraw,
const char *expected_source,
const char *expected_sourceraw,
const char *expected_target,
const char *expected_targetraw)
{
switch (event_type)
{
case PPME_SYSCALL_OPENAT_2_X:
case PPME_SYSCALL_OPENAT2_X:
case PPME_SYSCALL_OPEN_X:
case PPME_SYSCALL_OPEN_BY_HANDLE_AT_X:
{
std::string fd_name_value = get_field_as_string(evt, "fd.name");
std::string fs_path_name_value = get_field_as_string(evt, fs_path_name);
ASSERT_EQ(fs_path_name_value, fd_name_value);
}
break;
default:
break;
}
}You will notice it passes. But if you swap the order of the two statements std::string fd_name_value = get_field_as_string(evt, "fd.name"); and std::string fs_path_name_value = get_field_as_string(evt, fs_path_name); the test will fail.
So I think the approach we need is slightly different; most likely we will need to avoid reparsing the event in the extractor if possible or modifying this by not affecting the thread state.
There was a problem hiding this comment.
Thanks so much 🚀 @LucaGuerra. Pushed a co-authored commit. Feel free to overwrite it if you have a better idea.
Also created a follow ticket here: #2039
The macos test is currently failing ...
incertum
changed the title
fs.path filterchecks for relative paths (add dirfd concept) + fix PPME_SYSCALL_OPEN_BY_HANDLE_AT_X to use mount_fd as dirfdfs.path filterchecks for relative paths (add dirfd concept)
incertum
force-pushed
the
fix-fs-path-openat
branch
from
bfe211d to
d9b0e8d
Compare
Perf diff from master - unit testsPerf diff from master - scap fileHeap diff from master - unit testsHeap diff from master - scap file |
incertum
force-pushed
the
fix-fs-path-openat
branch
from
d9b0e8d to
cf83174
Compare
Perf diff from master - unit testsPerf diff from master - scap fileHeap diff from master - unit testsHeap diff from master - scap file |
incertum
changed the title
fs.path filterchecks for relative paths (add dirfd concept)fs.path filterchecks for relative paths (add dirfd concept)
incertum
force-pushed
the
fix-fs-path-openat
branch
from
cf83174 to
187b007
Compare
incertum
force-pushed
the
fix-fs-path-openat
branch
from
187b007 to
30f58f0
Compare
incertum
force-pushed
the
fix-fs-path-openat
branch
from
30f58f0 to
721785a
Compare
….path.* dirfd use cases Co-authored-by: Luca Guerra <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
incertum
force-pushed
the
fix-fs-path-openat
branch
from
721785a to
b28059f
Compare
| @@ -240,6 +240,56 @@ std::unique_ptr<sinsp_filter_check> sinsp_filter_check_fspath::allocate_new() | |||
| return ret; | |||
| } | |||
|
|
|||
| std::string sinsp_filter_check_fspath::parse_dirfd_stateless(sinsp_evt *evt, std::string_view name, int64_t dirfd) | |||
There was a problem hiding this comment.
If i understand correctly, all we need is to skip the evt->set_fd_info(evt->get_tinfo()->get_fd(dirfd)); call in parse_dirfd, right?
I think we can just add a boolean optional parameter (that of course defaults to true to parse_dirfd, like:
std::string sinsp_parser::parse_dirfd(sinsp_evt *evt, std::string_view name, int64_t dirfd, bool update_tinfo=true)
and pass false when called from the filterchecks.
There was a problem hiding this comment.
Correct, can do that. Are we sure we want one more if statement in the parsers logic?
|
@incertum pushed some changes, let me know wdyt (we can rework/remove the commit in case you don't really like it :D )
Also cc @LucaGuerra that helped me! |
…k_fspath to parsers. Simplified a bit the whole logic. Updated events_fspath tests adding the `PPM_O_DIRECTORY` flag as needed. Signed-off-by: Federico Di Pierro <[email protected]> Co-authored-by: Luca Guerra <[email protected]>
FedeDP
force-pushed
the
fix-fs-path-openat
branch
from
d2153c2 to
ba835da
Compare
| @@ -442,6 +442,33 @@ void sinsp_parser::process_event(sinsp_evt *evt) | |||
| case PPME_SYSCALL_PRCTL_X: | |||
| parse_prctl_exit_event(evt); | |||
| break; | |||
| case PPME_SYSCALL_NEWFSTATAT_X: | |||
There was a problem hiding this comment.
New parsers. I didn't add new methods for these small parsers.
Only parse when event are successful, and store in the event fdinfo the dirfd related fdinfo (that is already present in the thread table since it was previously added by an open related syscall)
| @@ -2307,6 +2334,12 @@ void sinsp_parser::parse_execve_exit(sinsp_evt *evt) | |||
| */ | |||
| std::string sdir = parse_dirfd(evt, pathname, dirfd); | |||
|
|
|||
| // Update event fdinfo since parse_dirfd is stateless | |||
| if (sdir != "." && sdir != "<UNKNOWN>") | |||
There was a problem hiding this comment.
Since parse_dirfd is now stateless, honor previous behavior by setting the fdinfo when needed (ie: when previous logic set it)
| { | ||
| add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_OPENAT2_E, 2, evt_dirfd, dirfd_path); | ||
| // pass PPM_O_DIRECTORY since we are creating a folder! | ||
| add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_OPENAT2_X, 5, evt_dirfd, evt_dirfd, dirfd_path, open_flags | PPM_O_DIRECTORY, mode); |
There was a problem hiding this comment.
We are creating a folder, enforce PPM_O_DIRECTORY otherwise our fdinfo will be off.
| case PPME_SYSCALL_SYMLINKAT_X: | ||
| { | ||
| add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_OPENAT2_E, 2, evt_dirfd, dirfd_path); | ||
| add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_OPENAT2_X, 5, evt_dirfd, evt_dirfd, dirfd_path, open_flags | PPM_O_DIRECTORY, mode); |
There was a problem hiding this comment.
We are creating a folder, enforce PPM_O_DIRECTORY otherwise our fdinfo will be off.
…ostic `PPM_AT_FDCWD` value instead of the platform dependent one. Signed-off-by: Federico Di Pierro <[email protected]>
Incredible work @FedeDP and @LucaGuerra, also thanks a lot for jumping in as we need this wrapped up soon. Changes LGTM! |
Aswered over there, imho there is no issue anymore since |
Signed-off-by: Luca Guerra <[email protected]>
|
Nice @LucaGuerra thanks for adding more tests! |
|
LGTM label has been added. Git tree hash: eedd25e41b16114378fcadd9c364bc8388189cd3
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, incertum 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 |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
While working on the anomalydetection plugin i was looking into the fd fallbacks and noticed that there are a few cases off in libs.
fs.pathfields incorrectly do not incorporate the concept ofdirfdand as such they differ from the more correctfd.namevalue if applicable.dirfdhandling forPPME_SYSCALL_OPEN_BY_HANDLE_AT_Xthroughout.@LucaGuerra and @FedeDP (when you are back), could you help take a look? We all were involved in recent fixes in these areas. Thank you.
CC @mstemm
@leogr these fixes will result in subtle semantic changes of the affected filtercheck fields. Please note that these changes are corrections meaning the current values are off in the here addressed cases.
Currently still in WIP as I need more time to seriously improve our test cases throughout.
Which issue(s) this PR fixes:
Fixes #2039
Special notes for your reviewer:
Does this PR introduce a user-facing change?: