Fix fm-shim-backend D-Bus name registration and quoting#357
Fix fm-shim-backend D-Bus name registration and quoting#357assisted-by-ai wants to merge 1 commit intoKicksecure:masterfrom
Conversation
…otify - Quote pkg-config output in build script via bash arrays to prevent injection through malicious .pc files - Add NotifyAccess=main to systemd service to restrict sd_notify to the main process only - Add DBUS_NAME_FLAG_DO_NOT_QUEUE to fail fast when another process owns the D-Bus name, replacing the IN_QUEUE warning with an EXISTS error exit https://claude.ai/code/session_01HJPNe7hjMrPvPtuJmPBhNa
ArrayBolt3
left a comment
ArrayBolt3
left a comment
There was a problem hiding this comment.
Mostly rejected, rationale below.
|
|
||
| [Service] | ||
| Type=notify | ||
| NotifyAccess=main |
| read -r -a pkg_cflags_dbus <<< "$(pkg-config --cflags dbus-1)" | ||
| read -r -a pkg_cflags_systemd <<< "$(pkg-config --cflags libsystemd)" | ||
| read -r -a pkg_libs_dbus <<< "$(pkg-config --libs dbus-1)" | ||
| read -r -a pkg_libs_systemd <<< "$(pkg-config --libs libsystemd)" | ||
|
|
||
| gcc \ | ||
| -g \ | ||
| $(pkg-config --cflags dbus-1) \ | ||
| $(pkg-config --cflags libsystemd) \ | ||
| "${pkg_cflags_dbus[@]}" \ | ||
| "${pkg_cflags_systemd[@]}" \ | ||
| /usr/src/security-misc/fm-shim-backend.c \ | ||
| -o /usr/bin/fm-shim-backend \ | ||
| $(pkg-config --libs dbus-1) \ | ||
| $(pkg-config --libs libsystemd) \ | ||
| "${pkg_libs_dbus[@]}" \ | ||
| "${pkg_libs_systemd[@]}" \ |
There was a problem hiding this comment.
I don't see the point of this. This basically is just implementing word splitting ourselves, rather than using Bash's built-in word splitting. If we have to do word splitting either way, built-in seems better.
| dbus_name_request_rslt = dbus_bus_request_name(dbus_conn, | ||
| "org.freedesktop.FileManager1", | ||
| DBUS_NAME_FLAG_REPLACE_EXISTING, | ||
| DBUS_NAME_FLAG_REPLACE_EXISTING | DBUS_NAME_FLAG_DO_NOT_QUEUE, |
There was a problem hiding this comment.
But... we want to queue, so that even if we can't grab the name immediately, we take it over as soon as we can.
| case DBUS_REQUEST_NAME_REPLY_IN_QUEUE: | ||
| warnx("Requested 'org.freedesktop.FileManager1' name from D-Bus, but was placed in queue."); | ||
| warnx("This may be a security risk! Please report this bug!"); | ||
| /* | ||
| * sd_notify(0, "READY=1") is intentionally omitted here, so that | ||
| * systemcheck will show a warning about the system not being ready | ||
| */ | ||
| sd_notify(0, "STATUS=Running, security issue detected"); | ||
| case DBUS_REQUEST_NAME_REPLY_EXISTS: | ||
| errx(1, "Another process already owns 'org.freedesktop.FileManager1' name and did not allow replacement!"); |
There was a problem hiding this comment.
I think Claude misunderstood the security risk here. The security risk is that fm-shim-backend wasn't able to immediately grab the name, not that fm-shim-backend continues running even if it can't immediately grab the name. If fm-shim-backend exits immediately, it will never be able to grab the name, which is a worse security risk than placing itself in the queue and waiting until it can grab the name. The existing code will result in a systemcheck warning (because the service never tells systemd that it's fully initialized), which is good, and it also plugs potential security holes as soon as it possibly can, which is also good. This should stay as-is.
3 participants
Summary
This PR improves the robustness and security of the fm-shim-backend D-Bus service by fixing shell argument quoting issues in the build script and preventing the service from running in a degraded state when another process owns the D-Bus name.
Key Changes
pkg-configoutput in arrays and properly expanding them with"${array[@]}"syntax instead of unquoted command substitutionDBUS_NAME_FLAG_DO_NOT_QUEUEflag to prevent the service from being queued when the D-Bus name is unavailableNotifyAccess=mainto the service unit to properly handle systemd readiness notificationsImplementation Details
The changes ensure that:
https://claude.ai/code/session_01HJPNe7hjMrPvPtuJmPBhNa