Skip to content
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.

Sign up for GitHub

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 attributes to exclude fstypes and paths from suid_check find #94

Closed
wants to merge 1 commit into from
Closed

add attributes to exclude fstypes and paths from suid_check find #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion controls/os_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@
description: 'blacklist of suid/sgid program on system'
)

find_exclude_paths = attribute(
'find_exclude_paths',
default: suid_check.default_exclude_paths,
description: 'exclude paths from being walked by find for permissions issues'
)

find_exclude_fstypes = attribute(
'find_exclude_fstypes',
default: suid_check.default_exclude_fstypes,
description: 'exclude mounts with these fstypes from being walked by find for permissions issues'
)

control 'os-01' do
impact 1.0
title 'Trusted hosts login'
Expand Down Expand Up @@ -158,7 +170,7 @@
title 'Check for SUID/ SGID blacklist'
desc 'Find blacklisted SUID and SGID files to ensure that no rogue SUID and SGID files have been introduced into the system'

describe suid_check(blacklist) do
describe suid_check(blacklist, exclude_fstypes: find_exclude_fstypes, exclude_paths: find_exclude_paths) do
its('diff') { should be_empty }
end
end
Expand Down
31 changes: 27 additions & 4 deletions libraries/suid_check.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,44 @@ class SUIDCheck < Inspec.resource(1)
name 'suid_check'
desc 'Use the suid_check resource to verify the current SUID/SGID against a blacklist'
example "
describe suid_check(blacklist) do
describe suid_check(blacklist, exclude_fstypes: %w[nfs nfs4], exclude_paths: %w[/proc]) do
its('diff') { should be_empty }
end
"

def initialize(blacklist = nil)
blacklist = default if blacklist.nil?
def initialize(blacklist = nil, exclude_fstypes: nil, exclude_paths: nil)
@blacklist = blacklist
@exclude_fstypes = exclude_fstypes || default_exclude_fstypes
@exclude_paths = exclude_paths || default_exclude_paths
end

def default_exclude_fstypes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By adding those, we change the default behaviour, which results in a breaking change. @arlimus @atomic111 we have two options:

  • release a new major version
  • keep the old behaviour but allow the override
    What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My $0.02 is that it should be the default behavior and have a major version bump because the current default of walking network filesystems is prohibitively expensive and redundant when you have a lot of nodes mounting the same share. If you're using the audit cookbook to run this profile in that scenario, some nodes could take longer than the Chef run interval to even complete. If you're using push jobs on large groups, the job will take as long as the longest node, so using the current version of this profile has increased deploy times as well. IMO, if you want to check network shares for SUID, that should be done by a single ad-hoc process somewhere.

%w[nfs nfs4 cifs smp nbd ceph]
end

def default_exclude_paths
%w[/sys /proc /var/lib/lxd/containers]
end

def permissions
output = inspec.command('find / -perm -4000 -o -perm -2000 -type f ! -path \'/proc/*\' ! -path \'/var/lib/lxd/containers/*\' -print 2>/dev/null | grep -v \'^find:\'')
exclude_opts = exclude_paths.map { |p| "-path #{p} -prune" }.join(' -o ')
output = inspec.command("find / \\( #{exclude_opts} \\) -o -perm -4000 -perm -2000 -type f -print 2>/dev/null " \
"| grep -v '^find:'")
output.stdout.split(/\r?\n/)
end

def exclude_fstype_paths
mounts = inspec.command('mount')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we open a ticket in inspec to expose their point parsing? https://github.com/inspec/inspec/blob/master/lib/resources/mount.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears they're just inspecting files to see if they are mounts and getting their options from mount if so (https://github.com/inspec/inspec/blob/master/lib/resources/file.rb#L94), rather than parsing all mounts. I don't think the two are really compatible. Individual objects vs. "walking" mounts.

mounts.stdout.split("\n").map do |m|
match = m.match(/ on (?<mount_path>.*) type (?<fstype>\w+) /)
match['mount_path'] if !match.nil? && @exclude_fstypes.include?(match['fstype'])
end.compact
end

def exclude_paths
exclude_fstype_paths + @exclude_paths
end

def diff
permissions & @blacklist
end
Expand Down