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

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

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.

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 ;-)

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.

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!

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.

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

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

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

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

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.

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.

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