VPN-4798: Linux cgroup application matching heuristics #8801

oskirby · 2023-12-12T23:16:54Z

Description

App exclusions on Linux mostly make use of the GTK Launched signal over D-Bus to kick off application detection by connecting a *.desktop file and a control group, and this has worked okay-ish for the most part, but it leaves a lot of edge cases where applications can fall through the cracks if the PID that we get from the GTK launch event dies before the following control group is created. And it just so happens that this has become the case for Snap-packaged applications as of Ubuntu/23.04 Lunar.

To fix this, I think it's finally time that we get off our butts and do some proper heuristic matching between the control group name and typical launcher commands used for Linux desktop environments. To make this work, we first convert the *.desktop file into a desktop file ID and then try to parse a matching desktop file ID out of the cgroup path name.

So far this supports applications launched via the Gnome shell launcher, Snaps and Flatpaks. Perhaps with works with more desktop environments too? Only testing will tell!

Platforms tested thus far:

Ubuntu 20.04 native apps
Ubuntu 20.04 snaps
Ubuntu 23.04 native apps
Ubuntu 23.04 snaps
Ubuntu 23.04 flatpaks
Debian 12 native apps on Gnome shell
Debian 12 native apps on KDE Plasma
Fedora 38 native apps
Fedora 38 flatpaks

Reference

Github issue #6917 (VPN-4798)
Github issue #8016 (VPN-5535)
Freedesktop spec: https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#desktop-file-id

Checklist

My code follows the style guidelines for this project
I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
I have performed a self review of my own code
I have commented my code PARTICULARLY in hard to understand areas
I have added thorough tests where needed

vinocher · 2023-12-15T01:13:14Z

cracks if the PID that we get from the GTK launch event dies before the following control group is created

Trying to understand this better: Why does this always happen for snap-packaged apps? It sounds like this is not an edge case for snap apps, is that right?

vinocher · 2023-12-15T00:55:52Z

src/platforms/linux/linuxdependencies.cpp

+
+  // Find the application dir in the path.
+  const QString dirComponent("/applications/");
+  qsizetype index = path.lastIndexOf(dirComponent);


Should this search be CaseInsensitive?

IIRC, path names in Linux are case sensitive, so I think the answer is no.

vinocher · 2023-12-15T01:00:20Z

src/platforms/linux/daemon/apptracker.cpp

