[Question] Preload library enhancements for performance and security

SailfishOS VERSION N/A
HARDWAREN/A
PATCHMANAGER VERSION 3.0 and later

QUESTION

Executive summary: Are there any benefits to expanding the "path blacklist" in Patchmanager's preload library?

So the current mode of operation of Patchmanager is something like this:

A Patch is "activated"
For each file the patch manipulates, a copy of the original file is put into a cache dir in /tmp, and the changes are applied there instead of on the original file.
A patched application is launched.
Through library preloading, the libpreloadpatchmanager.so library is injected into the launching binary.
The library:
1. intercepts calls to open(), analyzes which files the call was meant to open
2. asks the patchmanager daemon (via socket) whether a patched version exists
3. if yes, redirects the open call to that file instead of the original
4. otherwise opens the original file

Now, this is done for all binaries launched, and it's done for all calls to open().

This of course has some impact on performance, (and raises security concerns, but lets put that aside for the moment).

What we also have is some safeguards built into the library, lists of paths ~~and file names~~ where it will not intercept calls:

list of paths prefixes
list of full paths (currently just "/")

I believe these can be expanded to improve both security and performance.

I would propose expanding the lists to include these categories of paths:

firmware and related paths in / (such as /odm /vendor /apex and some others), patches have no business messing with those. Most of them are mounted read-only, but I think it would be possible to make them "writable" by constructing a clever patch.
paths to files which are very commonly opened. This means things like:
- /etc/ld.so.preload
- /etc/ld.so.cache
- /etc/localtime
- /usr/lib/locale/locale-archive
- /usr/share/locale/locale.alias
- /usr/lib/libselinux.so.1
- /lib/libc.so.6
- /lib/libpthread.so.0
- /lib/libdl.so.2
- /lib/librt.so.1
- ... and possibly some more like wayland and Qt libraries.
while we're at it, try to fix some of the security considerations, like
- /etc/pki (certificates)
- /usr/lib/security (PAM)
- /etc/sudoers
- /etc/passwd
- /etc/passwd-
- /etc/group
- /etc/group-
- /etc/shadow
- /etc/shadow-
  1. Maybe some other lists created by analyzing some commonly running, system- or performance-critical binaries, or those that tend to open a lot of files, and adding their most-opened files to the list. E.g. udev, everything systemd, any of the other daemons.

Further considerations

Stability

libpreloadpatchmanager.so is currently a simple and time-tested piece of software, and is deployed in a manner very sensitive to bugs.
Changes should be done with great care, and certainly not because some nephros has a hunch there might be some microseconds gained in performance.

Performance

The lists are parsed like this: https://github.com/sailfishos-patches/patchmanager/blob/master/src/preload/src/preloadpatchmanager.c#L121

If the lists become very long, that could have a negative impact all of its own. It is hard to judge (for me) whether these would offset any gains from not doing a round trip to the PM daemon, and not intercepting any libc calls.
(Preliminary research on optimizing strcmp calls comes back with "don't do that. You're not smarter than libc developers.")

Gains

It is not proven at all that performance or security would be enhanced at all!

Hence the opening of this issue so these points can be discussed and analyzed.

Note that current functionality only allows for directory paths blacklisting. Blacklisting file paths would need new functionality most likely.

@nephros, nice and interesting that you researched and discovered this function; and also of Andrey to have implemented this long ago.

When reading your write-up for the first time, I was all "Yeah, cool, let's do that". After some consideration I believe there is only little to do and recommend to put most of the ideas aside.

Background

To evaluate the security aspects, a rehearsal of the security model of SailfishOS and its software ecosystem is necessary.

