From 51d33c1a23ac8afe3af2caa9b171d9a9ebf88b49 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sun, 18 Jun 2023 10:19:29 -0700 Subject: [PATCH] parser: fix rule flag generation change_mount type rules MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1048 made it so rules like mount slave /snap/bin/** -> /**, mount /snap/bin/** -> /**, would get passed into change_mount_type rule generation when they shouldn't have been. This would result in two different errors. 1. If kernel mount flags were present on the rule. The error would be caught causing an error to be returned, causing profile compilation to fail. 2. If the rule did not contain explicit flags then rule would generate change_mount_type permissions based on souly the mount point. And the implied set of flags. However this is incorrect as it should not generate change_mount permissions for this type of rule. Not only does it ignore the source/device type condition but it generates permissions that were never intended. When used in combination with a deny prefix this overly broad rule can result in almost all mount rules being denied, as the denial takes priority over the allow mount rules. Fixes: https://bugs.launchpad.net/apparmor/+bug/2023814 Fixes: https://bugzilla.opensuse.org/show_bug.cgi?id=1211989 Fixes: 9d3f8c6cc ("parser: fix parsing of source as mount point for propagation type flags") Fixes: MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1048 Signed-off-by: John Johansen (cherry picked from commit 86d193e183bf71448e2394734b0f2d8c316bb262) Signed-off-by: John Johansen --- parser/mount.cc | 4 ++-- parser/tst/equality.sh | 11 +++++++++++ parser/tst/simple_tests/mount/bad_opt_32.sd | 6 ++++++ parser/tst/simple_tests/mount/bad_opt_35.sd | 6 ++++++ parser/tst/simple_tests/mount/bad_opt_36.sd | 6 ++++++ parser/tst/simple_tests/mount/bad_opt_37.sd | 6 ++++++ parser/tst/simple_tests/mount/bad_opt_38.sd | 6 ++++++ parser/tst/simple_tests/mount/bad_opt_39.sd | 6 ++++++ parser/tst/simple_tests/mount/bad_opt_40.sd | 6 ++++++ parser/tst/simple_tests/mount/bad_opt_41.sd | 6 ++++++ parser/tst/simple_tests/mount/ok_opt_84.sd | 8 ++++++++ tests/regression/apparmor/mount.sh | 10 ++++++++++ utils/test/test-parser-simple-tests.py | 8 ++++++++ 13 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 parser/tst/simple_tests/mount/bad_opt_32.sd create mode 100644 parser/tst/simple_tests/mount/bad_opt_35.sd create mode 100644 parser/tst/simple_tests/mount/bad_opt_36.sd create mode 100644 parser/tst/simple_tests/mount/bad_opt_37.sd create mode 100644 parser/tst/simple_tests/mount/bad_opt_38.sd create mode 100644 parser/tst/simple_tests/mount/bad_opt_39.sd create mode 100644 parser/tst/simple_tests/mount/bad_opt_40.sd create mode 100644 parser/tst/simple_tests/mount/bad_opt_41.sd create mode 100644 parser/tst/simple_tests/mount/ok_opt_84.sd diff --git a/parser/mount.cc b/parser/mount.cc index b8990006f..2b674a40a 100644 --- a/parser/mount.cc +++ b/parser/mount.cc @@ -996,7 +996,7 @@ int mnt_rule::gen_flag_rules(Profile &prof, int &count, unsigned int flags, if (!dev_type && !opts && gen_policy_bind_mount(prof, count, flags, opt_flags) == RULE_ERROR) return RULE_ERROR; - if (!dev_type && !opts && + if ((!device || !mnt_point) && !dev_type && !opts && gen_policy_change_mount_type(prof, count, flags, opt_flags) == RULE_ERROR) return RULE_ERROR; if (!dev_type && !opts && @@ -1012,7 +1012,7 @@ int mnt_rule::gen_flag_rules(Profile &prof, int &count, unsigned int flags, return gen_policy_bind_mount(prof, count, flags, opt_flags); } else if ((allow & AA_MAY_MOUNT) && (flags & (MS_MAKE_CMDS)) - && !dev_type && !opts) { + && (!device || !mnt_point) && !dev_type && !opts) { return gen_policy_change_mount_type(prof, count, flags, opt_flags); } else if ((allow & AA_MAY_MOUNT) && (flags & MS_MOVE) && !dev_type && !opts) { diff --git a/parser/tst/equality.sh b/parser/tst/equality.sh index ae3f009a8..c85799ef0 100755 --- a/parser/tst/equality.sh +++ b/parser/tst/equality.sh @@ -563,6 +563,17 @@ verify_binary_equality "link rules slash filtering" \ @{BAR}=/mnt/ /t { link @{FOO}/foo -> @{BAR}/bar, }" \ + +# This can potentially fail as ideally it requires a better dfa comparison +# routine as it can generates hormomorphic dfas. The enumeration of the +# dfas dumped will be different, even if the binary is the same +# Note: this test in the future will require -O filter-deny and +# -O minimize and -O remove-unreachable. +verify_binary_equality "mount specific deny doesn't affect non-overlapping" \ + "/t { mount options=bind /e/ -> /**, }" \ + "/t { audit deny mount /s/** -> /**, + mount options=bind /e/ -> /**, }" + if [ $fails -ne 0 -o $errors -ne 0 ] then printf "ERRORS: %d\nFAILS: %d\n" $errors $fails 2>&1 diff --git a/parser/tst/simple_tests/mount/bad_opt_32.sd b/parser/tst/simple_tests/mount/bad_opt_32.sd new file mode 100644 index 000000000..a02915b43 --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_32.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(slave) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/bad_opt_35.sd b/parser/tst/simple_tests/mount/bad_opt_35.sd new file mode 100644 index 000000000..02d4114aa --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_35.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(rslave) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/bad_opt_36.sd b/parser/tst/simple_tests/mount/bad_opt_36.sd new file mode 100644 index 000000000..17cc47766 --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_36.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(unbindable) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/bad_opt_37.sd b/parser/tst/simple_tests/mount/bad_opt_37.sd new file mode 100644 index 000000000..a3b194b03 --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_37.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(runbindable) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/bad_opt_38.sd b/parser/tst/simple_tests/mount/bad_opt_38.sd new file mode 100644 index 000000000..6f1c924ca --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_38.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(private) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/bad_opt_39.sd b/parser/tst/simple_tests/mount/bad_opt_39.sd new file mode 100644 index 000000000..d013554f7 --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_39.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(rprivate) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/bad_opt_40.sd b/parser/tst/simple_tests/mount/bad_opt_40.sd new file mode 100644 index 000000000..3ce9fad69 --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_40.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(shared) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/bad_opt_41.sd b/parser/tst/simple_tests/mount/bad_opt_41.sd new file mode 100644 index 000000000..51a336ba2 --- /dev/null +++ b/parser/tst/simple_tests/mount/bad_opt_41.sd @@ -0,0 +1,6 @@ +# +#=Description test we fail make rules with source and mntpnt associated with MR 1054 +#=EXRESULT FAIL +/usr/bin/foo { + mount options=(rshared) /snap/bin/** -> /**, +} diff --git a/parser/tst/simple_tests/mount/ok_opt_84.sd b/parser/tst/simple_tests/mount/ok_opt_84.sd new file mode 100644 index 000000000..9dfcc5241 --- /dev/null +++ b/parser/tst/simple_tests/mount/ok_opt_84.sd @@ -0,0 +1,8 @@ +# +#=Description test we can parse rules associated with MR 1054 +#=EXRESULT PASS +/usr/bin/foo { + mount options=(slave) /**, + mount options=(slave) -> /**, + mount /snap/bin/** -> /**, +} diff --git a/tests/regression/apparmor/mount.sh b/tests/regression/apparmor/mount.sh index 443a5a9ce..022fc70e7 100755 --- a/tests/regression/apparmor/mount.sh +++ b/tests/regression/apparmor/mount.sh @@ -398,6 +398,16 @@ else runchecktest "UMOUNT (confined cap umount:ALL)" pass umount ${loop_device} ${mount_point} remove_mnt + # MR:https://gitlab.com/apparmor/apparmor/-/merge_requests/1054 + # https://bugs.launchpad.net/apparmor/+bug/2023814 + # https://bugzilla.opensuse.org/show_bug.cgi?id=1211989 + # based on rules from profile in bug that triggered issue + genprofile cap:sys_admin "qual=deny:mount:/snap/bin/:-> /**" \ + "mount:options=(rw,bind):-> ${mount_point}/" + + runchecktest "MOUNT (confined cap bind mount with deny mount that doesn't overlap)" pass mount ${mount_point2} ${mount_point} -o bind + remove_mnt + test_options fi diff --git a/utils/test/test-parser-simple-tests.py b/utils/test/test-parser-simple-tests.py index 698691586..8d852c163 100644 --- a/utils/test/test-parser-simple-tests.py +++ b/utils/test/test-parser-simple-tests.py @@ -107,6 +107,14 @@ exception_not_raised = [ 'mount/bad_opt_29.sd', 'mount/bad_opt_30.sd', 'mount/bad_opt_31.sd', + 'mount/bad_opt_32.sd', + 'mount/bad_opt_35.sd', + 'mount/bad_opt_36.sd', + 'mount/bad_opt_37.sd', + 'mount/bad_opt_38.sd', + 'mount/bad_opt_39.sd', + 'mount/bad_opt_40.sd', + 'mount/bad_opt_41.sd', 'profile/flags/flags_bad10.sd', 'profile/flags/flags_bad11.sd', 'profile/flags/flags_bad12.sd',