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/wrappers: make mount have the +s bit. #95444

Merged
merged 2 commits into from Aug 16, 2020
Merged

Conversation

doronbehar
Copy link
Contributor

See
https://discourse.nixos.org/t/how-to-make-a-derivations-executables-have-the-s-permission/8555
and:
https://www.linuxquestions.org/questions/slackware-14/must-be-superuser-to-use-mount-fstab-is-correct-however-144932/

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-make-a-derivations-executables-have-the-s-permission/8555/6

@khumba
Copy link
Contributor

khumba commented Aug 15, 2020

Hello, there was some previous discussion on this at #9848. I still support doing this, it would be useful, maybe behind a option for those that don't want setuid, and/or a warning if user or users is used and mount isn't setuid. As far as I know, there was no follow-up from that issue.

@doronbehar
Copy link
Contributor Author

Wow this has been talked over a lot! I still don't understand the security concern against the mere assertion you wanted to suggest in #9998 . Do you know how much time I wasted on trying to find this out?

For the protocol, I can't use udisks as I'm not interested in mounting a device. I want to mount --bind a local directory.

@edolstra you are the boss. I'd like to save future users at least the time I spent debugging this - If you are against even a mere assertion such as in https://github.com/NixOS/nixpkgs/pull/9998/files :

          any (fs: [] != intersectLists (splitString "," fs.options)
                                        [ "owner" "user" "users" ])
              fileSystems
          -> all (x: elem x config.security.setuidPrograms)
                 [ "mount" "umount" ];
        message = ''
          The fstab "owner", "user", and "users" options require "mount" and "umount"
          to be setuid.  Please add the following to your system configuration:
              security.setuidPrograms = [ "mount" "umount" ];
        '';

How about only mentioning this in the options description? Example:

diff --git i/nixos/modules/tasks/filesystems.nix w/nixos/modules/tasks/filesystems.nix
index 0ade74b957a..796d8e8220f 100644
--- i/nixos/modules/tasks/filesystems.nix
+++ w/nixos/modules/tasks/filesystems.nix
@@ -52,9 +52,16 @@ let
       options = mkOption {
         default = [ "defaults" ];
         example = [ "data=journal" ];
-        description = "Options used to mount the file system.";
-        type = types.listOf nonEmptyStr;
-      };
+        description = ''
+          Options used to mount the file system - the . Note that using either of the
+          options <literal>users</literal>, <literal>user</literal>,
+          <literal>owner</literal> will require you to make `mount` have the +s
+          permission, which is a known security risk. See <link
+          xlink:href="https://www.linuxquestions.org/questions/slackware-14/must-be-superuser-to-use-mount-fstab-is-correct-however-144932/"
+          >this thread</link> and discussion at <link
+          xlink:href="https://github.com/NixOS/nixpkgs/issues/9848">#9848</link>
+        '';
+        type = types.listOf nonEmptyStr; };
 
     };

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

udisks is proposed is an alternative tool to mount things in #9848, but it's more tailored towards allowing users to mount block devices ad-hoc (which aren't in /etc/fstab anyway).

There's still usecases for the users option in /etc/fstab (which requires the suid bit via the wrapper) - while it's not particularily nice, it's better than silently keeping things broken, so 👍 for me on this PR.

@doronbehar
Copy link
Contributor Author

Thanks @flokli. The last push adds umount in the same manner.

@flokli
Copy link
Contributor

flokli commented Aug 15, 2020 via email

@doronbehar
Copy link
Contributor Author

Sure. I tested both the use case of expecting a mount denial when users is not specified and both my original use case.

nixos/tests/misc.nix Outdated Show resolved Hide resolved
nixos/tests/misc.nix Outdated Show resolved Hide resolved
@flokli
Copy link
Contributor

flokli commented Aug 15, 2020

black isn't happy, and asks to reflow. Can you do that, and also squash that (and the other suggestions) into d03da650d79b4b2b17768bd8f6aa021a696a35d0?

@doronbehar
Copy link
Contributor Author

black isn't happy, and asks to reflow.

Who is black?

@flokli
Copy link
Contributor

flokli commented Aug 15, 2020 via email

For NixOS#95444
Co-authored-by: Florian Klink <flokli@flokli.de>
@doronbehar
Copy link
Contributor Author

Oh I see - it's due to the now longer subtest descriptions. Now it's fixed.

@flokli flokli merged commit 609eb86 into NixOS:master Aug 16, 2020
wchresta pushed a commit to wchresta/nixpkgs that referenced this pull request Aug 17, 2020
For NixOS#95444
Co-authored-by: Florian Klink <flokli@flokli.de>
@doronbehar doronbehar deleted the fix/mount+s branch March 2, 2023 10:37
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

4 participants