AppArmor python tools fail to parse mounts with UTF-8 non-ascii characters

[Zakhar]

[impact]

This bug prevents users with mount points that contain non-ascii UTF_8 characters in their names from easily seeing the status of their apparmor policy.

[steps to reproduce]

1) create mount point with non-ascii characters in it
2) run aa-status; if fixed it should not crash

[regression potential]

This issue is being addressed by updating the python utilities to the
version in apparmor 2.9.2 as tracked in bug 1449769. This represents are
large change which would normally be risky; however, these changes are
isolated to the python utils (so no changes to the policy parser/loader
or enforcement), there are a large number of bugs that exist in the
trusty version that make using the tools difficult, so it would be
difficult to regress further, and the updated version includes many new
unit tests to try to prevent from regressions from occurring.

[additional info]

The python utils testsuite is run as part of the test-apparmor.py test
script in lp:qa-regression-testing. The test-apparmor.py also has
additional basic usage tests to ensure that basic functionality is
maintained. These tests are run as part of the process fro each kernel
update.

[original description]

Version:
14.04 Fresh Install.

$ uname -a
Linux alain-Desktop 3.13.0-24-generic #46-Ubuntu SMP Thu Apr 10 19:11:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

AppArmor crashes with this stack trace:

PythonArgs: ['/usr/sbin/aa-status']
Traceback:
 Traceback (most recent call last):
   File "/usr/sbin/aa-status", line 194, in <module>
     commands[cmd]()
   File "/usr/sbin/aa-status", line 17, in cmd_enabled
     if get_profiles() == {}:
   File "/usr/sbin/aa-status", line 82, in get_profiles
     apparmorfs = find_apparmorfs()
   File "/usr/sbin/aa-status", line 137, in find_apparmorfs
     for p in open("/proc/mounts").readlines():
   File "/usr/lib/python3.4/encodings/ascii.py", line 26, in decode
     return codecs.ascii_decode(input, self.errors)[0]
 UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 1200: ordinal not in range(128)

That is because I have a UTF-8 non-Ascii character in one of my mounts:
$ mount
/dev/sdb1 on / type ext4 (rw,errors=remount-ro)
proc on /proc type proc (rw,noexec,nosuid,nodev)
sysfs on /sys type sysfs (rw,noexec,nosuid,nodev)

(...)
/dev/sda6 on /home/alain/Vidéos type ext4 (rw,relatime,errors=remount-ro)
(...)

The Python code of aastatus near line 135 shows:

def find_apparmorfs():
    '''Finds AppArmor mount point'''
    for p in open("/proc/mounts").readlines():
        if p.split()[2] == "securityfs" and \
           os.path.exists(os.path.join(p.split()[1], "apparmor")):
            return os.path.join(p.split()[1], "apparmor")
    return False

So if I do:
$ cat /proc/mounts
rootfs / rootfs rw 0 0
sysfs /sys sysfs rw,nosuid,nodev,noexec,relatime 0 0

(...)
/dev/sda6 /home/alain/ ext4 rw,relatime,errors=remount-ro,data=ordered 0 0
(...)

There is indeed a line with a 'é'

UTF-8 code of the 'é' is 0xC3 0xA9

But apparently the call to .readlines() is set as ASCII and hence does NOT support the 0xC3 (first byte of the 'é' in UTF-8) and then it crashes.

I assume the solution could be to correctly read as UTF-8 as there are some non-Americans/English out there!

Sorry my knowledge of Python is just enough to understand the bug, and not enough to submit a patch.

Regards.
Alain

[Seth Arnold (seth-arnold)]

I'm afraid this will take more significant modifications than simply reading in utf-8 -- users in Russia or China or Japan may have mountpoints named using KOI-8, Big-5, Shift-JIS, etc., that can't be parsed as utf-8.

I suspect we'll need to read in the mounts as an array of bytes, split based on newlines, and search for the bytes that make up "securityfs" and "apparmor".

Thanks

[Zakhar (alainb06)]

You are very right Seth.

There could also be a "cleaner" method to get mounts via some system calls instead of relying on a reading (pseudo) file. I say they "could" because I never needed that in any of my programs, so didn't investigate the topic.

If the only/easier way is to read a file (/proc/mount), there could be a quick fix for 99% of UTF-8/ASCII users to change the reading mode to UTF-8 (which encompasses ASCII), then a cleaner and probably much time consuming solution to do it properly considering the locale for the non UTF-8/ASCII users.

Wether you chose to do the quick (not totally clean) fix or not is your call.

Regars.
Alain

