Support pid_with_namespace in more tables #7132

nabilschear · 2021-06-01T21:00:05Z

Several tables have support for looking inside of docker containers (using the pid_with_namespace option) from the host OS (e.g., deb_packages). This PR adds the same support for several other osquery tables that rely on filesystem access for ground truth. This is just an elaborate cut and paste of existing code from deb_packages into other tables to use the existing generateInNamespace function. I've tested each of these changes by inspecting a docker container from the host.

linux-foundation-easycla · 2021-06-01T21:00:10Z

The committers are authorized under a signed CLA.

✅ Nabil Schear (e32976f)

theopolis · 2021-06-02T02:37:33Z

heads up @Smjert

nabilschear · 2021-06-02T20:59:08Z

ok, i fixed the broken test (I think)

nabilschear · 2021-06-03T00:58:11Z

woohoo all the checks passed @Smjert can you take a look? I think it's your code that I copy and pasted =)

directionless · 2021-06-03T02:23:05Z

If I remember correctly, there was some question about whether this could be added to the underlying virtual implementation, without needing to update/maintain it everywhere.

nabilschear · 2021-06-08T23:57:08Z

@Smjert or @theopolis any chance you can get a look at this? I'd like to get this into the next release. Thanks!

theopolis · 2021-06-09T01:29:51Z

This seems correct to me, but I am not very confident that I understand these features well.

Smjert

Sorry for the delay, I had a quick look at this initially but I was worried about more tricky and hidden issues.

I had a better look now and there are unfortunately some blockers.
There are mainly a couple of things that have to be checked when adding support for namespace joining to a table (one is a subset of the other).
One is that the table should not try to take use a library that's being potentially used by some other thread, which is sharing locks with.
This will potentially cause a deadlock.

The other is specifically glog macros, which should not be used to do logging in tables. Which is why you have the Logger logger instance that should be used instead.

In these table I see a couple of VLOG() which have not been changed to use the Logger, but that's the minor part.

There are a good portion of tables that use the readFile or forensicReadFile API; unfortunately these APIs have a VLOG and LOG usage here

osquery/osquery/filesystem/filesystem.cpp

Lines 134 to 137 in afaa114

    
           LOG(WARNING) << "Cannot read file that exceeds size limit: " 
        
                        << path.string(); 
        
           VLOG(1) << "Cannot read " << path.string() 
        
                   << " size exceeds limit: " << file_size << " > " << read_max;

To solve this, and ideally in general, APIs shouldn't write anything in the logs, and should return messages via Status that then the caller logs.

The authorized_keys table ends up doing a query against another table via the selectAllFrom.
If I'm not wrong this path contains LOG/VLOG calls too. In general we should avoid doing queries to other tables to implement a table logic, while instead write an API that can be shared and that doesn't log.

There are other tables that use the listInAbsoluteDirectory and resolveFilePattern APIs that in turn use the genGlobs, which in the checkForLoops function contains other logging calls.

nabilschear · 2021-06-14T20:44:34Z

Ok @Smjert I took a pass at the fixes you recommended. Here's the status:

