security: close the privesc chain, harden pkexec, fix two silent failures - #27
Closed
vietanhdev wants to merge 6 commits into
Closed
security: close the privesc chain, harden pkexec, fix two silent failures#27vietanhdev wants to merge 6 commits into
vietanhdev wants to merge 6 commits into
Conversation
Nothing proved the packages RUN. 'npm run tauri build exited 0' and 'the .deb is 8MB' are both true of a binary that dies before it draws a window -- and this app builds a tray icon unconditionally against libayatana-appindicator3, the classic dlopen-panic shape. The hard part is that ThinkUtils reads /proc/acpi/ibm/fan and /sys/class/power_supply/BAT*, none of which exist in a container. So the suite does not test hardware paths. It tests that the app starts, renders its full UI, and degrades cleanly when the hardware is absent, using three hardware-independent signals: index.html ships only two visible strings plus empty containers. Every other word on screen arrives because templateLoader.js fetched a template over tauri:// and injected it. Those labels are literal markup that no /proc or /sys read produces, so OCR finding them proves the JS ran, on any machine. OCR finding index.html's static text while finding NO injected label is the exact signature of 'WebKit loaded the page, the JS died' -- checked explicitly, because every other assertion passes in that state. The app now prints hw probe / hw mode, so 'no ThinkPad here' is an observed state rather than an inference. The frontend reports uncaught exceptions to the backend. A view dying on an absent sysfs path leaves the sidebar painted and the process alive; that error line is the only tell. Running it immediately found two real bugs, both fixed here: hw mode reported 'full' inside a container. Containers inherit the host's /sys, so bat0 and cpufreq were present and only the fan interface was missing. Keyed on the fan interface now -- a battery and cpufreq exist on every Linux laptop and prove nothing about ThinkPad support. The OCR assertion missed the first-run permissions dialog. On a machine that has never been set up -- every container -- the app correctly opens that dialog over the main view, so the sidebar is not what is on screen. It comes from templates/dialogs.html, so it is injected template text and proves the same thing. Verified locally against real builds: deb on ubuntu:24.04 PASSES, and rpm on fedora:41 PASSES -- the first time the .rpm has ever been tested by anything. Artifact selection resolves the CURRENT version rather than taking the only match or the first: three stale builds were sitting in the bundle directory, and release.yml selects with 'ls *.deb | head -n 1', which sorts 0.1.10 before 0.1.5 and would rename an old package to the new version's name and publish it. Also fixes .gitignore, where a missing newline had merged two entries into 'docs/.vitepress/cachebuild/' -- so neither the VitePress cache nor build output was ignored.
The Download nav link pointed straight at the GitHub releases list, which shows every asset for every version and leaves the reader to work out which file they want. docs/download.md resolves the latest release through the GitHub API at view time rather than baking a version in at build time -- the docs site and the release pipeline deploy independently, so a hard-coded version would go stale the moment a release ships without a docs rebuild. If the API is unreachable or rate-limits (60/hour unauthenticated), every button falls back to the releases page, which always works. Assets are matched by predicate rather than exact filename so a version bump needs no edit here, and each predicate pins the architecture suffix. That is load-bearing rather than tidiness: find() returns the FIRST match, so a loose predicate would silently hand out the wrong package the day a second architecture is added, and the failure is quiet and user-side -- the page looks right, the download works, the package refuses to install. getting-started.md is restructured as an ordered path rather than a list of prerequisites, and leads with the question a reader actually has -- 'will this work on my machine' -- answered by one command. Step 2 is now stated as the step people miss, because it is: the thinkpad_acpi module refuses every fan write unless loaded with fan_control=1, and that is fixed at module load, so granting permissions cannot fix it. Presenting it as one prerequisite among several is what produces the 'I granted permissions and nothing happened' report. Also documents the Ubuntu 22.04 polkit 0.105 limitation, which silently makes passwordless fan control not work there, and the fan safety behaviour -- revert on disable, on unreadable sensors, on exit, plus the firmware watchdog.
…anifests
Distro packaging was blocked by one thing: the fan helper installed to
/usr/local/bin at runtime. Debian Policy 9.1.2 and the Fedora guidelines
both forbid a package writing there, and a helper materialised by a
button click is not package-owned -- dpkg -L would not list the most
security-sensitive file the app uses, and uninstalling would leave a
root-owned binary and a polkit rule behind.
HELPER_PATH becomes HELPER_CANDIDATES, searched in order:
/usr/lib/thinkutils/... Debian and Arch convention
/usr/libexec/thinkutils/... Fedora convention
/usr/local/bin/... legacy self-install, kept so existing
installs keep working
setup_permissions() now skips installing the helper and rule entirely
when helper_is_packaged(), because overwriting those files puts the
package database out of sync with the filesystem.
The path had been duplicated in four places -- fan_control, mcp,
fan_curve, and inside the polkit rule text. The rule is now generated
from the constant, since a rule naming a path the helper is not at
grants nothing while looking correct.
Two security improvements fall out of generating it:
subject.local && subject.active is now required. Without it any SSH
session belonging to a wheel/sudo user inherited passwordless
hardware control, as did a background session the user had switched
away from.
Packages ship the rule to /usr/share/polkit-1/rules.d, not /etc.
/etc is the administrator's namespace; a package writing there
shadows their rules and is never cleaned up.
packaging/ adds the AUR PKGBUILD and COPR spec, plus the helper and rule
as generated artifacts (cargo run --example gen-packaging).
tests/packaging.rs is the point of all this: 7 tests asserting the
committed files match what the source generates, that each format
installs where the app actually searches, that nothing writes to
/usr/local, that the rule goes under /usr/share, and that versions
agree. The drift they prevent fails SILENTLY -- polkit denies, the app
falls back to a password prompt, and it reads as a permissions problem
rather than a packaging bug.
bump-version.sh now covers PKGBUILD and the spec too, and ci.yml calls
it rather than keeping its own copy of the file list, so adding a
packaging file cannot leave CI checking a stale subset.
Suite: 80 -> 87.
Closes the other half of the local privilege-escalation chain. The first half -- unvalidated governor into a root shell -- was fixed earlier; this is the part that let a local user reach it. monitor.js rendered proc.name straight into innerHTML, and that string is the COMMAND column of `ps aux`. Any local user can name a binary `<img src=x onerror=...>`. With csp:null and withGlobalTauri:true, the injected script got the full __TAURI__ API -- including commands that end in pkexec. escapeHtml existed but was private to security.js, so every other view rendering external strings had none. It moves to utils.js and is applied to process names and status, disk mount points and devices, network interface names, battery strings, and sensor labels. The CSP replaces null with default-src 'self'; script-src 'self'; object-src 'none'; frame-ancestors 'none'. script-src deliberately has no unsafe-inline or unsafe-eval, which would defeat the point. style-src does allow unsafe-inline, because the templates use inline style attributes -- verified rather than assumed. Verified by building the real packages and running the container launch test: the frontend still fetches its 12 templates, injects them and paints. A CSP that broke template loading would have looked identical to a working one in unit tests. Tests: csp_is_set_and_restrictive asserts the directives and that script-src stays strict; views_escape_untrusted_strings asserts escapeHtml is shared and that proc.name specifically is escaped.
Five call sites each had their own copy of: build a script, write it to a predictable /tmp path with plain fs::write, chmod it, hand it to pkexec bash. The copies had drifted, so only some had either fix. fs::write on a predictable path follows symlinks and will happily open a file another user pre-created. auth.rs was the worst: /tmp/thinkutils_auth.sh, a fixed name with no randomness at all, so any local user could plant that path and have their content executed as root. privileged::run_script() replaces all of them. Creation is O_EXCL with a random name and mode 0600, which fails rather than following a symlink or reusing a planted file, and the script is always removed -- including when pkexec fails to launch, which several copies leaked. Migrated: performance.rs governor/turbo/boost, battery.rs thresholds, auth.rs, and fan_control.rs's own fallback. fan_control's create_secure_temp_script is gone; it was a second implementation of the same idea, which is how the drift started. Honest about what this does not fix: the file is owned by the invoking user between write and root execution, so that user could swap its contents. That matters only where an administrator authenticates on behalf of a less-privileged user, and closing it means not handing root a user-owned script at all -- the shape the fan helper already uses. Said so in the module docs rather than implying the problem is gone. security.rs also calls pkexec but passes arguments directly with no script file, so it has no equivalent exposure. Tests: mode is 0600, consecutive calls get distinct paths, and create_new refuses an existing path -- the last being the property that actually defeats the planted-file attack.
…th port Two silent failures, both from the same cause: the same thing named in two places, drifting apart. BATTERY THRESHOLDS permissions.rs granted write access to charge_start_threshold and charge_stop_threshold, while battery.rs wrote charge_control_start_threshold and charge_control_end_threshold. On a ThinkPad BOTH pairs exist and report the same value -- confirmed on hardware, both 75/80 -- but they are separate sysfs files, so a chmod on one never affected the other. The result: 'Grant Permissions' reported success and battery thresholds stayed unwritable, so every change fell through to a password prompt with no explanation. mcp.rs named a third variant. battery::threshold_paths() is now the single source of truth, preferring the generic kernel names and falling back to the thinkpad_acpi spelling. permissions.rs and mcp.rs both go through it. Also removed /sys/devices/platform/thinkpad_hwmon/pwm1 from the required list: that path does not exist. The real attribute is under .../thinkpad_hwmon/hwmon/hwmonN/pwm1, and the exists() guard meant the wrong path was skipped rather than reported. It is discovered now. PORT COLLISION The MCP server defaulted to 8765, which is the port sync.rs binds for the OAuth callback. With MCP running the callback listener could not bind, so Google sign-in never completed and nothing said why. MCP moves to 8779. It was the one to move: its port is local config, while the callback port is registered as the redirect URI in Google Cloud Console and cannot change without updating the OAuth client. Tests pin both: that the two ports differ, that REDIRECT_URI still embeds the callback port (it is a literal, since a const cannot call format!), that the generic attribute names are preferred, and that a candidate pair never mixes naming schemes -- writing a generic start with a legacy stop would touch two different files. Docs and the MCP view updated to 8779, with a note explaining the change for anyone who configured a client against the old port.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
thinkutils | 5165c8b | Jul 19 2026, 01:45 PM |
Owner
Author
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
1 participant
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.
Four fixes. Two are security, two are bugs where the same thing was named in two places and drifted.
XSS + CSP — closes the privilege-escalation chain
monitor.jsrenderedproc.nameintoinnerHTML, and that string is the COMMAND column ofps aux. Any local user can name a binary<img src=x onerror=...>. Withcsp: nullandwithGlobalTauri: true, the injected script got the full__TAURI__API — including commands that end inpkexec.The governor half was fixed earlier; this is the half that let a local user reach it.
escapeHtmlexisted but was private to security.js, so every other view rendering external strings had none. Moved toutils.jsand applied to process names, mount points, device names, interface names, battery strings and sensor labels.CSP is now
default-src 'self'; script-src 'self'; object-src 'none'; frame-ancestors 'none'.script-srcdeliberately has nounsafe-inline.style-srcdoes, because the templates use inline style attributes — verified rather than assumed by building the real packages and running the container launch test: the frontend still fetches its 12 templates and paints.One safe path for running a script as root
Five call sites each had their own copy of: write a script to a predictable
/tmppath with plainfs::write, chmod it, hand it topkexec bash.fs::writefollows symlinks and will open a file another user pre-created.auth.rswas worst —/tmp/thinkutils_auth.sh, a fixed name with no randomness.privileged::run_script()replaces all of them:O_EXCL, random name, mode 0600, and always cleans up — including when pkexec fails to launch, which several copies leaked.Honest about the residual: the file is user-owned between write and root execution, so that user could swap its contents. That matters only where an admin authenticates for a less-privileged user. Documented rather than implied away.
Battery thresholds were never actually grantable
permissions.rsgrantedcharge_start_threshold;battery.rswrotecharge_control_start_threshold. On a ThinkPad both exist and report the same value — confirmed on hardware, both 75/80 — but they're separate sysfs files, so the chmod never affected what was written.So "Grant Permissions" reported success and every threshold change still fell through to a password prompt.
mcp.rsnamed a third variant.battery::threshold_paths()is now the single source of truth.Also removed
/sys/devices/platform/thinkpad_hwmon/pwm1from the required list — that path doesn't exist; the real one is underhwmon/hwmonN/, and theexists()guard meant it was silently skipped.MCP was stealing the OAuth callback port
Both defaulted to 8765. With MCP running, the callback listener couldn't bind and Google sign-in never completed, with nothing saying why.
MCP moves to 8779 — it's the one to move, since its port is local config while the callback port is registered as the redirect URI with Google.
Tests
97 → the suite now pins: CSP directives and that
script-srcstays strict, thatproc.namespecifically is escaped, that temp scripts are 0600 with distinct paths and thatcreate_newrefuses an existing path, that the two ports differ, thatREDIRECT_URIstill embeds the callback port, and that a threshold pair never mixes naming schemes.