-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add package to perform path lookups and validations for commands #1443
Conversation
fb809d1
to
e5bea46
Compare
e5bea46
to
cd71600
Compare
.golangci.yml
Outdated
forbidigo: | ||
forbid: | ||
- p: ^exec\.Command.*$ | ||
msg: use pkg/allowedpaths functions instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. Really glad to see a linter for this
25bed2a
to
c866ddc
Compare
c866ddc
to
7eca23e
Compare
fa71182
to
25c351e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the caveat that I did not review all the calls. This looks pretty good. I'd accept it, though I wanted to let other people take a look before approving.
There's a couple of small nits I saw, I think they're ignorable.
I really like the compile safety
pkg/allowedcmd/cmd_linux.go
Outdated
return validatedCommand(ctx, "/opt/CrowdStrike/falcon-kernel-check", arg...) | ||
} | ||
|
||
func Gnomeextensions(ctx context.Context, arg ...string) (*exec.Cmd, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func Gnomeextensions(ctx context.Context, arg ...string) (*exec.Cmd, error) { | |
func GnomeExtensions(ctx context.Context, arg ...string) (*exec.Cmd, error) { |
Not really important, but I'd probably use camel case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I have a bunch of these to update, then. I was waffling on how to capitalize anything with abbreviations in it (e.g. bputil
-- BPUtil
or BpUtil
) and this was my solution for not having to decide 😅 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have, at least, now adjusted casing for any command with punctuation in it (so falcon-kernel-check/Falconkernelcheck
is now FalconKernelCheck
and gnome-extensions/Gnomeextensions
is now GnomeExtensions
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks understandable/useable and much safer, appreciate all the comments and the new linter! looks great 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is pretty cool, much better. I did not review every single cmd that was updated, but went over a few to understand how it works.
Recommendations for reviewing
This PR got a little larger than I intended. This is the order I recommend reviewing the PR in and what I think it's useful to pay attention to in each section.
pkg/allowedcmd
-- this is the new package that handles allowlisting for binaries. I would look especially atvalidatedCommand
andallowSearchPath
, which is the majority of the logic changes for this PR. The rest is mostly boilerplate..golangci.yml
-- this contains a new linting rule to ensure we don't useexec.Command
andexec.CommandContext
in the future.pkg/ptycmd
-- this package was removed because it is not in use and would not allow for easy usage ofallowedcmd
.cmd
,ee
, andpkg
-- these are just updates to use the newallowedcmd
package. These are helpful to review to get a sense of how you can useallowedcmd
in the future.Description
Update all usages of
exec.Command
andexec.CommandContext
to no longer perform path lookups, instead using a newallowedcmd
package to allowlist paths to binaries. In the case where the path cannot be allowlisted (e.g. when using Nix, the install path may be in/nix/store
stored in an unknown location), perform lookup in the PATH.Links
Closes #833
Relates to #896
Testing notes
Areas to regression test
kolide_diskutil_list
,kolide_apfs_list
,kolide_apfs_users
,kolide_tmutil_destinationinfo
,kolide_powermetrics
,kolide_remotectl
,kolide_softwareupdate
,kolide_softwareupdate_scan
,kolide_touchid_system_config
,kolide_touchid_user_config
,kolide_mdm_info
,kolide_spotlight
,kolide_apple_silicon_security_policy
,kolide_filevault
,kolide_firmwarepasswd
,kolide_ioreg
,kolide_mdmclient
,kolide_profiles
,kolide_pwpolicy
,kolide_system_profiler
kolide_lsblk
,kolide_apt_upgradeable
,kolide_dpkg_version_info
,kolide_cryptsetup_status
,kolide_gsettings
,kolide_gsettings_metadata
,kolide_xrdb
kolide_dsregcmd
,kolide_dsim_default_associations
,kolide_secedit