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: flatpak/xdg-dbus-proxy#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

[barthalion]

Thanks @bbhtt, I will go ahead and drop that error.