Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/plasma5: remove pointless setuid wrappers #254071

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

alois31
Copy link
Contributor

@alois31 alois31 commented Sep 8, 2023

The module for Plasma 5 contained two pointless setuid wrappers:

  • kscreenlocker_greet was introduced when the kscreenlocker package dropped kcheckpass. However, this was actually replaced by making proper use of PAM (which finally calls its unix_chkpwd setuid binary). kscreenlocker_greet itself was never intended to be setuid. Fortunately, this is not exploitable, because QCoreApplication immediately aborts if it detects setuid. The wrapper is still incorrect and pointless, so remove it.
  • start_kdeinit can optionally use setuid root or setcap CAP_SYS_RESOURCE to reduce its OOM killer score. However, with systemd startup, start_kdeinit does not get used at all. So in this case, the setuid wrapper is pointless, and so is removed as well. Ideally, the case where systemd startup is not enabled would use a capability wrapper instead, but since systemd startup is the default in NixOS and kinit is deprecated upstream for KF6, I don't bother any more.

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

The module for Plasma 5 contained two pointless setuid wrappers:
* kscreenlocker_greet was introduced when the kscreenlocker package
  dropped kcheckpass. However, this was actually replaced by making
  proper use of PAM (which finally calls its unix_chkpwd setuid binary).
  kscreenlocker_greet itself was never intended to be setuid.
  Fortunately, this is not exploitable, because QCoreApplication
  immediately aborts if it detects setuid. The wrapper is still
  incorrect and pointless, so remove it.
* start_kdeinit can optionally use setuid root or setcap
  CAP_SYS_RESOURCE to reduce its OOM killer score. However, with systemd
  startup, start_kdeinit does not get used at all. So in this case, the
  setuid wrapper is pointless, and so is removed as well. Ideally, the
  case where systemd startup is not enabled would use a capability
  wrapper instead, but since systemd startup is the default in NixOS and
  kinit is deprecated upstream for KF6, I don't bother any more.
@ajs124 ajs124 requested a review from K900 September 20, 2023 22:47
@K900 K900 merged commit daebf5c into NixOS:master Sep 22, 2023
21 checks passed
K900 pushed a commit that referenced this pull request Nov 4, 2023
In #254071, a mismatch between usage of
the Nix language and the NixOS module system was introduced. By merging the
kwin_wayland wrapper attrset into the mkIf representation, the former was
effectively ignored.
As a result, the capability wrapper for kwin_wayland stopped being installed,
leading to realtime scheduling being disabled. The issue was not detected
because the behavioral change is very subtle.

By consistently using language-level constructs, this mismatch is resolved.
The capability wrapper is thus installed again and realtime scheduling is
restored.
nyabinary pushed a commit to nyabinary/nixpkgs that referenced this pull request Nov 14, 2023
In NixOS#254071, a mismatch between usage of
the Nix language and the NixOS module system was introduced. By merging the
kwin_wayland wrapper attrset into the mkIf representation, the former was
effectively ignored.
As a result, the capability wrapper for kwin_wayland stopped being installed,
leading to realtime scheduling being disabled. The issue was not detected
because the behavioral change is very subtle.

By consistently using language-level constructs, this mismatch is resolved.
The capability wrapper is thus installed again and realtime scheduling is
restored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants