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 Touch seeded packages, which is subscribed to apparmor in Ubuntu. https://bugs.launchpad.net/bugs/2110236 Title: [SRU] fixes for AppArmor in Plucky Status in apparmor package in Ubuntu: Fix Released Status in apparmor source package in Plucky: Incomplete Bug description: [ Impact ] This SRU contains fixes for a number of bugs: * The unprivileged_userns profile did not have access to the root directory (LP: #2110616) * lsblk could not list DASD devices on IBM System Z (LP: #2107402) * Various commands segfaulted when run from a confined context due to missing permissions on the binary execution path (LP: #2107455, LP: #2110628) * The plasmashell profile was missing new path to QtWebEngineProcess, causing breakage of Web Browser widget (LP: #2107723) * fusermount3 lacked permissions to mount to /cvmfs (LP: #2110624) * openvpn lacked permissions to manage DNS settings for pushed DHCP settings (LP: #2107596) * openvpn lacked permissions to perform mDNS lookups (LP: #2109029) * remmina broke due to missing permissions (LP: #2107723) * fusermount3 lacked permissions to mount with flags needed for fuse_overlayfs and sshfs via fstab (LP: #2110626, LP: #2111807) * iotop-c failed to launch at all due to permission denials in nl_init (LP: #2107727) * The parser did not handle the norelatime mount flag correctly (LP: #2110688) * The apparmor.d man page contained incorrect information about the combination of mount options=(list) options in (list) (LP: #2110630) * This SRU also includes a regression test update (https://gitlab.com/apparmor/apparmor/-/merge_requests/1672) that is not part of the built package but that 1) ensures that the documented behavior lines up with the actual behavior, and 2) serves as a test for the parser patch [ Test Plan ] Bug-specific test plans can be found in their respective LP bug entries. Generic test plan (for all AppArmor changes, not specific to this SRU): AppArmor is also extensively tested via its QRT test suite, which includes execution of the AppArmor test suite. * To prepare the QRT test suite (can be done on any machine): - `git clone https://git.launchpad.net/qa-regression-testing` - `./scripts/make-test-tarball ./scripts/test-apparmor.py` * To run the QRT test suite: - Copy the tarball onto the machine with the new AppArmor installed and extract it - `sudo ./install-packages test-apparmor.py` - Reboot after dependency installation - `sudo ./test-apparmor.py -v` [ Where problems could occur ] All the profile changes in this SRU are loosening confinement on a profile. However, if a user manually modified the installed profiles, then the package upgrade would cause conflicts, and rejection of the incoming changes (either by hand during an interactive upgrade or automatically during an batch unattended upgrade) would result in end users not getting the complete set of packaged fixes. However, as each of the files updated are independent of each other, a partially fixed state will not be more broken than an unfixed (before upgrade) state. Remmina profile specific: If a user set up custom profiles that use "peer=remmina" IPC rules, then these rules would break upon the upgrade removing the remmina profile. However, none of the officially shipped profiles include such rules. The man page update is a documentation-only change. The risk exists that the new packaged man page could be malformed, but this is unlikely since the man page is generated by pod2man, and such issues can be caught during testing by attempting to open the man page after installation of the new version. The parser fix changes the behavior of mount rules that explicitly specify the norelatime flag. In particular, a custom profile containing `mount options in (norelatime)` will have different, more permissive behavior than before (reducing regression risk as compared to tightening behavior). However, this flag is not used in any of the commonly used profiles (including the ones in our repo and the profile fragments used by snapd), so this will not change the behavior of most profiles being used. [ Other Info ] The attached debdiff has a subset of the changes in the Questing AppArmor package (4.1.0~beta5-0ubuntu14..4.1.0~beta5-0ubuntu16), with the exception of the changelog. To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2110236/+subscriptions -- Mailing list: https://launchpad.net/~touch-packages Post to : touch-packages@lists.launchpad.net Unsubscribe : https://launchpad.net/~touch-packages More help : https://help.launchpad.net/ListHelp
