diff --git a/docs/notifications.md b/docs/notifications.md index 9e181aca..e4b7da26 100644 --- a/docs/notifications.md +++ b/docs/notifications.md @@ -94,12 +94,16 @@ an action and just expect click-to-jump. ### Click commands must be argv, not shell strings Prefer `--exec-arg` for every click command. Each `--exec-arg` contributes one -literal argument; the shell runs the resulting vector with -`Quickshell.execDetached(argv)` and **no shell**, so a value carrying data an -attacker controls — a downloaded video's title, a received filename, a crashed -process's name — is only ever a single argument and can never be reparsed as a -command. This is the parameterized form: pass untrusted data as its own -`--exec-arg` rather than quoting it into a string. +literal argument; the shell runs the resulting vector through +`Util.execArgv`, which invokes `bash -lc 'exec "$@"'` with the arguments as +**positional parameters** — never interpolated into the script text. bash +expands `"$@"` without re-tokenizing or re-evaluating it, so a value carrying +data an attacker controls — a downloaded video's title, a received filename, a +crashed process's name — is only ever a single argument and can never be +reparsed as a command. The login shell keeps the PATH and session environment +that GUI click targets (the screenshot editor, mpv, xdg-open) expect. This is +the parameterized form: pass untrusted data as its own `--exec-arg` rather than +quoting it into a string. `--exec` is the legacy free-form variant, run through `bash -lc`. It is safe only when the caller shell-quoted every interpolated value perfectly — the same diff --git a/shell/Commons/Util.qml b/shell/Commons/Util.qml index 64dac606..e3553ae8 100644 --- a/shell/Commons/Util.qml +++ b/shell/Commons/Util.qml @@ -54,12 +54,17 @@ QtObject { Quickshell.execDetached(["bash", "-lc", command]) } - // Run an argv vector directly, without a shell. Nothing in the array is - // reparsed, so an argument carrying attacker-controlled data (a filename, a - // title) can never turn into a command. Prefer this over execDetached for any - // command assembled from untrusted input. + // Run an argv vector safely: the script text is the constant `exec "$@"`, so + // the arguments only ever land in bash's positional parameters, which it + // expands without re-tokenizing or re-evaluating — a value carrying + // attacker-controlled data ($(id), a filename, a title) stays one literal + // argument and can never turn into a command. The login shell (-l) is what + // makes this a drop-in for execDetached: click actions launch GUI apps + // (tensaku, mpv, xdg-open) that need the same PATH and session environment the + // login shell set up. Prefer this over execDetached for any command assembled + // from untrusted input. function execArgv(argv) { - Quickshell.execDetached(argv) + Quickshell.execDetached(["bash", "-lc", 'exec "$@"', "bash"].concat(argv)) } function isPlainObject(value) { diff --git a/shell/plugins/notifications/NotificationLogic.js b/shell/plugins/notifications/NotificationLogic.js index 1229a37b..727940cb 100644 --- a/shell/plugins/notifications/NotificationLogic.js +++ b/shell/plugins/notifications/NotificationLogic.js @@ -75,7 +75,8 @@ function execFromHints(hints) { // The click action as an argv vector, sent by omarchy-notification-send // --exec-arg and carried as a JSON array string in the omarchy-exec-argv hint. -// The shell runs it with Quickshell.execDetached (no shell), so a value that an +// The shell runs it via Util.execArgv, which passes the arguments as bash +// positional parameters rather than interpolating them, so a value that an // attacker controls — a video title, a filename, a URL — is only ever one // argument and can never be reparsed as a command. This is the parameterized // form: the "prepared statement" to execFromHints's string concatenation. diff --git a/shell/plugins/notifications/Service.qml b/shell/plugins/notifications/Service.qml index 5df3e96c..794c4d9c 100644 --- a/shell/plugins/notifications/Service.qml +++ b/shell/plugins/notifications/Service.qml @@ -361,10 +361,11 @@ Item { if (index < 0 || index >= popupModel.count) return var entry = popupModel.get(index) - // Preferred path: an argv vector run without a shell, so data an attacker - // controls (a video title, a filename) is only ever an argument and can - // never be reparsed as a command. Detached so it outlives the shell, which - // the installer toasts depend on: they restart the shell as their first act. + // Preferred path: an argv vector whose arguments are passed as bash + // positional parameters (never interpolated into a command), so data an + // attacker controls (a video title, a filename) is only ever an argument and + // can never be reparsed as a command. Detached so it outlives the shell, + // which the installer toasts depend on: they restart it as their first act. var argv = NotificationLogic.parseExecArgv(entry ? entry.execArgv : "") if (argv) { Util.execArgv(argv)