Bug 1226306

Thanks for the bug report.
We will schedule it in our team shortly.

This is rather small, the helper code consists of 300 lines of code. I will
look into it.

There's only one `writecolor()` D-Bus method that is accessible without
authentication for locally logged in users. The implementation is a bit
peculiar in some spots:

> KAuth::ActionReply KameleonHelper::writecolor(const QVariantMap &args)
> {
>     QStringList devices = args.value(QStringLiteral("devices")).toStringList();
>     QStringList colors = args.value(QStringLiteral("colors")).toStringList();
>     if (devices.length() != colors.length()) {
>         qCWarning(KAMELEONHELPER) << "lists of devices and colors do not match in length";
>         return KAuth::ActionReply::HelperErrorReply();
>     }
>     for (int i = 0; i < devices.length(); ++i) {
>         QString device = devices.at(i);

no need to use the more costly at(i) when it's already clear that the length
is correct, operator[] would be easier to read here.

Since this is the kauth D-Bus API I don't see why there's two separate lists
for devices and colors. It could be a list of pairs or a map, to avoid having
to sync up both lists with each other.

>         if (device.contains(QLatin1String(".."))) {
>             qCWarning(KAMELEONHELPER) << "invalid character sequence '..' for device" << device;
>             return KAuth::ActionReply::HelperErrorReply();
>         }

This bit is important to avoid escaping the /sys path below. But I would like
it better if any "/" characters would be rejected as well. The way it
currently is done the caller could still select an unexpected sub-directory
within the /sys/class/led tree, which has symlinks pointing around in the
sysfs hierarchy.

>         QByteArray color = colors.at(i).toUtf8();
>         if (!(color.length() >= QByteArray("0 0 0").length() && color.length() <= QByteArray("255 255 255").length())) {
>             qCWarning(KAMELEONHELPER) << "invalid RGB color" << color << "for device" << device;
>             return KAuth::ActionReply::HelperErrorReply();
>         }

This seems to be akind of minimal syntax check, but such checks should be left
to the kernel device driver, and not be reproduced in userspace. Also the
inverted if clause is rather hard to read.

>         QFile file(LED_SYSFS_PATH + device + LED_RGB_FILE);
>         if (!QFileInfo(file).exists()) {
>             qCWarning(KAMELEONHELPER) << "writing to" << file.fileName() << "failed:"
>                                       << "file does not exist";
>             return KAuth::ActionReply::HelperErrorReply();
>         }
>         if (!QFileInfo(file).isWritable()) {
>             qCWarning(KAMELEONHELPER) << "writing to" << file.fileName() << "failed:"
>                                       << "file is not writable";
>             return KAuth::ActionReply::HelperErrorReply();
>         }

Similarly here it should be left to the kernel to decide whether the file
exists and whether it is writable. There's no real value in doing these
(possibly racy) checks before actually opening the file.

>         if (!file.open(QIODevice::WriteOnly)) {
>             qCWarning(KAMELEONHELPER) << "writing to" << file.fileName() << "failed:" << file.error() << file.errorString();
>             return KAuth::ActionReply::HelperErrorReply();
>         }
>         const int bytesWritten = file.write(color);
>         if (bytesWritten == -1) {
>             qCWarning(KAMELEONHELPER) << "writing to" << file.fileName() << "failed:" << file.error() << file.errorString();
>             return KAuth::ActionReply::HelperErrorReply();
>         }
>         qCDebug(KAMELEONHELPER) << "wrote" << color << "to" << device;
>     }
> 
>     return KAuth::ActionReply::SuccessReply();
> }

None if this is critical for security, though, so I'll whitelisting the
helper.

This is an autogenerated message for OBS integration:
This bug (1226306) was mentioned in
https://build.opensuse.org/request/show/1186321 Factory / polkit-default-privs

The whitelisting is in place by now. Closing as fixed.

You might want to forward the nots in comment 3 to upstream, the helper code
could still use some love.

(In reply to Matthias Gerstner from comment #5)
> The whitelisting is in place by now. Closing as fixed.
> 
> You might want to forward the nots in comment 3 to upstream, the helper code
> could still use some love.

See the initial report, I already made an MR to fix those:

> I already had a look and sent a MR with some improvements:
> https://invent.kde.org/plasma/kdeplasma-addons/-/merge_requests/598