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/dhcpcd: don't assert for hardened malloc, use scudo instead #157430

Closed
wants to merge 1 commit into from

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Jan 30, 2022

Motivation for this change

#151795 broke the hardened test (see #151696 (comment)). not sure this is a good solution, but it is a solution. the hardened test still fails with this applied, but apparently not due to dhcpcd:

machine: must succeed: su -l alice -c 'nix ping-store'
machine # [   22.382193] su[973]: Successful su for alice by root
machine # [   22.391510] su[973]: pam_unix(su:session): session opened for user alice(uid=1000) by (uid=0)
machine # warning: 'ping-store' is a deprecated alias for 'store ping'
machine # error: experimental Nix feature 'nix-command' is disabled; use '--extra-experimental-features nix-command' to override
machine # [   22.521474] su[973]: pam_unix(su:session): session closed for user alice
machine: output: 
Test "nix-daemon cannot be used by all users" failed with error: "command `su -l alice -c 'nix ping-store'` failed (exit code 1)"
cleanup
kill machine (pid 8)
machine # qemu-system-x86_64: terminating on signal 15 from pid 6 (/nix/store/i6vabb4div9iy6lsl642d86k1q8riasn-python3-3.9.9/bin/python3.9)
(finished: cleanup, in 0.10 seconds)
kill vlan (pid 7)
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/)
  • 22.05 Release Notes (or backporting 21.11 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.

@risicle
Copy link
Contributor

risicle commented Jan 30, 2022

I don't think it's your problem but nixosTests.hardened is currently failing for me with

Test "nix-daemon cannot be used by all users" failed with error: "command `su -l alice -c 'nix ping-store'` failed (exit code 1)"

at least we can now see that failure though.

Copy link
Contributor

@risicle risicle left a comment

Choose a reason for hiding this comment

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

"works" for me, x86_64

@pennae
Copy link
Contributor Author

pennae commented Jan 30, 2022

yeah, that's the test failure noted up top. looks like the test doesn't like nix >=2.4.

@risicle
Copy link
Contributor

risicle commented Jan 30, 2022

Yes I should read things...

@pennae
Copy link
Contributor Author

pennae commented Jan 30, 2022

to be clear: we're very unsure this is the right solution. other memory allocators are also broken, so falling back to scudo can still cause some systems to fail to build. the mechanism by which this overrides the malloc implementation is also pretty ad-hoc and should probably be formalized somewhat, but no idea how that would even look (just adding it as an option to systemd.services probably won't be a good idea, since client utilities may also fail with overriden malloc, or fail differently)

@risicle
Copy link
Contributor

risicle commented Jan 31, 2022

Perhaps we could do with an environment variable that causes /etc/ld-nix.so.preload to be ignored. It would probably also need to be considered "unsafe" and ignored for setuid binaries.

@pennae
Copy link
Contributor Author

pennae commented Jan 31, 2022

we could achieve the same thing localized to dhcpcd by bind mounting an empty file over /etc/ld-nix.so.preload. for now that's probably safe for now since only the allocator selection uses this file, but out-of-tree users may add their own libraries here as well. feels like a pretty big hammer to disable all of them, and adding multiple preload paths isn't something we feel at all confident doing :/

@alyssais alyssais requested a review from emilazy July 8, 2022 16:47
alyssais added a commit to alyssais/nixpkgs that referenced this pull request Jul 8, 2022
Since 831024e ("nixos/dhcpcd: assert if privSep && alternative
malloc"), this test has an assertion failure because dhcpcd (with
privsep enabled) is not compatible with the allocator used by the
hardened profile.

Since it's unclear[1] what to do about this for the hardened profile,
I propose doing the simplest thing possible to make the test eval,
which is to just disable dhcpcd privsep.  It's very inconvenient when
trying to refactor the NixOS test infrastructure to have a test that
doesn't evaluate.  Once the correct solution is found for using dhcpcd
with privsep with the hardened profile, this patch can be reverted.

[1]: NixOS#157430
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 7, 2023
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@abathur
Copy link
Member

abathur commented Jun 29, 2024

@pennae Do you know if this is still relevant?

@pennae
Copy link
Contributor Author

pennae commented Jun 29, 2024

@pennae Do you know if this is still relevant?

no idea.

@pennae pennae closed this by deleting the head repository Jul 1, 2024
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

5 participants