SailfishOS promises to protect a device owner and device users from device-external attacks to a certain extent. Examples for such attacks are a malicious WLAN router trying to compromise a device via network interfaces, prepared web-pages one visits ("drive by" attacks) leveraging one or multiple web-browser component(s) (HTML engine, CSS engine, JavaScript interpreter etc.), specially crafted e-mails attacking the e-mail handling and rendering etc. Unfortunately the extent is diminishing, because of the stale components Jolla employs: A Qt, which is almost 4 years out of security support but is used for rendering e-mails (e.g., its HTML engine: QtWebEngine), but even for the browser components (Gecko, NSS & Co.) which Jolla updated lately (with SFOS 4.4 in early 2022) the gap to the recent upstream components is expanding (plus the versions deployed by Jolla are also out of security support for long), not shrinking. For all these components the scheme "pick a few, highly scoring CVEs, some basic (in)security know-how and some inventor spirit to create an attack, which runs on all SailfishOS devices" is likely very successful; another reason not to use Jolla's Browser and Mail apps.
But it always has been in the full responsibility of the device owner which software (s)he installs and to check what this software does (which is infeasible / practically impossible). Even the Jolla Store with all its API and functionality restrictions (no permanently running services etc.) the security assurances are zero: There have been apps calling home on every app start (implicitly transmitting the IP address of the device) in the Jolla Store and no reason exists to expect that an app which systematically collects secrets and personal information to transmit it somewhere ("exfiltrate") besides some user facing functionality will be detected by Jolla with their Store check-in show. Plus for OpenRepos and SFOS:Chum anything goes, any way.

While the second bullet point is the price many users want pay ("with great power comes great responsibility") in order to not be restricted (by some "Apple ecosystem model"), the first point is one of Jolla's big failures (many years ago (> 7) all components used in SailfishOS and its apps had security support and Jolla was updating them, though never in a timely manner).

Consequences

Consequently any serious effort to prevent Patches to do something malicious is futile; Patches are uninteresting vectors, because the target audience is too small for individual Patches, their functionality is limited to patching via diffs etc. A Super-Duper-Filemananger which also supports (S)FTP(S)- and HTTP(S)-transfers (to explain and disguise network traffic) in the Jolla Store, OpenRepos and SFOS:Chum would be a far better approach.

What does make sense are safety considerations: A badly written Patch should be limited in wrecking havoc, as long these measures do not limit functionality or performance of Patchmanager. Ultimately I assume this was the reason for Andrey to implement this functionality. _{Side note: I often reason for handling any external input as probably malicious, because that catches people much better than "may contain the weirdest stuff one can think of" and ultimately the two aspects are not far apart; but honestly this and its practical application "quote in shell scripts whenever you cannot prove that the content is sanitised" originates from experiencing oh so many shell scripts to fail when they encounter paths / filenames with special, but allowed characters or simply spaces; and Windows users love to generate such paths.}

Assessments

So, "Yes, let us consider how these mechanisms can improve safety", but this shall not be detrimental to performance or functionality. More specifically:

It makes sense to only filter directories because most Unixes (Linux for sure) cache directory lookups more extensively than file lookups: There is a special cache only for dentrys in VFS layer, while generic inode caching is performed at filesystem level.
I was on my way to write, "and it sure does not make any sense to filter filesystems which are read-only mounted during regular operations", but then I hesitated, because I feel I need to think about that longer: Is it correct, that Patchmanager enables to virtually alter files on these quasi-immutable filesystems? If so, this is "cool functionality" on one side and may be safety-relevant on the other. Up to now people who had the idea to alter firmware content usually did not pursue this with a lot of effort, because there was no way to deploy any findings safely on other peoples devices. Hence I tend to "this is really cool functionality Patchmanager bears" and rather consider documenting than preventing it.
The same applies to "filter often called binaries": If a Patch slows down the OS but carries some useful function, it is nothing we should prevent structurally; it is up to the administrator of a device to decide if this is worth it.

Ultimately I think the current selection is fine and it does make sense investigate if there are any other locations with volatile, ever-changing data (in short: not pseudo-static data) or real safety concerns than the current entries /dev, /sys, /proc, /run, /tmp. Spontaneously I can think of /cache, /var/tmp, /var/cache, /var/spool. Does this work on real paths, i.e., ones which have ., .. and symlinks expanded (as realpath and readlink do)? If not, /var/run, /var/lock and /var/mail have to be added to my spontaneous list.

