Skip to content

Fixing attribute name mistake: setguid => setgid #26657

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

Merged
merged 1 commit into from
Jun 17, 2017

Conversation

ixmatus
Copy link
Contributor

@ixmatus ixmatus commented Jun 16, 2017

Motivation for this change

Fixing a coding mistake in the changes made to nixos/modules/security/wrappers/default.nix for the setcap wrapper support.

The mistake was related to conditional logic checking if a setuid or setgid attribute was set on an argument set and if so if either were true. The mistake was that setgid was misspelled as setugid which means that if a wrapper program had setuid = false but setgid = true and no permissions attribute in the argset it would use the default mkSetuidProgram configuration.

This is not correct behavior because it means a program's user or group if neither of those attributes were not set in the argset would be defaulted to root and this is dangerous.

The fix is to correct the spelling mistake.

This was caught while investigating #26611.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

I have yet to build and test this, I will probably get to that tonight, so don't merge until that has happened.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
bagder Daniel Stenberg
@mention-bot
Copy link

@ixmatus, thanks for your PR! By analyzing the history of the files in this pull request, we identified @globin, @abbradar and @bjornfor to be potential reviewers.

@vcunat
Copy link
Member

vcunat commented Jun 16, 2017

It seems a safe change to me, but let's wait if you prefer. Typo introduced in e92b840.

@ixmatus
Copy link
Contributor Author

ixmatus commented Jun 17, 2017

@vcunat okay, I tested the build and I also verified that when services.postfix.enable = true; the sendmail program's uid is nobody and its gid is postdrop with the SGID bit set.

(I also tested that the other common setuid and setcap wrappers work correctly, not all, just a sanity check)

Thanks for the quick review.

@vcunat vcunat merged commit 5ca644c into NixOS:master Jun 17, 2017
vcunat added a commit that referenced this pull request Jun 17, 2017

Verified

This commit was signed with the committer’s verified signature.
bagder Daniel Stenberg
Fixes #26611.
vcunat added a commit that referenced this pull request Jun 17, 2017

Verified

This commit was signed with the committer’s verified signature.
bagder Daniel Stenberg
Fixes #26611.

(cherry picked from commit c416641)
@vcunat
Copy link
Member

vcunat commented Jun 17, 2017

Picked to 17.03, too, as the situation in the surrounding code seems very much the same.

ambrop72 pushed a commit to ambrop72/nixpkgs that referenced this pull request Jun 21, 2017
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 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.

None yet

3 participants