I have consolidated all the review notes for the individual bugs in this one, matching my review log:
Reviewing apparmor SRU: I went through the bug list one by one and checked each bug against the changes in the package one by one. Meta bug: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110236 * General note: * The test plans should ensure the profiles are active; otherwise if there is a syntax error in them, we’d pass the tests because the processes are running unconfined\! Preferably by checking that something else that we expect to be denied is denied rather than it being loaded in the kernel. * Having more backlinks from patches to Ubuntu bugs would be preferable. Also consider adopting the patch tagging guidelines from [https://dep-team.pages.debian.net/deps/dep3/](https://dep-team.pages.debian.net/deps/dep3/) * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2102033](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2102033) * This was a bit hard to identify where the file got renamed; the profile got introduced in apparmor-4.1.0\~beta5/debian/patches/ubuntu/remmina\_mr\_1348.patch and there the filename is changed. * Impact and test plan are quite straightforward, if not entirely end oto end: We don’t actually test that something that got denied now works (we don’t test a user story), but rather that apparmor doesn’t interfer anymore (since it has no profile loaded) * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107402](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107402) * This adds a simple new patch lsblk-s390-fixes.patch that allows the path. * It is a bit confusing to review because it also includes the fix for [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107455](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107455) in the same patch (allowing /usr/bin/lsbk). * Luckily the patch has some metadata about the bugs it fixes * Regression risk is accordingly minimal since it only loosens a profile * I believe that “heck if apparmor is enabled and the profile active:” should be in the test plan, not the “Where problems could occur” * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107455](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107455) * This is essentially a subset of [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110628](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110628) focused only on lsblk. * The impact could give an example of a configuration this commonly happens in \[containers are listed in the other info\] * The test plan: * inadvertently does not actually test lsblk but rather random binaries from the other bug. * Should ensure the lsblk profile is actually loaded * The problem potential is straightforward since we only add the “/usr/bin/lsblk” mapping. * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107596](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107596) * Impact: It’s not clear how OpenVPN fails: Does it simply not apply the DHCP settings because the script fails or is the connection not established? * The test plan should * be extended to ensure that the pushed dhcp option is actually being applied * ensure that the relevant openvpn profile is active when testing * Where problems could occur should explain the `attach_disconnected` change and how it impacts the security * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107723](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107723) * The test plan should ensure that the relevant profile is active The patch is quite straight forward, adjusting to a new path * The where problems could occur part is wrong this time: This does not loosen confinement * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107727](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107727) * The impact here isn’t exactly matching the patch since we also fix nicing processes (which is tested in the test plan). * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2109029](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2109029) * Test plan seems reasonable, we only need to check that the correct remote was connected to, which this achieves. Otherwise refer to the notes on [2107596](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107596) * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110616](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110616) * The test plan should ensure that the relevant profile is active * The patch itself is quite straightforward. * Nitpick: I’d optimally like the problems section to go into further detail on how allowing access to the root directory could cause a problem, is being able to list / security relevant in some places that we don’t expect. * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110624](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110624) * The test plan should ensure that the relevant profile is active * Nitpick: This is rather straightforward but I feel like adding each possible FUSE to fusermount3 profile rather than having the FUSE fs’s extend the profile is not the best way to approach this issue as it leads to allowing all sorts of weird interactions. * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110626](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110626) * The test plan should ensure that the relevant profile is active * The patch does a bit of refactoring which makes things easier to read but requires double checking that both sections that got merged are correctly presented * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110628](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110628) * The test plan seems reasonable straight forward. * The test plan should ensure that the relevant profile that is being tested is active (i.e say `foo` profile is being applied to process `foo` being run inside aa-exec). * The test plan should ensure that all changed profiles can still be loaded, even the ones we did not test * The patch is straightforward just verbose :D * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110630](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110630) * Nothing to complain about really * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110688](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110688) * I’d like the test plan to link to the specific test in the test suite and include verification that that test was actually being run (i.e. the test suite could be old or failed to run the test perhaps) * [https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2111807](https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2111807) * The test plan should ensure that the relevant profile is active * I’m confused a bit where the noexec is coming from, but I suppose that sshfs adds it by default * Other changes lacking an individual bug: I went over the entire diff looking for stuff I have not reviewed above: * I noticed `regression-verify-documented-mount-flag-behavior.patch` here which is mentioned in the meta bug. ** Changed in: apparmor (Ubuntu Plucky) Status: Fix Committed => Incomplete -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/2110236 Title: [SRU] fixes for AppArmor in Plucky To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110236/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
