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

kpmcore: patch trustedprefixes #191822

Merged
merged 1 commit into from
Nov 8, 2022
Merged

kpmcore: patch trustedprefixes #191822

merged 1 commit into from
Nov 8, 2022

Conversation

vlinkz
Copy link
Member

@vlinkz vlinkz commented Sep 18, 2022

Description of changes

Adds a patch to kpmcore to fix execution on Nix/NixOS.
Ever since the following commit, kpmcore checks where executions are being called from, and blacklists anything not in /{bin,sbin} or /usr/{bin,sbin}.
https://invent.kde.org/system/kpmcore/-/commit/6b260fa84e75944fd15c3fff1a77723086af2038

Things done

Patch the trusted prefix method to check for /nix/store or /run/current-system/sw instead.

  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@mweinelt
Copy link
Member

Wouldn't pushing our paths into the trustedprefixes file also work, since it works with prefixes?

@vlinkz
Copy link
Member Author

vlinkz commented Oct 22, 2022

If I remember correctly no, because then any other application using kpmcore would also need it's /nix/store/xxxxxxxx-program path pushed to the file. But I'll try it again and see

@vlinkz
Copy link
Member Author

vlinkz commented Nov 2, 2022

So for pushing our paths to trustedprefixes file, we would need to path this part of the code

    if (dirname == QStringLiteral("bin") || dirname == QStringLiteral("sbin")) {
        prefix.cdUp();
    }

To something maybe like

    if (dirname == QStringLiteral("bin") || dirname == QStringLiteral("sbin") || dirname <starts with long hash or something>) {
        prefix.cdUp();
    }

and then add /nix/store to trusted prefixes. I'm thinking the current implementation requires less patching and fewer modified files, so would probably be preferable.

substituteInPlace src/backend/corebackend.cpp \
--replace /usr/share/polkit-1/actions/org.kde.kpmcore.externalcommand.policy $out/share/polkit-1/actions/org.kde.kpmcore.externalcommand.policy
substituteInPlace src/util/externalcommandhelper.cpp \
--replace "(trustedPrefixes.find(prefix.path()) == trustedPrefixes.end())" "(!prefix.path().startsWith(QStringLiteral(\"/nix/store\")) && !prefix.path().startsWith(QStringLiteral(\"/run/current-system/sw\")))"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to create a patch file with description for this. It's not quite trivial.

Also isn't allowing everything in /nix/store insecure? For Nix, allowed-users are not necessary trusted-users. So there are some users can build things with nix-daemon but doesn't have root access.
I think we should only allow /run/current-system if it works. Privileged programs from desktop environments should be installed system-wide.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately only allowing /run/current-system does not work. Not sure how big of a security concern it is though given that trusted prefixes are a recent addition, and root permissions are still needed to actually modify partitions

@mweinelt
Copy link
Member

mweinelt commented Nov 7, 2022

@oxalica Please review so we can get this in. We need to know if the installer ISO works rather sooner than later.

@oxalica
Copy link
Contributor

oxalica commented Nov 7, 2022

@mweinelt

@oxalica Please review so we can get this in. We need to know if the installer ISO works rather sooner than later.

IMO adding any /nix/store is kind of worrying. But I'm assuming this implies your approval.


I tested in QEMU. partitionmanager now is able to create partition table or format partitions. But it fails with following messages when creating a new partition, even with "unformatted" type. I'm able to create the partition manually using sudo sfdisk /dev/sda.

Create a new partition (198.00 MiB, unformatted) on ‘/dev/sda’ 
Job: Create new partition on device ‘/dev/sda’ 
Command: sfdisk --force --append /dev/sda 

Failed to add partition ‘New Partition’ to device ‘/dev/sda’. 

Failed to add partition ‘New Partition’ to device ‘/dev/sda’. 
Create new partition on device ‘/dev/sda’: Error
Create a new partition (198.00 MiB, unformatted) on ‘/dev/sda’: Error

@vlinkz
Copy link
Member Author

vlinkz commented Nov 7, 2022

I tested in QEMU. partitionmanager now is able to create partition table or format partitions. But it fails with following messages when creating a new partition, even with "unformatted" type. I'm able to create the partition manually using sudo sfdisk /dev/sda.

Thought it could be since partitionmanager and kpmcore are out of sync in nixpkgs at the moment, but it worked when installed on my laptop, so not sure what that error could be. Are both partition-manager and libsForQt5.kpmcore in environment.systemPackages?

Copy link
Contributor

@oxalica oxalica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, it works with I rebase this to master.

@mweinelt mweinelt merged commit 2c05f56 into NixOS:master Nov 8, 2022
@vlinkz vlinkz deleted the kpmcorefix branch November 27, 2022 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants