From d7bead3f6584782059809c64a27fa8b75c2a330d Mon Sep 17 00:00:00 2001 From: Jeff Mahoney Date: Mon, 6 Jun 2022 16:19:36 -0400 Subject: [PATCH 1/3] rule: Report missing rules during deletion Although the implementation for AuditClient.Delete is only used to back AuditClient.DeleteAll, we'd like to be able to delete individual rules. This commit adds checking of the netlink error field and reports when the deletion has failed. When DeleteAll is called, we ignore the ENOENT return since it could've raced somewhere and we don't actually care since we're deleting all of the rules. --- audit.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/audit.go b/audit.go index 1a4c1b4..d1a7720 100644 --- a/audit.go +++ b/audit.go @@ -242,7 +242,7 @@ func (c *AuditClient) DeleteRules() (int, error) { } for i, rule := range rules { - if err := c.DeleteRule(rule); err != nil { + if err := c.DeleteRule(rule); err != nil && !errors.Is(err, syscall.ENOENT) { return 0, fmt.Errorf("failed to delete rule %v of %v: %w", i, len(rules), err) } } @@ -251,6 +251,8 @@ func (c *AuditClient) DeleteRules() (int, error) { } // DeleteRule deletes the given rule (specified in binary format). +// This can return errors from the underlying subsystem calls. syscall.ENOENT can +// be safely ignored as it indicates the rule to be deleted doesn't exist. func (c *AuditClient) DeleteRule(rule []byte) error { msg := syscall.NetlinkMessage{ Header: syscall.NlMsghdr{ @@ -266,11 +268,19 @@ func (c *AuditClient) DeleteRule(rule []byte) error { return fmt.Errorf("failed sending delete rule request: %w", err) } - _, err = c.getReply(seq) + ack, err := c.getReply(seq) if err != nil { return fmt.Errorf("failed to get ACK to rule delete request: %w", err) } + if ack.Header.Type != syscall.NLMSG_ERROR { + return fmt.Errorf("unexpected ACK to AUDIT_DEL_RULE, got type=%d", ack.Header.Type) + } + + if err = ParseNetlinkError(ack.Data); err != nil { + return fmt.Errorf("error deleting audit rule: %w", err) + } + return nil } From 95acdd87868c826afef692338e85ef8a27ac1b2d Mon Sep 17 00:00:00 2001 From: Jeff Mahoney Date: Mon, 6 Jun 2022 16:21:39 -0400 Subject: [PATCH 2/3] rule.Rule.Build: Don't assume that no syscalls means all syscalls Rule.Build assumes that if no syscalls are specified they all are set. This is really only the case when the exit list is used since the syscall numbers aren't available in the other lists. When we assume that all of the syscalls are enabled, we end up generating wireformat rules for e.g. 'task,never' that have all of the syscall bits set. That doesn't match what is already used when 'auditctl -a task,never' is used. It may be ignored by the kernel when such a rule is added, but it would cause problems when that rule is deleted. --- rule/rule.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/rule/rule.go b/rule/rule.go index 33e887e..4e1adf8 100644 --- a/rule/rule.go +++ b/rule/rule.go @@ -41,7 +41,7 @@ const ( // Build builds an audit rule. func Build(rule Rule) (WireFormat, error) { - data := &ruleData{allSyscalls: true} + data := &ruleData{} var err error switch v := rule.(type) { @@ -49,6 +49,14 @@ func Build(rule Rule) (WireFormat, error) { if err = data.setList(v.List); err != nil { return nil, err } + + // While it's possible to set syscalls on lists other than the 'exit' list + // they don't actually do anything since the syscall information isn't + // available at that time. Don't assume that all syscalls are enabled. + if data.flags == exitFilter { + data.allSyscalls = true + } + if err = data.setAction(v.Action); err != nil { return nil, err } From b0c222766f53b24fe706f54377ef12f5a107d33d Mon Sep 17 00:00:00 2001 From: Jeff Mahoney Date: Mon, 6 Jun 2022 16:25:16 -0400 Subject: [PATCH 3/3] rule: Add support for removing individual rules We currently don't handle the '-d' or '-W' options that would remove list rules or file watches. This commit adds support to handle those properly. rule.ToCommandLine still returns the expected result, but I've added a rule.ToCommandLineAddRemove that takes a bool indicating whether the rule would be added or removed. This was required to do testing of deletion rules. --- audit_test.go | 129 +++++++++++++++++++++++++++++++++++++++++++- rule/flags/flags.go | 96 +++++++++++++++++++++++++-------- rule/rule.go | 35 ++++++++++-- rule/types.go | 10 ++-- 4 files changed, 238 insertions(+), 32 deletions(-) diff --git a/audit_test.go b/audit_test.go index 816662c..d0521af 100644 --- a/audit_test.go +++ b/audit_test.go @@ -797,7 +797,7 @@ func TestAuditClientSetImmutable(t *testing.T) { assert.EqualValues(t, 2, status.Enabled) } -func TestRuleParsing(t *testing.T) { +func TestValidRuleParsing(t *testing.T) { var rules []string switch runtime.GOARCH { case "386": @@ -833,6 +833,20 @@ func TestRuleParsing(t *testing.T) { "-a always,user -F uid=root", "-a always,task -F uid=root", "-a always,exit -S mount -F pid=1234", + "-d always,exit -F arch=b64 -S execve,execveat -F key=exec", + "-d never,exit -F arch=b64 -S connect,accept,bind -F key=external-access", + "-W /etc/group -p wa", + "-W /etc/passwd -p rx", + "-W /etc/gshadow -p rwxa", + "-W /tmp/test -p rwa", + "-d always,exit -F arch=b64 -S open,truncate,ftruncate,creat,openat,open_by_handle_at -F exit=-EACCES -F key=access", + "-d never,exit -F arch=b64 -S open,truncate,ftruncate,creat,openat,open_by_handle_at -F exit=-EPERM -F key=access", + "-d always,exit -F arch=b32 -S open -F key=admin -F uid=root -F gid=root -F exit=33 -F path=/tmp -F perm=rwxa", + "-d always,exit -F arch=b64 -S open -F key=key -F uid=30000 -F gid=333 -F exit=-151111 -F filetype=fifo", + "-d never,exclude -F msgtype=GRP_CHAUTHTOK", + "-d always,user -F uid=root", + "-d always,task -F uid=root", + "-d always,exit -S mount -F pid=1234", } default: // Can't have multiple syscall testing as ordering of individual syscalls @@ -851,6 +865,19 @@ func TestRuleParsing(t *testing.T) { "-a always,user -F uid=root", "-a always,task -F uid=root", "-a always,exit -S mount -F pid=1234", + "-d always,exit -S execve -F key=exec", + "-W /etc/group -p wa", + "-W /etc/passwd -p rx", + "-W /etc/gshadow -p rwxa", + "-W /tmp/test -p rwa", + "-d always,exit -S all -F exit=-EACCES -F key=access", + "-d never,exit -S all -F exit=-EPERM -F key=access", + "-d always,exit -S open -F key=admin -F uid=root -F gid=root -F exit=33 -F path=/tmp -F perm=rwxa", + "-d always,exit -S open -F key=key -F uid=30000 -F gid=333 -F exit=-151111 -F filetype=fifo", + "-d never,exclude -F msgtype=GRP_CHAUTHTOK", + "-d always,user -F uid=root", + "-d always,task -F uid=root", + "-d always,exit -S mount -F pid=1234", } } t.Logf("checking %d rules", len(rules)) @@ -864,7 +891,12 @@ func TestRuleParsing(t *testing.T) { if err != nil { t.Fatal(err, msg) } - cmdline, err := rule.ToCommandLine(data, true) + addRule := true + switch r.TypeOf() { + case rule.DeleteFileWatchRuleType, rule.DeleteSyscallRuleType: + addRule = false + } + cmdline, err := rule.ToCommandLineAddRemove(data, true, addRule) if err != nil { t.Fatal(err, msg) } @@ -872,6 +904,99 @@ func TestRuleParsing(t *testing.T) { } } +func TestInvalidRuleParsing(t *testing.T) { + rules := []string{ + "-D -a", + "-D -A", + "-D -d", + + "-D -a always", + "-D -A always", + "-D -d always", + + "-D -a never", + "-D -A never", + "-D -d never", + + "-D -a always,task", + "-D -A always,task", + "-D -d always,task", + + "-D -a always,task -w /foo/bar -p rw", + "-D -a always,task -W /foo/bar -p rw", + "-D -A always,task -w /foo/bar -p rw", + "-D -A always,task -W /foo/bar -p rw", + "-D -d always,task -w /foo/bar -p rw", + "-D -d always,task -W /foo/bar -p rw", + + "-D -a never,task", + "-D -A never,task", + "-D -d never,task", + + "-D -w /foo/bar", + "-D -W /foo/bar", + "-D -w /foo/bar -p rw", + "-D -W /foo/bar -p rw", + "-D -w /foo/bar -p garbage", + "-D -W /foo/bar -p garbage", + + "-w /foo/bar -W /foo/bar", + "-w /foo/bar -W /foo/bar -p rw", + + "-w foo/bar", + "-W foo/bar", + "-w foo/bar -p rw", + "-W foo/bar -p rw", + + "-w foo/bar -S 42", + "-W foo/bar -S 42", + + "-w foo/bar -W foo/bar", + "-w foo/bar -W foo/bar -p rw", + + "-w foo/bar -W foo/bar -S 42", + + "-w /foo/bar -F uid=100", + "-W /foo/bar -F uid=100", + + "-w /foo/bar -S 42", + "-W /foo/bar -S 42", + + "-w /foo/bar -F uid=100", + "-W /foo/bar -F uid=100", + + "-w /foo/bar -C auid!=uid", + "-W /foo/bar -C auid!=uid", + + "-a always,exit -w /foo/bar -p rw", + "-a always,exit -W /foo/bar -p rw", + "-A always,exit -w /foo/bar -p rw", + "-A always,exit -W /foo/bar -p rw", + + "-a always,exit -w /foo/bar", + "-a always,exit -W /foo/bar", + "-A always,exit -w /foo/bar", + "-A always,exit -W /foo/bar", + } + + t.Logf("checking %d rules", len(rules)) + for idx, line := range rules { + r, err := flags.Parse(line) + if err != nil { + t.Log(err) + continue + } + + _, err = rule.Build(r) + if err != nil { + t.Log(err) + continue + } + + t.Errorf("parsing line #%d: `%s' should have failed", idx, line) + } +} + func extractDecimalNumber(s []int8, pos int) (value, nextPos int) { for value = 0; ; pos++ { c := s[pos] diff --git a/rule/flags/flags.go b/rule/flags/flags.go index e1efe9b..3317af5 100644 --- a/rule/flags/flags.go +++ b/rule/flags/flags.go @@ -58,14 +58,19 @@ func Parse(s string) (rule.Rule, error) { Type: rule.DeleteAllRuleType, Keys: ruleFlagSet.Key, } - case rule.FileWatchRuleType: - r = &rule.FileWatchRule{ - Type: rule.FileWatchRuleType, + case rule.FileWatchRuleType, rule.DeleteFileWatchRuleType: + fileWatchRule := &rule.FileWatchRule{ + Type: ruleFlagSet.Type, Path: ruleFlagSet.Path, Permissions: ruleFlagSet.Permissions, Keys: ruleFlagSet.Key, } - case rule.AppendSyscallRuleType, rule.PrependSyscallRuleType: + r = fileWatchRule + + if ruleFlagSet.Type == rule.DeleteFileWatchRuleType { + fileWatchRule.Path = ruleFlagSet.DeletePath + } + case rule.AppendSyscallRuleType, rule.PrependSyscallRuleType, rule.DeleteSyscallRuleType: syscallRule := &rule.SyscallRule{ Type: ruleFlagSet.Type, Filters: ruleFlagSet.Filters, @@ -74,12 +79,16 @@ func Parse(s string) (rule.Rule, error) { } r = syscallRule - if ruleFlagSet.Type == rule.AppendSyscallRuleType { + switch ruleFlagSet.Type { + case rule.AppendSyscallRuleType: syscallRule.List = ruleFlagSet.Append.List syscallRule.Action = ruleFlagSet.Append.Action - } else if ruleFlagSet.Type == rule.PrependSyscallRuleType { + case rule.PrependSyscallRuleType: syscallRule.List = ruleFlagSet.Prepend.List syscallRule.Action = ruleFlagSet.Prepend.Action + case rule.DeleteSyscallRuleType: + syscallRule.List = ruleFlagSet.Delete.List + syscallRule.Action = ruleFlagSet.Delete.Action } default: return nil, fmt.Errorf("unknown rule type: %v", ruleFlagSet.Type) @@ -99,11 +108,13 @@ type ruleFlagSet struct { // Audit Rule Prepend addFlag // -A Prepend rule (list,action) or (action,list). Append addFlag // -a Append rule (list,action) or (action,list). + Delete addFlag // [-d] delete single rule Filters filterList // -F [n=v | n!=v | nv | n<=v | n>=v | n&v | n&=v] OR -C [n=v | n!=v] Syscalls stringList // -S Syscall name or number or "all". Value can be comma-separated. // Filepath watch (can be done more expressively using syscalls) Path string // -w Path for filesystem watch (no wildcards). + DeletePath string // -W Path for filesystem watch to remove (no wildcards). Permissions fileAccessTypeFlags // -p [r|w|x|a] Permission filter. Key stringList // -k Key(s) to associate with the rule. @@ -120,11 +131,13 @@ func newRuleFlagSet() *ruleFlagSet { rule.flagSet.BoolVar(&rule.DeleteAll, "D", false, "delete all") rule.flagSet.Var(&rule.Append, "a", "append rule") rule.flagSet.Var(&rule.Prepend, "A", "prepend rule") + rule.flagSet.Var(&rule.Delete, "d", "delete rule") rule.flagSet.Var((*interFieldFilterList)(&rule.Filters), "C", "comparison filter") rule.flagSet.Var((*valueFilterList)(&rule.Filters), "F", "filter") rule.flagSet.Var(&rule.Syscalls, "S", "syscall name, number, or 'all'") rule.flagSet.Var(&rule.Permissions, "p", "access type - r=read, w=write, x=execute, a=attribute change") rule.flagSet.StringVar(&rule.Path, "w", "", "path to watch, no wildcards") + rule.flagSet.StringVar(&rule.DeletePath, "W", "", "path to unwatch, no wildcards") rule.flagSet.Var(&rule.Key, "k", "key") return rule @@ -140,51 +153,92 @@ func (r *ruleFlagSet) Usage() string { func (r *ruleFlagSet) validate() error { var ( - deleteAll uint8 - fileWatch uint8 - syscall uint8 + deleteAll uint8 + fileWatchFlags uint8 + addFileWatch uint8 + deleteFileWatch uint8 + syscallFlags uint8 + addSyscall uint8 + deleteSyscall uint8 ) r.flagSet.Visit(func(f *flag.Flag) { switch f.Name { case "D": deleteAll = 1 - case "w", "p": - fileWatch = 1 - case "a", "A", "C", "F", "S": - syscall = 1 + case "p": + fileWatchFlags = 1 + case "w": + addFileWatch = 1 + case "W": + deleteFileWatch = 1 + case "S", "F", "C": + syscallFlags = 1 + case "a", "A": + addSyscall = 1 + case "d": + deleteSyscall = 1 } }) // Test for mutual exclusivity. - switch deleteAll + fileWatch + syscall { + switch deleteAll + addFileWatch + deleteFileWatch + addSyscall + deleteSyscall { case 0: return errors.New("missing an operation flag (add or delete rule)") case 1: switch { case deleteAll > 0: r.Type = rule.DeleteAllRuleType - case fileWatch > 0: + case addFileWatch > 0: r.Type = rule.FileWatchRuleType - case syscall > 0: + case deleteFileWatch > 0: + r.Type = rule.DeleteFileWatchRuleType + case addSyscall > 0: r.Type = rule.AppendSyscallRuleType + case deleteSyscall > 0: + r.Type = rule.DeleteSyscallRuleType + default: + return fmt.Errorf("unknown state") } default: ops := make([]string, 0, 3) if deleteAll > 0 { ops = append(ops, "delete all [-D]") } - if fileWatch > 0 { - ops = append(ops, "file watch [-w|-p]") + if addFileWatch > 0 { + ops = append(ops, "file watch [-w]") + } + if deleteFileWatch > 0 { + ops = append(ops, "delete file watch [-W]") } - if syscall > 0 { - ops = append(ops, "audit rule [-a|-A|-S|-C|-F]") + if addSyscall > 0 { + ops = append(ops, "audit rule [-a|-A]") } + if deleteSyscall > 0 { + ops = append(ops, "delete audit rule [-d]") + } + return fmt.Errorf("mutually exclusive flags uses together (%v)", strings.Join(ops, " and ")) } - if syscall > 0 { + if (addFileWatch+deleteFileWatch) > 0 && syscallFlags > 0 { + return fmt.Errorf("audit rule [-F|-S|-C] flags cannot be used with file watches") + } + + if (addSyscall+deleteSyscall) > 0 && fileWatchFlags > 0 { + return fmt.Errorf("file watch [-p] flags cannot be used with syscall rules") + } + + if deleteAll > 0 && syscallFlags > 0 { + return fmt.Errorf("audit rule [-F|-S|-C] flags cannot be used when deleting all rules") + } + + if deleteAll > 0 && fileWatchFlags > 0 { + return fmt.Errorf("file watch permission [-p] flags cannot be used when deleting all rules") + } + + if addSyscall > 0 { var zero addFlag if r.Prepend == zero && r.Append == zero { return errors.New("audit rules must specify either [-A] or [-a]") diff --git a/rule/rule.go b/rule/rule.go index 4e1adf8..f5555f7 100644 --- a/rule/rule.go +++ b/rule/rule.go @@ -111,6 +111,22 @@ func Build(rule Rule) (WireFormat, error) { // the current machine. This is misleading, so this code will print the actual // architecture. func ToCommandLine(wf WireFormat, resolveIds bool) (rule string, err error) { + return ToCommandLineAddRemove(wf, resolveIds, true) +} + +// ToCommandLineAddRemove decodes a WireFormat into a command-line rule. +// When resolveIds is set, it tries to resolve the argument to UIDs, GIDs, +// file_type fields. +// When addRule is set, it indicates the rule is being added to the system. +// When addRule is unset, it indicates the rule is being removed from the system. +// `auditctl -l` always prints the numeric (non-resolved) representation of +// this fields, so when the flag is set to false, the output is the same as +// auditctl. +// There is an exception to this rule when parsing the `arch` field: +// auditctl always prints "b64" or "b32" even for architectures other than +// the current machine. This is misleading, so this code will print the actual +// architecture. +func ToCommandLineAddRemove(wf WireFormat, resolveIds, addRule bool) (rule string, err error) { ar, err := fromWireFormat(wf) if err != nil { return "", fmt.Errorf("failed to parse wire format: %w", err) @@ -162,7 +178,15 @@ func ToCommandLine(wf WireFormat, resolveIds bool) (rule string, err error) { } } if !extraFields { - arguments := []string{"-w", path, "-p", permission(r.values[permIdx]).String()} + addRemove := "-w" + if !addRule { + addRemove = "-W" + } + + arguments := []string{ + addRemove, path, "-p", + permission(r.values[permIdx]).String(), + } if len(key) > 0 { arguments = append(arguments, "-k", key) } @@ -171,12 +195,13 @@ func ToCommandLine(wf WireFormat, resolveIds bool) (rule string, err error) { } // Parse rule as syscall type - - arguments := []string{ - "-a", - fmt.Sprintf("%s,%s", act, list), + addRemove := "-a" + if !addRule { + addRemove = "-d" } + arguments := []string{addRemove, fmt.Sprintf("%s,%s", act, list)} + // Parse arch field first, if present // Here there is a significant difference to what auditctl does. // Auditctl will allow to install a rule for a different platform diff --git a/rule/types.go b/rule/types.go index cc6d498..d8102e2 100644 --- a/rule/types.go +++ b/rule/types.go @@ -24,10 +24,12 @@ type Type int // The rule types supported by this package. const ( - DeleteAllRuleType Type = iota + 1 // DeleteAllRule - FileWatchRuleType // FileWatchRule - AppendSyscallRuleType // SyscallRule - PrependSyscallRuleType // SyscallRule + DeleteAllRuleType Type = iota + 1 // DeleteAllRule + FileWatchRuleType // FileWatchRule + DeleteFileWatchRuleType // FileWatchRule + AppendSyscallRuleType // SyscallRule + PrependSyscallRuleType // SyscallRule + DeleteSyscallRuleType // SyscallRule ) // Rule is the generic interface that all rule types implement.