finish-args-broken-kde-tray-permission is incorrect

[TingPing]

The finish-args-broken-kde-tray-permission forbids anything with org.kde.StatusNotifierItem at the beginning, however this isn't correct.

As you can see in knotification: https://github.com/KDE/knotifications/blob/7fb8c5b3130646845efb0483fc1cf3c7769c5830/src/kstatusnotifieritemdbus_p.cpp#L134

This is a unchanging format. In flatpak it will always be the same value for the same pid for the same item number.

Now this is very broken inside of flatpak but it is the correct permission to say --own-name=org.kde.StatusNotifierItem-2-1 because you know your values will always be the same. Fixing it means patching KDE libraries but that's a discussion for other people.

[TingPing]

Also recommending people use --own-name=org.kde.* is a terrible idea...

[Erick555]

What will happen when two apps own the same address?

[hfiguiere]

it's only fixed if you consider getpid() to always return the same value. What if the implementation changes?

[TingPing]

What will happen when two apps own the same address?

It probably just fails. This has to be fixed in the libraries.

it's only fixed if you consider getpid() to always return the same value. What if the implementation changes?

This is just a thought experiment. Flatpak is 8 years old and this has never changed. When it changes once in 25 years somebody can increment the 2 to a 3.

Giving ownership permission to org.kde.* however is actively harmful to security and can absolutely be exploited.

[hfiguiere]

This is just a thought experiment. Flatpak is 8 years old and this has never changed. When it changes once in 25 years somebody can increment the 2 to a 3.

I was more thinking of PID randomisation. It's not default on Linux but it remain a known technique for hardening system. It should even just happen without flatpak knowing.

[hfiguiere]

Also if we have to pin the behaviour on the specific version of flatpak, then it fails one of the idea of flatpak: to be system setup agnostic.

[Chocobo1]

https://github.com/flathub/flatpak-builder-lint/blob/350139c770a0c4c40decc01bbaa28e84e867a453/flatpak_builder_lint/checks/finish_args.py#L56-L57

The error says "broken-kde-tray-permission" but what exactly is broken?

[hfiguiere]

the design of the KDE tray SNI

[Erick555]

I was more thinking of PID randomisation. It's not default on Linux but it remain a known technique for hardening system. It should even just happen without flatpak knowing.

AFAIK PID randomization functionality doesn't exist at all in linux kernel and was dropped from security oriented forks ages ago:

Removal of randomized PIDs feature, since it provides no useful additional security and wastes memory with the 2.6 kernel's pid bitmap

[hfiguiere]

Fair enough.

But then:

1. will the app always be PID 2 ?
2. what about other apps with PID 2 ? The assumption that it's always PID 2 mean that many will compete for the same bus name.

[Erick555]

1. will the app always be PID 2 ?

I guess if the app changes its architecture to become a multiprocess like electron then the PID may change but it's still limited. There are apps who have PID 2 or 3, did anyone saw 4?

What if we give --own-name=org.kde.StatusNotifierItem-2-1 + --own-name=org.kde.StatusNotifierItem-3-1 for each app?

2. what about other apps with PID 2 ? The assumption that it's always PID 2 mean that many will compete for the same bus name.

Does --own-name=org.kde.* change anything here? If the app has PID 2 then it will still get PID 2 and conflict with others PID 2 apps even if you give it whole session bus, no?

[TingPing]

Does --own-name=org.kde.* change anything here? If the app has PID 2 then it will still get PID 2 and conflict with others PID 2 apps even if you give it whole session bus, no?

Correct. It is broken by design inside of flatpak. This requires work in the libraries used to solve anything.

[Erick555]

@barthalion would be ok to degrade this to warning? For apps which use right tray permission switching over to org.kde.* is an awkward change and sandbox weakening. I think the errors should be obviously wrong while this one isn't.

In the future you may setup bot a'la f-e-d-c that will open issue/PR for every warning to make people aware of the problem.

[hfiguiere]

Using org.kde.* will not make it work.

[Erick555]

What do you mean? With current linter rule apps can chose between org.kde.* or no tray functionality. For an app which had working tray before each choice is inferior.

[ilya-fedin]