P.S.: by the way, I'm almost sure that if you have an English/American standard Trusty install you DO use UTF-8 in your locale. So the bug exists. The only thing is that you do not trigger it because you probably do not have mounts with characters out of the ASCII 32-127 range. So in this case, it works "by chance", because reading ASCII and UTF-8 is the same for codepoints in 32-127!
So you should really consider the quick fix: having UTF-8 as default, then work on a more stable fix.

[Christian Boltz (cboltz)]

The "funny" thing is that this bug depends on your locale.

I just did a mount on an non-ASCII mountpoint and can reproduce the bug with
LANG=C aa-status

However, everything works fine if I use
LANG=de_DE aa-status

That's python charset "fun" at its best...

hmmm...
I'm not sure if "Workaround: learn german" is a good answer ;-)

[Zakhar (alainb06)]

After some months where it didn't seem to crash -or at least apport didn't report anything-, now this is crashing again with the same stack trace about reading my UTF-8/non-ascii mounts.

I'm sending new apports automatic crash reports.

Although, Christian, I would find it normal that it crashes with
LANG=C aa-status

... because that defaults to english/ASCII, and then you should NOT have non-ascii mounts.

If I do
aa-status (implied with my locale fr_FR.UTF-8) it works fine and says:
apparmor module is loaded.
22 profiles are loaded.
22 profiles are in enforce mode.
   /sbin/dhclient
   /usr/bin/evince
   /usr/bin/evince-previewer
   /usr/bin/evince-previewer//sanitized_helper
   /usr/bin/evince-thumbnailer
   /usr/bin/evince-thumbnailer//sanitized_helper
   /usr/bin/evince//sanitized_helper
   /usr/bin/vidalia
   /usr/lib/NetworkManager/nm-dhcp-client.action
   /usr/lib/connman/scripts/dhclient-script
   /usr/lib/cups/backend/cups-pdf
   /usr/lib/lightdm/lightdm-guest-session
   /usr/lib/lightdm/lightdm-guest-session//chromium
   /usr/lib/telepathy/mission-control-5
   /usr/lib/telepathy/telepathy-*
   /usr/lib/telepathy/telepathy-*//pxgsettings
   /usr/lib/telepathy/telepathy-*//sanitized_helper
   /usr/lib/telepathy/telepathy-ofono
   /usr/sbin/cups-browsed
   /usr/sbin/cupsd
   /usr/sbin/tcpdump
   system_tor
0 profiles are in complain mode.
4 processes have profiles defined.
4 processes are in enforce mode.
   /sbin/dhclient (1375)
   /usr/lib/telepathy/mission-control-5 (2551)
   /usr/sbin/cups-browsed (1371)
   /usr/sbin/cupsd (2606)
0 processes are in complain mode.
0 processes are unconfined but have a profile defined.

Thus, although there is a crash reported at startup, it seems that I still have app-armor up an running.

The bug might then be at startup. Could be that when aa is run at startup the language is not yet defined, thus it is running with default LANG=C and failing (which is then kind of "normal situation" as the mounts can use any encoding they see fit!).

I'll try to investigate a little bit more around that idea.

[Zakhar (alainb06)]

Ok, so *I fixed it!*

Sorry guys, the 'patch' process is a bit complicated here, so I'll just give a diff to what I have done, and explain why:

$ diff aa-status aa-status_orig
137,140c137,140
< for p in open("/proc/mounts","rb").readlines():
< if p.split()[2].decode() == "securityfs" and \
< os.path.exists(os.path.join(p.split()[1].decode(), "apparmor")):
< return os.path.join(p.split()[1].decode(), "apparmor")
---
> for p in open("/proc/mounts").readlines():
> if p.split()[2] == "securityfs" and \
> os.path.exists(os.path.join(p.split()[1], "apparmor")):
> return os.path.join(p.split()[1], "apparmor")

**And I, Alain BENEDETTI, hereby grant Canonical all rights to use this patch as it sees fit!**

Explanation:
- Using readlines() on proc/mounts opened as regular stream is wrong! The kernel considers the mount points as bunch of binary data and does NOT assume any locale. Here, I have exhibited the bug because I'm mounting on a UTF-8 non-ASCII mount point.

- So to fix the bug, we do the same thing as the kernel:
==> we read the /proc/mounts as BINARY data, hence the first modification opening it "rb"

- Then we are searching for the string 'securityfs" on the second split. But as p is now of binary type, we must .decode() this split()[2] that contains the filetype. Here it is SAFE to .decode() as we know it is a filetype, therefore we ALWAYS have ASCII here and the .decode() will always succeed.