etc_hosts: readFile -> changed to return status instead of logging
dns_resolvers: no changes needed?
users: no direct file I/O but there is a mutex for pwdEnumerationMutex for calling getpwnam or getpwuid. refactored to use the _r thread-safe versions of getpwnam etc. Removed mutex.
apt_sources: remove VLOG call, calls resolveFilePattern -> genGlobs -> checkForLoops. Removed logging call for symlink loops entirely (this was changed from a LOG to VLOG before, I think it shouldn't be logged at all see Change "Symlink loop" message from warning to verbose #6545)
authorized_keys: forensicReadFile-> readFile is good, calls select * on users. See users above.
crontab: listFilesInDirectory->genGlobs->checkForLoops already dealt with. forensicReadFile->readFile
suid_bin: pass logger into genSuidBinsFromPath
yum_sources: pass the logger around. resolveFilePattern->genGlobs->checkForLoops
python_packages: pass the logger down into genPackage. Calls readFile (already done). and listDirectoriesInDirectory which eventually gets to checkForLoops.
ssh_keys: listFilesInDirectory, forensicReadFile already patched. Calls select * from users

The problem with this is that it is kinda hard to trace all the issues. I tried reading through the code looking for potentially worrying internal library calls.

nabilschear · 2021-06-14T22:22:45Z

ok, I went and implemented the users table using the re-entrant _r versions of getpwent, getpwuid, and getpwnam. Code adapted from the man pages.

nabilschear · 2021-06-29T19:29:56Z

@Smjert how does this look? Ready to merge?

osquery/filesystem/filesystem.cpp

Smjert

Sorry, I have only done a quick pass, but thank you for making these changes!

The problem with this is that it is kinda hard to trace all the issues. I tried reading through the code looking for potentially worrying internal library calls.

I understand, but unfortunately there isn't really a way around it. This was documented in the original blueprint (#5975).

The plan was always to slowly add support to new tables exactly because the osquery core needed some refactors.

osquery/filesystem/filesystem.cpp

osquery/tables/system/linux/user_groups.cpp

osquery/tables/system/linux/users.cpp

nabilschear · 2021-07-08T19:42:25Z

ok, I've cleaned up all the readFile related logging and other suggested changes. It's ready for review now.

nabilschear · 2021-07-16T20:11:08Z

@Smjert can you make a final review? I think i've cleaned up everything and fixed the logging in the impacted tables.

nabilschear · 2021-07-20T21:06:38Z

ok, i finally fixed my silly #include issues after rebasing against master. Ready for review @Smjert

nabilschear · 2021-07-28T23:44:45Z

hi, would love to see this merged before the next release! @Smjert @theopolis

nabilschear · 2021-08-19T17:41:43Z

Another ping for getting this merged @Smjert and @theopolis

theopolis

Thanks for all of the effort here. Just a few minor requests and then it should be good to go.

Heads up that I did not audit all of the code to assure that ONLY the new logger is used. But I did my best to check at least the code within the table implementations.

osquery/filesystem/filesystem.cpp

osquery/tables/networking/etc_hosts.cpp

osquery/tables/system/linux/user_groups.cpp

osquery/tables/system/posix/apt_sources.cpp

osquery/tables/system/posix/authorized_keys.cpp

osquery/tables/system/posix/crontab.cpp

osquery/tables/system/python_packages.cpp

osquery/tables/system/ssh_keys.cpp

nabilschear · 2021-08-20T20:31:10Z

@theopolis i got all the changes you suggested in

theopolis

One last round of nitpicks, sorry about this. The thing I really care about is the == -> > change. I think that is my fault for having made the wrong originally code suggestion in the GitHub code review UI.

osquery/tables/system/linux/groups.cpp

osquery/tables/system/linux/user_groups.cpp

osquery/tables/system/linux/users.cpp

…1 to master * commit 'f72b7c5510b8cd78c9d0450cbd1f31903681caa5': (53 commits) Add `TimeoutStopSec` to systemd unit files (osquery#7190) Prevent osquery from killing itself when the --force flag is used (osquery#7295) Linux: Support AF_PACKET sockets. (osquery#7282) libs: update openssl to 1.1.1l (osquery#7293) Correct macOS installed app bundle path in osqueryctl and doc (osquery#7289) macos path fix in launchd plist (osquery#7288) Update osquery installed artifacts default paths in code (osquery#7285) Update osquery installed artifacts paths in the documentation (osquery#7286) Update packaging SHA (osquery#7279) Change to the `disk_encryption` table to support QueryContext (osquery#7209) Add feature to skip denylist for event-based queries (osquery#7158) Support pid_with_namespace in more tables (osquery#7132) audit: socket_events improvements (osquery#7269) [linux][packaging] Update packaging paths (osquery#7271) Change logger_mode flag to be actually interpreted as an octal (osquery#7273) Update `uptime` table descrption (osquery#7270) [macOS][packaging] Create an app bundle along with other package_data (osquery#7263) Add case sensitive pragma to the pragma/actions authorizer allow list (osquery#7267) Fix audit rule removal upon osquery exit (osquery#7221) Fix osquery_info build_platform column value on Linux (osquery#7254) ...

nabilschear requested review from a team as code owners June 1, 2021 21:00

nabilschear force-pushed the namespaces branch 2 times, most recently from 376c1bb to 8ce98ea Compare June 1, 2021 21:07

add namespace support for various tables

e32976f

nabilschear force-pushed the namespaces branch from 8ce98ea to e32976f Compare June 1, 2021 21:52

theopolis added Linux virtual tables labels Jun 2, 2021

add pid_with_namespace option to output

a59c554

nabilschear changed the title ~~Namespaces~~ Support pid_with_namespace in more tables Jun 2, 2021

Smjert requested changes Jun 10, 2021

View reviewed changes

fix logger related race conditions

8749477

nabilschear force-pushed the namespaces branch from 220eebc to 2981dc3 Compare June 14, 2021 22:18

nabilschear force-pushed the namespaces branch from 2981dc3 to 278b42a Compare June 14, 2021 23:37

trying the _r versions from <pwd.h> to avoid needing a mutex.

5b992fc

nabilschear force-pushed the namespaces branch from 278b42a to 5b992fc Compare June 15, 2021 18:24

sargun reviewed Jun 29, 2021

View reviewed changes

osquery/filesystem/filesystem.cpp Outdated Show resolved Hide resolved

Smjert requested changes Jun 30, 2021

View reviewed changes

osquery/filesystem/filesystem.cpp Outdated Show resolved Hide resolved

osquery/tables/system/linux/user_groups.cpp Outdated Show resolved Hide resolved

osquery/tables/system/linux/users.cpp Outdated Show resolved Hide resolved

nabilschear force-pushed the namespaces branch 2 times, most recently from da36f9f to a239e2b Compare July 5, 2021 18:26

raii for buffers

9611893

nabilschear force-pushed the namespaces branch from a239e2b to 9611893 Compare July 8, 2021 17:41

consistent fork safe logging

9dd31d2

move this to avoid a merge conflict

f31448a

Merge remote-tracking branch 'upstream/master' into namespaces

2700a31

nabilschear force-pushed the namespaces branch 2 times, most recently from 0d3bd94 to 7d9f657 Compare July 19, 2021 22:06

update tests to use new interfaces

e67ce25

nabilschear force-pushed the namespaces branch from 7d9f657 to e67ce25 Compare July 20, 2021 19:45

add pid_with_namespace to groups table

882d844

theopolis requested changes Aug 20, 2021

View reviewed changes

suggested improvements from @theopolis

a23c5dd

theopolis requested changes Aug 21, 2021

View reviewed changes

osquery/tables/system/linux/groups.cpp Outdated Show resolved Hide resolved

osquery/tables/system/linux/user_groups.cpp Outdated Show resolved Hide resolved

osquery/tables/system/linux/users.cpp Outdated Show resolved Hide resolved

theopolis added this to the 5.0.0 milestone Aug 21, 2021

a few more syntax fixes

bdc0ae5

theopolis approved these changes Aug 23, 2021

View reviewed changes

Smjert approved these changes Aug 23, 2021

View reviewed changes

mike-myers-tob merged commit 482a751 into osquery:master Aug 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pid_with_namespace in more tables #7132

Support pid_with_namespace in more tables #7132

nabilschear commented Jun 1, 2021 •

edited

linux-foundation-easycla bot commented Jun 1, 2021 •

edited

theopolis commented Jun 2, 2021

nabilschear commented Jun 2, 2021

nabilschear commented Jun 3, 2021 •

edited

directionless commented Jun 3, 2021

nabilschear commented Jun 8, 2021

theopolis commented Jun 9, 2021

Smjert left a comment •

edited

nabilschear commented Jun 14, 2021 •

edited

nabilschear commented Jun 14, 2021 •

edited

nabilschear commented Jun 29, 2021

Smjert left a comment

nabilschear commented Jul 8, 2021

nabilschear commented Jul 16, 2021

nabilschear commented Jul 20, 2021

nabilschear commented Jul 28, 2021

nabilschear commented Aug 19, 2021

theopolis left a comment

nabilschear commented Aug 20, 2021

theopolis left a comment

	LOG(WARNING) << "Cannot read file that exceeds size limit: "
	<< path.string();
	VLOG(1) << "Cannot read " << path.string()
	<< " size exceeds limit: " << file_size << " > " << read_max;

Support pid_with_namespace in more tables #7132

Support pid_with_namespace in more tables #7132

Conversation

nabilschear commented Jun 1, 2021 • edited

linux-foundation-easycla bot commented Jun 1, 2021 • edited

theopolis commented Jun 2, 2021

nabilschear commented Jun 2, 2021

nabilschear commented Jun 3, 2021 • edited

directionless commented Jun 3, 2021

nabilschear commented Jun 8, 2021

theopolis commented Jun 9, 2021

Smjert left a comment • edited

Choose a reason for hiding this comment

nabilschear commented Jun 14, 2021 • edited

nabilschear commented Jun 14, 2021 • edited

nabilschear commented Jun 29, 2021

Smjert left a comment

Choose a reason for hiding this comment

nabilschear commented Jul 8, 2021

nabilschear commented Jul 16, 2021

nabilschear commented Jul 20, 2021

nabilschear commented Jul 28, 2021

nabilschear commented Aug 19, 2021

theopolis left a comment

Choose a reason for hiding this comment

nabilschear commented Aug 20, 2021

theopolis left a comment

Choose a reason for hiding this comment

nabilschear commented Jun 1, 2021 •

edited

linux-foundation-easycla bot commented Jun 1, 2021 •

edited

nabilschear commented Jun 3, 2021 •

edited

Smjert left a comment •

edited

nabilschear commented Jun 14, 2021 •

edited

nabilschear commented Jun 14, 2021 •

edited