core: fix security and concurrency issues found in a backend audit#2805
Open
asteroidzman wants to merge 1 commit into
Open
core: fix security and concurrency issues found in a backend audit#2805asteroidzman wants to merge 1 commit into
asteroidzman wants to merge 1 commit into
Conversation
Security: - privesc: pipe the sudo password via stdin (sudo -S) instead of embedding it in the command string, so it no longer appears in argv (readable by any local user via /proc/<pid>/cmdline or ps) - greeter: tokenize a session .desktop Exec= line into argv and execve directly instead of running it through /bin/sh -c, closing a command- injection path via user-writable ~/.local/share/wayland-sessions - plugins: reject path-separator/.. in plugin id/name before joining into a filesystem path, closing an arbitrary-directory-delete in the uninstall/update fallback - keybinds/hyprland: always quote unrecognized bind actions/keys when writing generated Lua; only re-emit genuine round-tripped custom Lua verbatim (tracked via an explicit flag), closing a Lua-injection path - desktop/mimeapps: reject newline/bracket in mime/desktop-id fields so they can't inject fake sections into the shared mimeapps.list Robustness / concurrency: - server: recover panics in the request-dispatch path so one bad handler can't crash the daemon and drop every client - go-wayland: recover panics in the shared dispatch choke point so a malformed compositor event can't crash CLI tools / the daemon - server: per-connection D-Bus client ID instead of a shared constant, fixing cross-client signal delivery and subscription teardown - network: guard the NetworkManager device maps with a mutex (a concurrent map read/write here is an unrecoverable fatal error) - cups: close the event channel on Stop() so Unsubscribe() of the last subscriber no longer deadlocks; allocate the fresh channel in Start() - freedesktop: reuse the shared session conn for the settings watcher and tear it down in Close(), fixing a per-Manager conn+goroutine leak - clipboard: mutex-guard lazy dbusConn creation - geolocation: use WithMatchMember for the GeoClue2 LocationUpdated signal (was WithMatchSender with an interface.member string, so the match never fired and live location updates never arrived) - screenshot: set failed=true on buffer/pool creation errors so the dispatch loop doesn't wait forever for a ready/failed that never comes
bbedward
requested changes
Jul 10, 2026
bbedward
left a comment
bbedward
left a comment
Collaborator
There was a problem hiding this comment.
Most of the changes are good, but I'm skeptical about the dbus ID changes breaking clients. That needs verification, as well as ensuring the greeter launcher changes are adhering to XDG specs, and generally flattening the verbosity of comments
| // of the same clientID formula for the streaming "subscribe" | ||
| // endpoint, which must stay in sync with this one). | ||
| clientID := fmt.Sprintf("meta-client-%p", conn) | ||
| serverDbus.HandleRequest(conn, req, dbusManager, clientID) |
Collaborator
There was a problem hiding this comment.
I suspect this probably breaks client signal delivery because ClientID lookup wont match in the QMl-side. Same problem in server.go, you can test plugins like DankKDEConnect to see if they still work afterwards
| // bug in any one handler (bad type assertion, nil deref, out-of-range | ||
| // index, etc. -- reachable from client-controlled req.Method/req.Params) | ||
| // can't crash the whole daemon and disconnect every other client. It | ||
| // reports back to the offending client as a normal error response instead. |
Collaborator
There was a problem hiding this comment.
I prefer to cut down all of these comments, its way too verbose for the code its covering which is largely self-documenting anyway
| // basic single/double quoting and backslash escapes, but never invokes a | ||
| // shell. Session .desktop files can live in a user-writable directory | ||
| // (~/.local/share/wayland-sessions), so shell metacharacters in Exec= | ||
| // must never reach an actual shell -- they're just literal argv text here. |
Collaborator
There was a problem hiding this comment.
Not a security fix since the user is already authenticated at this point, but maybe a spec correctness change
| hasCur = false | ||
| } | ||
| i++ | ||
| case c == '\'': |
Collaborator
There was a problem hiding this comment.
I believe XDG spec dictates single quotes shouldnt be treated as a quote, but a reserved character
|
|
||
| argv := make([]string, 0, len(tokens)) | ||
| for _, tok := range tokens { | ||
| if strings.HasPrefix(tok, "%") { |
Collaborator
There was a problem hiding this comment.
I would maybe reference quickshell code DeskopEntries for all of this stuff, to ensure its following the XDG specification
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While auditing the
corebackend for a downstream fork, I found a few security and concurrency issues that apply to upstream as well. Each is an independent, self-contained fix; happy to split into separate PRs if you'd prefer.Security
sudo -S) instead of embedding it in the command string, so it no longer appears inargv(readable by any local user via/proc/<pid>/cmdlineorps)..desktopExec=line into argv andexecvedirectly instead of running it through/bin/sh -c, closing a command-injection path via user-writable~/.local/share/wayland-sessions...in plugin id/name before joining into a filesystem path, closing an arbitrary-directory-delete in the uninstall/update fallback.Concurrency / robustness
Notes
core/only — no UI/QML changes.make build,go vet, and the keybinds tests pass on currentmaster.