+// swapped from a dot to a dash. Which makes the parsing a bit of a pain.
+//
+// See: https://github.com/snapcore/snapd/blob/master/sandbox/cgroup/scanning.go
+QString AppTracker::snapDesktopId(const QString& scope) {


Should these string manipulation functions be moved to LinuxDependencies or some String utils file since they don't need any class state? Also, consider adding some tests for the string manipulation functions, now or later.

vinocher · 2023-12-15T01:04:02Z

src/platforms/linux/daemon/dbusservice.cpp

-  // HACK: Quick and dirty split tunnelling.
-  // TODO: Apply filtering to currently-running apps too.
-  if (m_excludedApps.contains(appId)) {
+  // Remove any firewall rules applied to this control group.


Shouldn't this app also be removed from m_excludedApps?

No, the gist of this, is that m_excludedApps holds the applications that the user wishes to exclude, so it kind of hangs around as a persistent setting, but m_excludedCgroups holds the matched applications that are currently running.

This signal occurs when the application closes, in which case the cgroup has been destroyed, but we still need to remember the setting in case the user re-opens it.

Is there any risk of desktopId re-use that can impact m_excludedApps, because the ID is not removed when the app is terminated? For example, could an excluded app be uninstalled and another app installed with the same desktopId while the VPN is on? Or is this an unlikely edge case that is difficult to handle?

I think this scenario would be commonly encountered if a user updates software on their device while the VPN is active, and I think the correct behavior in that case is to treat the newly-installed application identically to the previous version.

The recommendation in the freedesktop specification is for application names to follow a reverse-DNS naming convention, in which case they are expected to be a kind of globally unique identifier.

src/platforms/linux/daemon/dbusservice.cpp

vinocher · 2023-12-15T01:23:22Z

src/platforms/linux/linuxdependencies.cpp

@@ -156,3 +156,31 @@ QString LinuxDependencies::kdeFrameworkVersion() {

  return QString();
 }
+
+// static
+QString LinuxDependencies::desktopFileId(const QString& path) {


This method seems to return a non-empty string even if '/applications/' is not in the path. Should it try to detect error conditions and return an empty string in those cases, or is that not possible? Callers seem to pass on the result to other methods without additional checks.

It's not entirely clear how to handle that edge case, but I wanted to try and err on the side of caution and make sure that we always return something suitable for cgroup matching. I guess taking the file name and using that might be a more suitable fallback for that case.

vinocher · 2023-12-15T01:36:37Z

src/platforms/linux/daemon/apptracker.h

+    return m_runningApps.value(cgroup);
+  }
+  QStringList findByDesktopId(const QString& desktopId) const {
+    return m_runningApps.keys(desktopId);


Is the cgroup path <--> desktopId mapping 1:1? Or can there be multiple cgroup paths that map to a desktopId?

The desktopId -> cgroup mapping is one-to-many. Each cgroup matches at most one desktopId, but a desktopId can match multiple cgroups.

Thanks. It may be useful to add this information as a comment above the m_runningApps definition. I'm a bit confused because the cgroups man page says:
A cgroup is a collection of processes that are bound to a set of limits or parameters defined via the cgroup filesystem

src/platforms/linux/daemon/apptracker.cpp

src/platforms/linux/daemon/dbusservice.h

vinocher · 2023-12-15T01:45:26Z

src/platforms/linux/daemon/dbusservice.cpp

+    if (m_excludedCgroups.contains(cgroup)) {
+      m_wgutils->resetCgroup(cgroup);
+    }
+    m_excludedCgroups[cgroup] = state;
+    if (state == Excluded) {
+      // Excluded control groups are given special netfilter rules to direct
+      // their traffic outside of the VPN tunnel.
+      m_wgutils->excludeCgroup(cgroup);
+    }


Should we check if the cgroup state is already in the expected state before doing this work?

That felt like an unnecessary edge case to handle. If the cgroup is already in the expected state then this will simply remove the firewall rules for it, and then re-add them.

oskirby · 2023-12-15T19:03:24Z

Trying to understand this better: Why does this always happen for snap-packaged apps? It sounds like this is not an edge case for snap apps, is that right?

I think this breakage occurs on snap because applications are launched as a two-stage process. The first step is that snaps create a .desktop file that the desktop environment can see, which contains a command like /usr/bin/snap <target app>. The GTK Launched event does successfully report the creation of this process, and we can split-tunnel it just fine, but that command is just a wrapper that creates a sandbox environment for the target app (including a new cgroup) and launches the target app within it. The detection of that new cgroup has always been brittle because it depends on matching the PID from the GTK Launched event and there is no real guarantee that PIDs will stay running for any length of time after that signal, which opens us up to race conditions.

So, the real fix happening here is the addition of the AppTracker::snapDesktopId() to try and figure out the desktopId of a snap without any dependency on the lifetime of the contained process IDs, and then converting the AppTracker to match apps by desktopId rather than the full file path. The rest of this PR is just trying to clean up and modernize some of my old code.

And given that we are now using desktopIds instead of files, we can also do the same for Gnome and Flatpak apps, which also embed the desktopId in the cgroup path name - which makes the GTK Launched signal unnecessary.

src/platforms/linux/daemon/apptracker.h

vinocher · 2023-12-16T23:12:53Z

src/platforms/linux/daemon/apptracker.h

+    return m_runningApps.value(cgroup);
+  }
+  QStringList findByDesktopId(const QString& desktopId) const {
+    return m_runningApps.keys(desktopId);


Thanks. It may be useful to add this information as a comment above the m_runningApps definition. I'm a bit confused because the cgroups man page says:
A cgroup is a collection of processes that are bound to a set of limits or parameters defined via the cgroup filesystem

src/platforms/linux/daemon/apptracker.h

vinocher · 2023-12-16T23:49:27Z

src/platforms/linux/daemon/apptracker.cpp

+      result.replace(start, match.capturedLength(0), QString(code));
+      offset = start + 1;
+    } else {
+      offset = match.capturedEnd(0);


Just double checking that if the match is at the end of the original string, captureEnd will return an offset beyond the end of the string. Is that right?

That's not a problem, the loop will terminate in such a case because the loop condition requires the offset to be contained within the string.

src/platforms/linux/daemon/apptracker.cpp

vinocher · 2023-12-17T00:30:42Z

src/platforms/linux/daemon/apptracker.cpp

-      data->rootpid = m_lastLaunchPid;
-      break;
+// Make an attempt to resolve the desktop ID from a cgroup scope.
+QString AppTracker::findDesktopId(const QString& cgroup) {


Consider adding some examples of cgroup scope names, to make it easier for the reader to understand this method. Examples for various names that start with app-gnome-, app-flatpak-, gnome-launched- etc. would be helpful.

vinocher · 2023-12-17T00:39:16Z

I think this breakage occurs on snap because applications are launched as a two-stage process. The first step is that snaps create a .desktop file that the desktop environment can see, which contains a command like /usr/bin/snap <target app>. ...

Thanks for the explanation. It may be helpful to add some of this information as a class header comment in AppTracker.

vinocher

Looks good! Comments are very helpful!

oskirby added 14 commits December 12, 2023 10:31

Add method to convert desktop file path to desktop ID

1f1c1f2

Use desktop ID instead of file path for app matching

083c093

Parse desktop ID out of Gnome and Flatpak scope names

451533e

Fix unicode expansion regex

ccf5fe8

Remove PIDs from AppTracker public API

2ea5c64

Remove use of GTK launched signal, it has been unreliable

6bc2cfa

Remove AppData class, it doens't do anything useful anymore

84f3a2a

Fix Gnome launcher prefix on Ubuntu/focal

9bab097

Initial attempt to parse snap cgroup scopes

4faf397

Ignore unicode null chars

e57dcef

Linter fixes

4be15b6

Even more lint

bd0ee81

Better state tracking of control groups

c2bf05b

Lint for the lint gods!

5e5e106

oskirby requested a review from strseb December 13, 2023 20:34

oskirby marked this pull request as ready for review December 13, 2023 20:34

vinocher reviewed Dec 15, 2023

View reviewed changes

oskirby added 2 commits December 15, 2023 11:44

Remove unused methods in D-Bus API

98b5026

Use filename for fallback case when converting desktop file IDs.

951e8d1

vinocher reviewed Dec 17, 2023

View reviewed changes

oskirby added 3 commits December 18, 2023 10:19

Add more comments to explain the AppTracker class

8d10a4f

Linter fixes

4c772b1

More comment refining

3874360

oskirby requested a review from vinocher December 18, 2023 21:38

vinocher approved these changes Dec 18, 2023

View reviewed changes

oskirby enabled auto-merge (squash) December 18, 2023 22:15

github-actions bot added the 🛬 Landing This PR is marked as "auto-merge" label Dec 18, 2023

oskirby merged commit caf70fc into main Dec 18, 2023
125 of 127 checks passed

oskirby deleted the vpn-4798-cgroup-heuristic-matching branch December 18, 2023 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN-4798: Linux cgroup application matching heuristics #8801

VPN-4798: Linux cgroup application matching heuristics #8801

oskirby commented Dec 12, 2023 •

edited

Loading

vinocher commented Dec 15, 2023

vinocher Dec 15, 2023

oskirby Dec 15, 2023

vinocher Dec 15, 2023

vinocher Dec 15, 2023

oskirby Dec 15, 2023 •

edited

Loading

vinocher Dec 15, 2023 •

edited

Loading

oskirby Dec 18, 2023

vinocher Dec 15, 2023

oskirby Dec 15, 2023

vinocher Dec 15, 2023

oskirby Dec 15, 2023

vinocher Dec 16, 2023

vinocher Dec 15, 2023

oskirby Dec 15, 2023

oskirby commented Dec 15, 2023 •

edited

Loading

vinocher Dec 16, 2023

vinocher Dec 16, 2023

oskirby Dec 18, 2023

vinocher Dec 17, 2023

vinocher commented Dec 17, 2023

vinocher left a comment

VPN-4798: Linux cgroup application matching heuristics #8801

VPN-4798: Linux cgroup application matching heuristics #8801

Conversation

oskirby commented Dec 12, 2023 • edited Loading

Description

Reference

Checklist

vinocher commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskirby Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

vinocher Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskirby commented Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinocher commented Dec 17, 2023

vinocher left a comment

Choose a reason for hiding this comment

oskirby commented Dec 12, 2023 •

edited

Loading

oskirby Dec 15, 2023 •

edited

Loading

vinocher Dec 15, 2023 •

edited

Loading

oskirby commented Dec 15, 2023 •

edited

Loading