_{P.S. / side note: /tmp must be filtered, because otherwise one could create a Patch patching a file patched by Patchmanager, maybe even in a circular manner. It makes no sense any way, because by definition /tmp contains volatile data which is deleted every reboot.}

Is it correct, that Patchmanager enables to virtually alter files on these quasi-immutable filesystems?

One would have to verify, but I think there could be a way, yes. At least for files for which a diff file can be created, i.e. text and text-like files. This is hard (or maybe even not possible) for binary files.

Such a patch would create a copy in /tmp just as with any other file, and the preload magic would happily serve the changed content.

Very good points about the security angle and you are correct PM would be a weird attack vector to choose.

If a Patch slows down the OS but carries some useful function, it is nothing we should prevent structurally; it is up to the administrator of a device to decide if this is worth it.

You may be missing the point a bit - it's not any patches slowing down anything.

The very nature of the preload-library-hijacks-the-open-call setup means that all processes on the system have all open() calls passed through the library code, which checks for a couple of things including those blacklists, and makes a call via socket to patchmanager-daemon to ask about changed files.

This happens as soon as PM installed, there don't even need to be any patches installed or activated.

(As a side note: it's really a testament to the brilliance of the programmers of kernel, libc, and the PM library that this works without any perceivable downsides at all!)

I have added some stats output for testing this, and here's an example for a rather-freshly-booted patchmanager-daemon:

Apr 19 00:00:36 PGXperiiia10 patchmanager[3240]: Patchmanager Daemon runtime stats:
                                                   Daemon life-time: ............... 4072 seconds
                                                   Curently active patches: ........ 37
                                                   File accesses redirected: ....... 186
                                                   File accesses passed as-is: ..... 200867
                                                   Known changed files: ............ 61
                                                 ===========================
Apr 19 00:00:43 PGXperiiia10 patchmanager[3240]: void PatchManagerObject::doPatch(const QVariantMap&, const QDBusMessage&, bool) Sending reply.
Apr 19 00:00:44 PGXperiiia10 patchmanager[3240]: Patchmanager Daemon runtime stats:
                                                   Daemon life-time: ............... 4081 seconds
                                                   Curently active patches: ........ 38
                                                   File accesses redirected: ....... 186
                                                   File accesses passed as-is: ..... 202989
                                                   Known changed files: ............ 62
                                                 ===========================

I'll continue to monitor these stats, but it's clear that a really really significant majority of open calls are not hitting any patched files, not really surprising either.

This is where I think the potential for improvement lies. (Or rather, it irks me that the hit/miss ratio is so bad.)

To the extent that one might even consider switching to a whitelist, or in the extreme limiting the preloading to only those binaries like invoker and sailjail. That would of course recuce the potential functionality of PM to a great extent, but in reality wouldn't change much for existing patches (which tend to alter files in /usr/share and /usr/lib and not much more).

So the main goal of this question is actually whether we can and should try to minimize the effects of this by explicitly backing off these checks in the lib as early as possible.

Some experiments I'm doing can be looked at here:

https://github.com/nephros/patchmanager/tree/preload-blacklist

That tree:

adds some of the "common" locations (determined by a couple of straces) to the blacklists
introduces an "executable blacklist" where we back out of the checking stuff for things like the systemd processes and a couple of others
adds the aforementioned stats gathering in the daemon
...

Relevant changes in https://github.com/nephros/patchmanager/blob/preload-blacklist/src/preload/src/preloadpatchmanager.c

Of course these are all experiments, I still have to come up with a method to measure whether any of it makes any difference at all.

Does this work on real paths, i.e., ones which have ., .. and symlinks expanded (as realpath and readlink do)? If not, /var/run, /var/lock and /var/mail have to be added to my spontaneous list.

Yes, the file-to-be-checked is resolved to a real file by doing realpath here.

So entries in the blacklists need to be the actual paths, not symlinks. (Not sure about hardlinks - probably another can of worms ;) )

