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

s/apparmor: include files in apparmor kernel features #15130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+79 −23
Open

s/apparmor: include files in apparmor kernel features #15130

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
12 changes: 5 additions & 7 deletions sandbox/apparmor/apparmor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,8 +664,6 @@ func probeKernelFeatures() ([]string, error) {
}
}
if data, err := os.ReadFile(filepath.Join(rootPath, featuresSysPath, "policy", "notify", "user")); err == nil {
// XXX: there's no feature added for policy:notify:user, since user is
// a file rather than a directory.
notifyUserFeatures := strings.Fields(string(data))
for _, feat := range notifyUserFeatures {
features = append(features, "policy:notify:user:"+feat)
Expand All @@ -683,12 +681,12 @@ func probeKernelFeaturesInDirRecursively(dir string, prefix string) ([]string, e
}
features := make([]string, 0, len(dentries))
for _, fi := range dentries {
featureName := fi.Name()
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this make /sys/kernel/securtiy/apparmor/features/capability show up as a kernel feature now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as-is it would show up as a feature. The file itself won't be read, though. Is this something we want to avoid?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if capability is indicative of any particular feature in AppArmor, I'm assuming it's not and as such it'd be better to skip it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like the boolean files we care about should all be subdirectories of /sys/kernel/security/apparmor/features/policy/, so perhaps this change could be narrowed to only include files under that directory? I don't think there's harm in including all files, but we could be more selective.

More info on the tagging support checks here, for example: https://docs.google.com/document/d/1_WvEM9Qi2Je2Vwulzv5TzHwLJ8ld0W8gTNESZZd9VsY/edit?tab=t.0#heading=h.f728esb5to7y

Copy link
Contributor

Choose a reason for hiding this comment

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

looking at https://elixir.bootlin.com/linux/v6.13.4/source/security/apparmor/apparmorfs.c#L2404 capability seems to be an actual 'feature'. It's actually quite confusing how we're approaching this e.g. there's just one network_v8 entry, even though network_v8/af_mask has multiple values inside, but we have 10+ entries for kernel:policy:permstable32:.*. Maybe we should have been more selective about the features we list and only include the ones we know/care about rather than adding all of them simply because entries are there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reply from the kernel team about what it is/means was this:

it lists the capabilities that are supported in that kernel. capabilities are not encoded the same way that the other rules are (using the dfa). each capability is represented as a bit of a uint64_t

if prefix != "" {
featureName = prefix + ":" + fi.Name()
}
features = append(features, featureName)
if fi.IsDir() {
featureName := fi.Name()
if prefix != "" {
featureName = prefix + ":" + fi.Name()
}
features = append(features, featureName)
subFeatures, err := probeKernelFeaturesInDirRecursively(filepath.Join(dir, fi.Name()), featureName)
if err != nil {
return []string{}, err
Expand Down
90 changes: 74 additions & 16 deletions sandbox/apparmor/apparmor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,52 +299,110 @@ func (s *apparmorSuite) TestProbeAppArmorKernelFeatures(c *C) {
c.Assert(err, IsNil)
c.Check(features, DeepEquals, []string{"bar", "foo", "foo:baz", "foo:qux", "xyz"})

// Also test boolean file features
file, err := os.OpenFile(filepath.Join(d, featuresSysPath, "bar", "feat1"), os.O_CREATE, 0o644)
c.Assert(err, IsNil)
c.Assert(file.Close(), IsNil)
file, err = os.OpenFile(filepath.Join(d, featuresSysPath, "bar", "feat2"), os.O_CREATE, 0o644)
c.Assert(err, IsNil)
c.Assert(file.Close(), IsNil)
file, err = os.OpenFile(filepath.Join(d, featuresSysPath, "foo", "qux", "v3"), os.O_CREATE, 0o644)
c.Assert(err, IsNil)
c.Assert(file.Close(), IsNil)
file, err = os.OpenFile(filepath.Join(d, featuresSysPath, "foo", "qux", "v5"), os.O_CREATE, 0o644)
c.Assert(err, IsNil)
c.Assert(file.Close(), IsNil)
features, err = apparmor.ProbeKernelFeatures()
c.Assert(err, IsNil)
c.Check(features, DeepEquals, []string{"bar", "bar:feat1", "bar:feat2", "foo", "foo:baz", "foo:qux", "foo:qux:v3", "foo:qux:v5", "xyz"})

// Also test that prompt feature is read from permstable32 if it exists
c.Assert(os.Mkdir(filepath.Join(d, featuresSysPath, "policy"), 0755), IsNil)
for _, testCase := range []struct {
permstableContent string
expectedSuffixes []string
expectedFeatures []string
}{
{
"allow deny prompt fizz buzz",
[]string{"allow", "buzz", "deny", "fizz", "prompt"},
[]string{
"policy:permstable32",
"policy:permstable32:allow",
"policy:permstable32:buzz",
"policy:permstable32:deny",
"policy:permstable32:fizz",
"policy:permstable32:prompt",
},
},
{
"allow deny prompt fizz buzz ",
[]string{"allow", "buzz", "deny", "fizz", "prompt"},
[]string{
"policy:permstable32",
"policy:permstable32:allow",
"policy:permstable32:buzz",
"policy:permstable32:deny",
"policy:permstable32:fizz",
"policy:permstable32:prompt",
},
},
{
"allow deny\nprompt fizz \n buzz",
[]string{"allow", "buzz", "deny", "fizz", "prompt"},
[]string{
"policy:permstable32",
"policy:permstable32:allow",
"policy:permstable32:buzz",
"policy:permstable32:deny",
"policy:permstable32:fizz",
"policy:permstable32:prompt",
},
},
{
"allow deny\nprompt fizz \n buzz\n ",
[]string{"allow", "buzz", "deny", "fizz", "prompt"},
[]string{
"policy:permstable32",
"policy:permstable32:allow",
"policy:permstable32:buzz",
"policy:permstable32:deny",
"policy:permstable32:fizz",
"policy:permstable32:prompt",
},
},
{
"fizz",
[]string{"fizz"},
[]string{
"policy:permstable32",
"policy:permstable32:fizz",
},
},
{
"\n\n\nfizz\n\nbuzz",
[]string{"buzz", "fizz"},
[]string{
"policy:permstable32",
"policy:permstable32:buzz",
"policy:permstable32:fizz",
},
},
{
"fizz\tbuzz",
[]string{"buzz", "fizz"},
[]string{
"policy:permstable32",
"policy:permstable32:buzz",
"policy:permstable32:fizz",
},
},
{
"fizz buzz\r\n",
[]string{"buzz", "fizz"},
[]string{
"policy:permstable32",
"policy:permstable32:buzz",
"policy:permstable32:fizz",
},
},
} {
c.Assert(os.WriteFile(filepath.Join(d, featuresSysPath, "policy", "permstable32"), []byte(testCase.permstableContent), 0644), IsNil)
features, err = apparmor.ProbeKernelFeatures()
c.Assert(err, IsNil)
expected := []string{"bar", "foo", "foo:baz", "foo:qux", "policy"}
for _, suffix := range testCase.expectedSuffixes {
expected = append(expected, fmt.Sprintf("policy:permstable32:%s", suffix))
}
expected := []string{"bar", "bar:feat1", "bar:feat2", "foo", "foo:baz", "foo:qux", "foo:qux:v3", "foo:qux:v5", "policy"}
expected = append(expected, testCase.expectedFeatures...)
expected = append(expected, "xyz")
c.Check(features, DeepEquals, expected, Commentf("test case: %+v", testCase))
}
Expand All @@ -355,7 +413,7 @@ func (s *apparmorSuite) TestProbeAppArmorKernelFeatures(c *C) {
c.Assert(os.Mkdir(filepath.Join(d, featuresSysPath, "policy", "notify"), 0755), IsNil)
features, err = apparmor.ProbeKernelFeatures()
c.Assert(err, IsNil)
expected := []string{"bar", "foo", "foo:baz", "foo:qux", "policy", "policy:notify", "policy:permstable32:allow", "policy:permstable32:deny", "policy:permstable32:prompt", "xyz"}
expected := []string{"bar", "bar:feat1", "bar:feat2", "foo", "foo:baz", "foo:qux", "foo:qux:v3", "foo:qux:v5", "policy", "policy:notify", "policy:permstable32", "policy:permstable32:allow", "policy:permstable32:deny", "policy:permstable32:prompt", "xyz"}
c.Check(features, DeepEquals, expected)

// Also test that prompt feature is read from notify/user if it exists
Expand All @@ -379,11 +437,11 @@ func (s *apparmorSuite) TestProbeAppArmorKernelFeatures(c *C) {
c.Assert(os.WriteFile(filepath.Join(d, featuresSysPath, "policy", "notify", "user"), []byte(testCase.userContent), 0644), IsNil)
features, err = apparmor.ProbeKernelFeatures()
c.Assert(err, IsNil)
expected = []string{"bar", "foo", "foo:baz", "foo:qux", "policy", "policy:notify"}
expected = []string{"bar", "bar:feat1", "bar:feat2", "foo", "foo:baz", "foo:qux", "foo:qux:v3", "foo:qux:v5", "policy", "policy:notify", "policy:notify:user"}
for _, suffix := range testCase.expectedSuffixes {
expected = append(expected, fmt.Sprintf("policy:notify:user:%s", suffix))
}
expected = append(expected, "policy:permstable32:allow", "policy:permstable32:deny", "policy:permstable32:prompt", "xyz")
expected = append(expected, "policy:permstable32", "policy:permstable32:allow", "policy:permstable32:deny", "policy:permstable32:prompt", "xyz")
c.Check(features, DeepEquals, expected, Commentf("test case: %+v", testCase))
}
}
Expand Down
Loading