As you can see in knotification: https://github.com/KDE/knotifications/blob/7fb8c5b3130646845efb0483fc1cf3c7769c5830/src/kstatusnotifieritemdbus_p.cpp#L134

The line you link to is an internal connection identifier used by Qt (i.e. ID to access the C++ global connection object from any place), it's not exposed to dbus in any way. What you wanted refer to is likely QDBusConnection::registerService API use of which was removed ages ago. It was also fixed in Qt itself, but only since 6.2. Although I believe the fix is present in KDE Patch Collection.

So this linter rule is indeed correct, own name shouldn't be necessary nowadays.

[TingPing]

What you wanted refer to is likely QDBusConnection::registerService API use of which was removed ages ago. It was also fixed in Qt itself, but only since 6.2. Although I believe the fix is present in KDE Patch Collection.

Excellent!

So this linter rule is indeed correct, own name shouldn't be necessary nowadays.

The linter is wrong, but in a different way then, it should recommend removing ownership permissions entirely, if the runtime is new enough to have Qt 6.2 (or the patches for this).

[ilya-fedin]

The linter is wrong, but in a different way then, it should recommend removing ownership permissions entirely, if the runtime is new enough to have Qt 6.2 (or the patches for this).

As far as I understand, all non-EOLed KDE runtimes have the fix

[TingPing]

I tested this on one application (qbittorrent (KDE 5.15-21.08)) and it worked. So I've opened a PR to error when people use org.kde.*.

[ilya-fedin]

Given all the references to this issue, my guess is it may still be a problem for Electron applications as Chromium has to be fixed as well https://github.com/chromium/chromium/blob/4493a1eb4595194a262617589c5a265de40e203e/chrome/browser/ui/views/status_icons/status_icon_linux_dbus.cc#L298-L301

[hfiguiere]

Some maybe this other issue could be closed too: https://github.com/flatpak/xdg-dbus-proxy/issues/15

[TingPing]

my guess is it may still be a problem for Electron applications as Chromium has to be fixed as well

Ugh, that is frustrating, its relatively new code too. Electron historically used libappindicator which already worked fine.

[TingPing]

I opened an issue here: https://bugs.chromium.org/p/chromium/issues/detail?id=1408315

[ilya-fedin]

I'd also mention in the issue that flatpak creates a PID namespace meaning only one Electron application at a time can have a tray icon what is a bigger problem than owning a name in general

[Erick555]

So for non-electron apps that use at least 5.15-21.08 runtime usage of --own=org.kde.* should be considered as bug?

[ilya-fedin]

So for non-electron apps that use at least 5.15-21.08 runtime usage of --own=org.kde.* should be considered as bug?

Any non-deprecated KDE runtime, so you can say just 'non-electron apps'

[TingPing]

Good news, this was just fixed in Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/4179380

[ilya-fedin]

How fast

[ilya-fedin]

I wonder whether Electron can cherry-pick this as a patch to Chromium in a minor update

[foresto]

I don't understand why flatpak-builder-lint is trying to police packaged applications, when its job is to police manifest files.

A patch I recently submitted for the im.riot.Riot manifest is now being rejected by flatpak-builder-lint, because of this overreach. The packaged application uses the dbus service name org.kde.StatusNotifierItem-3-1. It gets that name from Electron, which gets it from Chromium, which gets it from a Freedesktop recommendation. I have no control over Google's code. I have no way to fix it. It has been changed upstream, but it will take time for that change to trickle down to the real-world builds the people are using today.

My manifest patch is perfectly good, yet it's being red-flagged for breaking the build, which it does not.

It's fine to point out that the name is a poor choice, and that Freedesktop's recommendation is broken, but rejecting a manifest file that correctly accommodates what the application does is madness.

Won't you please downgrade this to a warning?

[ilya-fedin]

Freedesktop's recommendation

That page on freedesktop wiki is highly outdated I believe and it seems a bad thing to reference to it nowadays. The modern SNI spec has no formal page and evolves mainly with implementations. E.g. Qt, Plasma, KNotifications and Ubuntu's extension support ProvideXdgActivationToken for window activation on Wayland. Another notable difference is that everyone uses org.kde, not org.freedesktop.

