From 3ccce25f9b33acef896a9cdbf9dc8f2c7be1da7e Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Mon, 18 Nov 2024 17:30:36 +0000 Subject: [PATCH 1/3] Refactor how the deleted files are procossed in apparmor profile Change-Id: I6fe2e9669c8f6c58f141aa77be3626581c5c07ce Signed-off-by: Cosmin Cojocar --- .../bpfrecorder/bpfrecorder_apparmor.go | 48 +++++++++++-------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go index be6a50f9a..dfde05539 100644 --- a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go +++ b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go @@ -264,26 +264,7 @@ func (b *AppArmorRecorder) processExecFsEvents(mid mntnsID) BpfAppArmorFileProce } for fileName, access := range b.recordedFiles[mid] { - // Workaround for HUGETLB support with apparmor: - // AppArmor treats mmap(..., MAP_ANONYMOUS | MAP_HUGETLB) calls as - // file access to "", which is then attached to "/" (attach_disconnected). - // So for HUGETLB to work with AppArmor, we need a `/ rw` rule in our profile. - // (note that there is no wildcard here - subdirectories/files are not affected). - // https://gitlab.com/apparmor/apparmor/-/issues/345 - // - // At the same time, eBPF's bpf_d_path is also slightly confused and reports - // access to a path named "/anon_hugepage (deleted)" on mmap. Instead of building complex - // workarounds and hooking mmap, we just treat that as a canary for HUGETLB usage. - if fileName == "/anon_hugepage (deleted)" { - processedEvents.ReadWritePaths = append(processedEvents.ReadWritePaths, "/") - continue - } - - // This is returned by the kernel when a dentry is removed. - // https://github.com/torvalds/linux/blob/2e1b3cc9d7f790145a80cb705b168f05dab65df2/fs/d_path.c#L255-L288 - // - // It should be ignored since is an invalid path in the apparmor profile. - if fileName == "/ (deleted)" { + if ok := b.processDeletedFiles(fileName, &processedEvents); ok { continue } @@ -322,6 +303,33 @@ func (b *AppArmorRecorder) processExecFsEvents(mid mntnsID) BpfAppArmorFileProce return processedEvents } +func (b *AppArmorRecorder) processDeletedFiles(fileName string, processedEvents *BpfAppArmorFileProcessed) bool { + // Workaround for HUGETLB support with apparmor: + // AppArmor treats mmap(..., MAP_ANONYMOUS | MAP_HUGETLB) calls as + // file access to "", which is then attached to "/" (attach_disconnected). + // So for HUGETLB to work with AppArmor, we need a `/ rw` rule in our profile. + // (note that there is no wildcard here - subdirectories/files are not affected). + // https://gitlab.com/apparmor/apparmor/-/issues/345 + // + // At the same time, eBPF's bpf_d_path is also slightly confused and reports + // access to a path named "/anon_hugepage (deleted)" on mmap. Instead of building complex + // workarounds and hooking mmap, we just treat that as a canary for HUGETLB usage. + if fileName == "/anon_hugepage (deleted)" { + processedEvents.ReadWritePaths = append(processedEvents.ReadWritePaths, "/") + return true + } + + // This is returned by the kernel when a dentry is removed. + // https://github.com/torvalds/linux/blob/2e1b3cc9d7f790145a80cb705b168f05dab65df2/fs/d_path.c#L255-L288 + // + // It should be ignored since is an invalid path in the apparmor profile. + if fileName == "/ (deleted)" { + return true + } + + return false +} + func (b *AppArmorRecorder) processCapabilities(mid mntnsID) []string { if _, ok := b.recordedCapabilities[mid]; !ok { return []string{} From 0684f7ffdc83a987c9089014008ed824ff3b3966 Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Mon, 18 Nov 2024 18:14:06 +0000 Subject: [PATCH 2/3] apparmor: allow any files in a directory if at least two files have read-write premissions Change-Id: Id5799bc8897d3579bf7518d54ef243510b9ba361 Signed-off-by: Cosmin Cojocar --- .../bpfrecorder/bpfrecorder_apparmor.go | 34 +++++++++++++++- .../bpfrecorder/bpfrecorder_apparmor_test.go | 40 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go index dfde05539..c2c40c913 100644 --- a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go +++ b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "log" + "path/filepath" "regexp" "slices" "strings" @@ -264,7 +265,7 @@ func (b *AppArmorRecorder) processExecFsEvents(mid mntnsID) BpfAppArmorFileProce } for fileName, access := range b.recordedFiles[mid] { - if ok := b.processDeletedFiles(fileName, &processedEvents); ok { + if ok := processDeletedFiles(fileName, &processedEvents); ok { continue } @@ -294,6 +295,14 @@ func (b *AppArmorRecorder) processExecFsEvents(mid mntnsID) BpfAppArmorFileProce } } + // Allow any files in a directory if already at least two files are allowed to have read-write + // permissions. There are binaries like nginx which typically create files with random name on + // every start in the /etc/nginx/config.d/ directory. These random named files cannot be captured + // in advance and allowed in the apparmor profile. This logic SHOULD NOT be applied to read-only + // files because in that case the file paths are static and should be captured up-front by the + // recorder. + processedEvents.ReadWritePaths = allowAnyFiles(processedEvents.ReadWritePaths) + slices.Sort(processedEvents.AllowedExecutables) slices.Sort(processedEvents.AllowedLibraries) slices.Sort(processedEvents.ReadOnlyPaths) @@ -303,7 +312,8 @@ func (b *AppArmorRecorder) processExecFsEvents(mid mntnsID) BpfAppArmorFileProce return processedEvents } -func (b *AppArmorRecorder) processDeletedFiles(fileName string, processedEvents *BpfAppArmorFileProcessed) bool { +// processDeletedFiles process file paths which are marked as deleted by the Linux kernel. +func processDeletedFiles(fileName string, processedEvents *BpfAppArmorFileProcessed) bool { // Workaround for HUGETLB support with apparmor: // AppArmor treats mmap(..., MAP_ANONYMOUS | MAP_HUGETLB) calls as // file access to "", which is then attached to "/" (attach_disconnected). @@ -330,6 +340,26 @@ func (b *AppArmorRecorder) processDeletedFiles(fileName string, processedEvents return false } +// allowAnyFiles allows any file in a directory if more than two files are allowed. +func allowAnyFiles(filePaths []string) []string { + dupDirs := map[string]int{} + for _, fp := range filePaths { + dir := filepath.Dir(fp) + dupDirs[dir] += 1 + } + result := []string{} + for _, fp := range filePaths { + dir := filepath.Dir(fp) + if dupDirs[dir] > 1 { + result = append(result, filepath.Join(dir, "*")) + dupDirs[dir] = 0 + } else if dupDirs[dir] == 1 { + result = append(result, fp) + } + } + return result +} + func (b *AppArmorRecorder) processCapabilities(mid mntnsID) []string { if _, ok := b.recordedCapabilities[mid]; !ok { return []string{} diff --git a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go index 398b2d81f..5c083a889 100644 --- a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go +++ b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go @@ -63,3 +63,43 @@ func TestReplaceVarianceInFilePath(t *testing.T) { }) } } + +func TestAllowAnyFiles(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + paths []string + want []string + }{ + { + name: "allow any files if at least two files are already allowed", + paths: []string{"/etc/nginx/conf.d/default.conf", "/dev/null", "/etc/nginx/conf.d/sedIWASqqq"}, + want: []string{"/etc/nginx/conf.d/*", "/dev/null"}, + }, + { + name: "allow any files if more than two files are already allowed", + paths: []string{"/etc/nginx/conf.d/default.conf", "/dev/null", + "/etc/nginx/conf.d/sedIWASqqq", "/etc/nginx/conf.d/abcd"}, + want: []string{"/etc/nginx/conf.d/*", "/dev/null"}, + }, + { + name: "do not allow any files ", + paths: []string{"/etc/nginx/conf.d/default.conf", "/dev/null"}, + want: []string{"/etc/nginx/conf.d/default.conf", "/dev/null"}, + }, + { + name: "do not allow anything if nothing is allowed", + paths: []string{}, + want: []string{}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + got := allowAnyFiles(test.paths) + require.Equal(t, test.want, got) + }) + } +} From 051fe5e306a177eda582ff15a0417d1148f6217c Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Mon, 18 Nov 2024 18:18:17 +0000 Subject: [PATCH 3/3] Format the files Change-Id: I3dc2bcc46ba2e416888bb7b53e6fc13ad6b060f6 Signed-off-by: Cosmin Cojocar --- .../pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go index 5c083a889..7ed7e5ebb 100644 --- a/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go +++ b/internal/pkg/daemon/bpfrecorder/bpfrecorder_apparmor_test.go @@ -79,8 +79,10 @@ func TestAllowAnyFiles(t *testing.T) { }, { name: "allow any files if more than two files are already allowed", - paths: []string{"/etc/nginx/conf.d/default.conf", "/dev/null", - "/etc/nginx/conf.d/sedIWASqqq", "/etc/nginx/conf.d/abcd"}, + paths: []string{ + "/etc/nginx/conf.d/default.conf", "/dev/null", + "/etc/nginx/conf.d/sedIWASqqq", "/etc/nginx/conf.d/abcd", + }, want: []string{"/etc/nginx/conf.d/*", "/dev/null"}, }, {