P.S. / side note: /tmp must be filtered, because otherwise one could create a Patch patching a file patched by Patchmanager, maybe even in a circular manner. It makes no sense any way, because by definition /tmp contains volatile data which is deleted every reboot.

Absolutely. The blacklisting of /tmp by the way may lead to the fun effect that you can't vim a patched file if vim is set up to put its temporary files in tmp. At least that is what I suspect happens when I try.

vim always displays empty files if they are patchmanager-patched...

Are you suggesting a whitelist written by the daemon that can be used as a first-stage filtering by the preload library?

Bloom filters could do that, I met a practical application for them :)

But.., there was this case of patchmanager preload library checking on tmp that also introduced enough delay to lose a race when sending MMS: https://forum.sailfishos.org/t/bug-mms-doesnt-send-pictures-when-patchmanager-is-installed/9925/13 Most probably the race was lost because the preload library was used, not because the /tmp check was slow..

If a Patch slows down the OS but carries some useful function, it is nothing we should prevent structurally; it is up to the administrator of a device to decide if this is worth it.

You may be missing the point a bit - it's not any patches slowing down anything.

It was just me becoming carried away. Yes, this is only about Patchmanager slowing down calls to executables (which it clearly does) plus if that is significant and can or should be alleviated.

The very nature of the preload-library-hijacks-the-open-call setup means that all processes on the system have all open() calls passed through the library code, which checks for a couple of things including those blacklists, and makes a call via socket to patchmanager-daemon to ask about changed files.

Yes, this was the cool invention of Jakibaki's Prepatch, which was integrated in Patchmanager, resulting in PM3.

I'll continue to monitor these stats, but it's clear that a really really significant majority of open calls are not hitting any patched files, not really surprising either.

Yes, that is absolutely expected and natural, hence …

This is where I think the potential for improvement lies. (Or rather, it irks me that the hit/miss ratio is so bad.)

… this should not really bother you, IMO.

It is an simple and easy design, which performs sufficiently well while still retaining all possible applications of Patches for PM.

To the extent that one might even consider switching to a whitelist, or in the extreme limiting the preloading to only those binaries like invoker and sailjail. That would of course reduce the potential functionality of PM to a great extent, but in reality wouldn't change much for existing patches (which tend to alter files in /usr/share and /usr/lib and not much more).

So the main goal of this question is actually whether we can and should try to minimize the effects of this by explicitly backing off these checks in the lib as early as possible.

IMO, "No", because it is a non-issue practically and I do not want to have PM's possible usage restricted by a "solution" for a non-issue.

Yes, the file-to-be-checked is resolved to a real file by doing realpath here.

So entries in the blacklists need to be the actual paths, not symlinks. (Not sure about hardlinks - probably another can of worms ;) )

Then there is not much to do from my POV, maybe except for adding these:

Spontaneously I can think of /cache, /var/tmp, /var/cache, /var/spool.

Hardlinks are easy (plus ancient and usually the wrong tool since the late 1980s), just another inode pointing to the same content (blocks on mass storage): SO they are always "real".

Is it correct, that Patchmanager enables to virtually alter files on these quasi-immutable filesystems?

One would have to verify, but I think there could be a way, yes. At least for files for which a diff file can be created, i.e. text and text-like files. This is hard (or maybe even not possible) for binary files.

Is that due to the strict use of the unified-diff format by PM? I am asking, because diff basically does handle comparisons between binary files and this enables incredibly cool use-cases. At least it should be possible to alter text strings in binaries, which is often sufficient.

Such a patch would create a copy in /tmp just as with any other file, and the preload magic would happily serve the changed content.

Yes, please let us not cripple this nicely generic function.

sailfishos-patches / patchmanager