[ilya-fedin]

I have no control over Google's code. I have no way to fix it. It has been changed upstream, but it will take time for that change to trickle down to the real-world builds the people are using today.

I believe you can ask Electron people to cherry-pick the fix as a patch https://github.com/electron/electron/tree/main/patches/chromium And then the fix will be in the next Electron's patch release.

[TingPing]

I opened an Electron bug requesting the Chromium patch is backported: https://github.com/electron/electron/issues/37053

[foresto]

To be clear, this policy is currently blocking the commit of a security fix.

[ilya-fedin]

It seems the plan is to block both org.kde.StatusNotifierItem- and org.kde. (#114), so apparently the right security fix is to remove the permission at all

[foresto]

apparently the right security fix is to remove the permission at all

I think that will break applications that minimize to the system tray. With no tray icon, users will have no way to reopen the application window. This is especially bad for applications that preserve their window state between runs.

[ilya-fedin]

They're usually single instance and will open the window after clicking on the shortcut in the applications list, aren't they?

[foresto]

I have used applications that did not open the first instance's window when a second instance was run. One such app was a pain to restore when a Gtk change broke its tray icon.

[vladimiry]

They're usually single instance and will open the window after clicking on the shortcut in the applications list, aren't they?

Detecting a second instance running attempt and showing the main app window instead is a behavior that needs to be explicitly implemented. Not all apps implement this.

[foresto]

It's also worth noting that there are apps whose only UI is a tray icon. (I don't know how many of these are currently on flathub.)

[TingPing]

To be clear, we're down to the point where only Electron is broken, maybe, and even when Electron has the permission its still very broken (two apps try to own the same name, first one wins, everybody else loses).

@foresto The immediate action you can make is to change your permission to --own-name=org.kde.*. I'm for downgrading it to a warning but that's up to others to decide and probably would happen next week.

@vladimiry That is an application bug. Of course many applications don't care though.

[foresto]

The immediate action you can make is to change your permission to --own-name=org.kde.*.

That exposes users to keyring impersonation and similar exploits, which is a security hole that I am not comfortable opening.

It seems the plan is to block both org.kde.StatusNotifierItem- and org.kde. (https://github.com/flathub/flatpak-builder-lint/pull/114), so apparently the right security fix

Forbidding org.kde.StatusNotifierItem-* does not fix that security hole.

Frankly, I'm still puzzled over who thinks it fixes anything at all, or why. AFAICT, all it does is move affected apps from "tray icons failing in certain circumstances" to "tray icons never working for anyone". There is no explanation for its presence in 0dc7f804, where it was introduced along with various other checks.

[ilya-fedin]

There is no explanation for its presence in 0dc7f80,

Apparently they knew Qt is fixed and Electron wasn't affected back then. So this wasn't a problem for anyone.

[foresto]

Apparently they knew Qt is fixed and Electron wasn't affected back then. So this wasn't a problem for anyone.

Perhaps, but even in that case, the rule served no purpose that I can see.

I imagine it might have made sense if the author thought --own-name=org.kde.StatusNotifierItem-2-1 caused tray icon failures. But it doesn't.

So why is the rule forbidding it still there? It looks to me like its addition was based on a misunderstanding.

[ilya-fedin]

So why is the rule forbidding it still there?

Apparently to force people get rid of legacy permission

[foresto]

But it's not legacy. Some applications still need it.

[ilya-fedin]

But it's not legacy. Some applications still need it.

Well, it was legacy at that point in time

[foresto]

Okay, then my understanding of the situation is this:

• The rule was most likely put in place based on assumptions that are not true today.
• The rule forbids a permission that some applications need today, thereby breaking them.
• The only workaround available to packagers exposes users to a dangerous security vulnerability.

Given that the rule does obvious harm for no discernible benefit, does it not make sense to remove it?

[ilya-fedin]

Maybe it's better to wait for answer from Electron devs? Maybe multiple days and it will become legacy again.

[foresto]

Every day of inaction keeps users exposed to the security vulnerability.

Surely protecting users is more important than a crusade to purge all manifest files of a harmless permission?