- When this first condition passes, we do the same on the split()[1], we apply .decode(), because join wants 2 strings, and not a binary + a string.
It is also safe here, because securityfs has been mounted on an ASCII mount point. Considering that this piece of code won't be run when the first part of the condition fails. We are in an "and" condition, and pyhton (as many languages) don't evaluate the remaining conditions when the first condition of an "and" is false.

So, it is closed for me with my own code modifications.

... expecting now the "official" patch of aa-status on the repositories, whether you use my exact patch, or make it even better!

[Christian Boltz (cboltz)]

Thanks for the patch!

I just forwarded it to the mailinglist for the usual formal review. I also tested it successfully, after setting up a non-ascii mountpoint ;-)

I'll update the bugreport when commiting the patch to bzr.

[Zakhar (alainb06)]

Thanks Christian for the heavy lifting on the mailinglist and bzr.

I didn't understand, on the FAQ, about yielding permissions to Canonical. Is that about proposing patches on Launchpad itself, or does it also apply for projects hosted on launchpad.
Anyway, to whomever, Canonical or "upstream team" for aa-status, I grant all rights for that patch... and as a courtesy, as far as possible, I'll appreciate to be listed under the contributors *Alain BENEDETTI* (if there is such list of contributors for this project).

I'm also very sorry, I see that the launchpad munched the Python indentation in my diff quote. If you need a cleaner diff, I'll be glad to attach it, but I'm assuming you corrected the wrong indentations from the post above!

For the record I did also try to correct applying a locale. That is impractical because when aa-status is run first, from the apparmor script in the init.d, the environment has no locale. When you run it later, once connected to a session, that works because it then gets the locale of the session, which is generally UTF-8.
After further reading, I saw that the kernel considered mountpoints as binary strings, thus I did accordingly.

I also tested "security tricks", like naming a directory: "pwned securityfs" with a "space" (ASCII 0x20) in the name, which is totally legal, and same for "\n" which is also legal in paths!
Fortunately, that does break aa-status, because in such cases the kernel "escapes" the names, and you would get:
"pwned\040securityfs"... so all seems safe for security, even if you try fancy mountpoints!

(otherwise I would have opened another undisclosed ticket)

Regards.
Alain

[Zakhar (alainb06)]

[EDIT] Obviously I meant fancy mountpoints having spaces or \n, do *NOT* break aa-status!

[Steve Beattie (sbeattie)]

Alain, thanks for the patch. I have committed it to upstream apparmor.

[Launchpad Janitor (janitor)]

This bug was fixed in the package apparmor - 2.9.2-0ubuntu1

---------------
apparmor (2.9.2-0ubuntu1) wily; urgency=medium

  * Update to apparmor 2.9.2
    - Fix minitools to work with multiple profiles at once (LP: #1378095)
    - Parse mounts that have non-ascii UTF-8 chars (LP: #1310598)
    - Update dovecot profiles (LP: #1296667)
    - Allow ubuntu-helpers to build texlive fonts (LP: #1010909)
  * dropped patches incorporated upstream:
    add-mir-abstraction-lp1422521.patch, systemd-dev-log-lp1413232.patch
    parser-fix_modifier_compilation_+_tests.patch,
    tests-fix_systemd_breakage_in_pivot_root-lp1436109.patch,
    GDM_X_authority-lp1432126.patch, and
    debian/patches/easyprof-framework-policy.patch
  * Partial merge with debian apparmor package:
    - debian/rules: enable the bindnow hardening flag during build.
    - debian/upstream/signing-key.asc: add new upstream public
      signing key
    - debian/watch: fix watch file, add gpg signature checking
    - install libapparmor.so dev symlink under /usr not /lib
    - debian/patches/reproducible-pdf.patch: make techdoc.pdf
      reproducible even in face of timezone variations.
    - debian/control: sync fields
    - debian/debhelper/postrm-apparmor: remove
      /etc/apparmor.d/{disable,} on package purge
    - debian/libapache2-mod-apparmor.postrm: on package purge, delete
      /etc/apparmor.d/{,disable} if empty
    - debian/libapparmor1.symbols: Use Build-Depends-Package in the
      symbols file.
    - debian/copyright: sync

 -- Steve Beattie <email address hidden> Mon, 11 May 2015 22:03:04 -0700

[Steve Beattie (sbeattie)]

I have reproduced the traceback with aa-status from apparmor 2.8.95~2430-0ubuntu5.1 in trusty-updates, and have verified that the version in of apparmor in trusty-proposed, 2.8.95~2430-0ubuntu5.2, fixes the issue.

[Adam Conrad (adconrad)]

The verification of the Stable Release Update for apparmor has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Christian Boltz (cboltz)]

This is fixed in the (already released) AppArmor 2.10, but I'm unable to set the target milestone to 2.10.