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

chmod the /build subdir to 750 #11807

Closed
wants to merge 1 commit into from

Conversation

gador
Copy link
Member

@gador gador commented Nov 5, 2024

Due to unknown reasons, some packages (notably yarn and npm) will stall during a build process. This causes a major problem, because the process cannot be killed and a cold-reset is needed to restart the system (a shutdown or reboot will hang trying to umount the partition where the build is happening).

By letting the /build subdirectory be group-readable by the nixbld group, the problem is fixed.

Also we do not sacrifice build privacy, because the parrent directory is owned by root and set to 700.

So even if we have a malicious setguid binary in one build and another tries to run it, it cannot access it because the parent folder is owned by root.

fixes #11806
fixes NixOS/nixpkgs#353709

Motivation

See linked issues above

Context

See linked issues above

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Due to unknown reasons, some packages (notably yarn and npm)
will stall during a build process. This causes a major problem,
because the process cannot be killed and a cold-reset is needed
to restart the system (a shutdown or reboot will hang trying to
umount the partition where the build is happening).

By letting the `/build` subdirectory be group-readable by the
nixbld group, the problem is fixed.

Also we do not sacrifice build privacy, because the parrent
directory is owned by `root` and set to `700`.

So even if we have a malicious setguid binary in one build
and another tries to run it, it cannot access it because the
parent folder is owned by `root`.

fixes NixOS#11806
fixes NixOS/nixpkgs#353709

Signed-off-by: Florian Brandes <[email protected]>
@gador
Copy link
Member Author

gador commented Nov 5, 2024

This does not in itself fix the issue. I've found that this patch works when on an older release of NixOS but not on newer NixOS versions. Investigating..

Copy link
Member

@edolstra edolstra left a comment

Choose a reason for hiding this comment

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

Fix looks good in principle, though it would be good to know what the underlying issue.

@@ -555,7 +555,7 @@ void LocalDerivationGoal::startBuilder()
possible. Any mitigation along these lines would have to be
done directly in the sandbox profile. */
tmpDir = topTmpDir + "/build";
createDir(tmpDir, 0700);
createDir(tmpDir, 0750);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment here why we're doing 0750 instead of 0700.

@gador gador marked this pull request as draft November 5, 2024 21:31
@gador
Copy link
Member Author

gador commented Nov 6, 2024

This is not needed. Bug is in the kernel, not in nix. See related issues

@gador gador closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential regression due to chroot Kernel 6.6.57+ io_uring stall ("yarn install takes indefinitely")
2 participants