Skip to content

Commit

Permalink
Fixes #18124: atomically replace /var/setuid-wrappers/ (#18186)
Browse files Browse the repository at this point in the history
Before this commit updating /var/setuid-wrappers/ folder introduced
a small window where NixOS activation scripts could be terminated
and resulted into empty /var/setuid-wrappers/ folder.

That's very unfortunate because one might lose sudo binary.

Instead we use two atomic operations mv and ln (as described in
https://axialcorps.com/2013/07/03/atomically-replacing-files-and-directories/)
to achieve atomicity.

Since /var/setuid-wrappers is not a directory anymore, tmpfs mountpoints
were removed in installation scripts and in boot process.

Tested:

- upgrade /var/setuid-wrappers/ from folder to a symlink
- make sure /run/setuid-wrappers-dirs/ legacy symlink is really deleted
  • Loading branch information
domenkozar authored Sep 1, 2016
1 parent 78cd9f8 commit a6670c1
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 16 deletions.
8 changes: 8 additions & 0 deletions nixos/doc/manual/release-notes/rl-1609.xml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ following incompatible changes:</para>
behavior of Redis 3.2</para>
</listitem>

<listitem>
<para>/var/setuid-wrappers/
<link xlink:href="https://github.com/NixOS/nixpkgs/pull/18124">is now a symlink so
it can be atomically updated</link>
and it's not mounted as tmpfs anymore since setuid binaries are located on /run/ as tmpfs.
</para>
</listitem>

<listitem>
<para>Gitlab's maintainence script gitlab-runner was removed and split up into the more clearer
gitlab-run and gitlab-rake scripts because gitlab-runner is a component of Gitlab CI.</para>
Expand Down
2 changes: 0 additions & 2 deletions nixos/modules/installer/tools/nixos-install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,12 @@ fi
mkdir -m 0755 -p $mountPoint/dev $mountPoint/proc $mountPoint/sys $mountPoint/etc $mountPoint/run $mountPoint/home
mkdir -m 01777 -p $mountPoint/tmp
mkdir -m 0755 -p $mountPoint/tmp/root
mkdir -m 0755 -p $mountPoint/var/setuid-wrappers
mkdir -m 0700 -p $mountPoint/root
mount --rbind /dev $mountPoint/dev
mount --rbind /proc $mountPoint/proc
mount --rbind /sys $mountPoint/sys
mount --rbind / $mountPoint/tmp/root
mount -t tmpfs -o "mode=0755" none $mountPoint/run
mount -t tmpfs -o "mode=0755" none $mountPoint/var/setuid-wrappers
rm -rf $mountPoint/var/run
ln -s /run $mountPoint/var/run
for f in /etc/resolv.conf /etc/hosts; do rm -f $mountPoint/$f; [ -f "$f" ] && cp -Lf $f $mountPoint/etc/; done
Expand Down
34 changes: 27 additions & 7 deletions nixos/modules/security/setuid-wrappers.nix
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ let
installPhase = ''
mkdir -p $out/bin
cp ${./setuid-wrapper.c} setuid-wrapper.c
gcc -Wall -O2 -DWRAPPER_DIR=\"${wrapperDir}\" \
gcc -Wall -O2 -DWRAPPER_DIR=\"/run/setuid-wrapper-dirs\" \
setuid-wrapper.c -o $out/bin/setuid-wrapper
'';
};
Expand Down Expand Up @@ -102,11 +102,11 @@ in
source=/nix/var/nix/profiles/default/bin/${program}
fi
cp ${setuidWrapper}/bin/setuid-wrapper ${wrapperDir}/${program}
echo -n "$source" > ${wrapperDir}/${program}.real
chmod 0000 ${wrapperDir}/${program} # to prevent races
chown ${owner}.${group} ${wrapperDir}/${program}
chmod "u${if setuid then "+" else "-"}s,g${if setgid then "+" else "-"}s,${permissions}" ${wrapperDir}/${program}
cp ${setuidWrapper}/bin/setuid-wrapper $wrapperDir/${program}
echo -n "$source" > $wrapperDir/${program}.real
chmod 0000 $wrapperDir/${program} # to prevent races
chown ${owner}.${group} $wrapperDir/${program}
chmod "u${if setuid then "+" else "-"}s,g${if setgid then "+" else "-"}s,${permissions}" $wrapperDir/${program}
'';

in stringAfter [ "users" ]
Expand All @@ -115,9 +115,29 @@ in
# programs to be wrapped.
SETUID_PATH=${config.system.path}/bin:${config.system.path}/sbin
rm -f ${wrapperDir}/* # */
mkdir -p /run/setuid-wrapper-dirs
wrapperDir=$(mktemp --directory --tmpdir=/run/setuid-wrapper-dirs setuid-wrappers.XXXXXXXXXX)
${concatMapStrings makeSetuidWrapper setuidPrograms}
if [ -L ${wrapperDir} ]; then
# Atomically replace the symlink
# See https://axialcorps.com/2013/07/03/atomically-replacing-files-and-directories/
old=$(readlink ${wrapperDir})
ln --symbolic --force --no-dereference $wrapperDir ${wrapperDir}-tmp
mv --no-target-directory ${wrapperDir}-tmp ${wrapperDir}
rm --force --recursive $old
elif [ -d ${wrapperDir} ]; then
# Compatibility with old state, just remove the folder and symlink
rm -f ${wrapperDir}/*
# if it happens to be a tmpfs
umount ${wrapperDir} || true
rm -d ${wrapperDir}
ln -d --symbolic $wrapperDir ${wrapperDir}
else
# For initial setup
ln --symbolic $wrapperDir ${wrapperDir}
fi
'';

};
Expand Down
7 changes: 0 additions & 7 deletions nixos/modules/system/boot/stage-2-init.sh
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,6 @@ if [ -n "@useHostResolvConf@" -a -e /etc/resolv.conf ]; then
cat /etc/resolv.conf | resolvconf -m 1000 -a host
fi


# Create /var/setuid-wrappers as a tmpfs.
rm -rf /var/setuid-wrappers
mkdir -m 0755 -p /var/setuid-wrappers
mount -t tmpfs -o "mode=0755" tmpfs /var/setuid-wrappers


# Log the script output to /dev/kmsg or /run/log/stage-2-init.log.
# Only at this point are all the necessary prerequisites ready for these commands.
exec {logOutFd}>&1 {logErrFd}>&2
Expand Down

4 comments on commit a6670c1

@dezgeg
Copy link
Contributor

@dezgeg dezgeg commented on a6670c1 Sep 2, 2016

Choose a reason for hiding this comment

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

@domenkozar The sudo test in tests.misc is failing, could it be due to this change?

http://hydra.nixos.org/build/39769273/nixlog/1/raw

subtest: sudo
machine: must succeed: su - sybil -c 'sudo true'
machine# [   37.093309] su[1165]: Successful su for sybil by root
machine# [   37.101065] su[1165]: pam_unix(su:session): session opened for user sybil by (uid=0)
machine# sudo: /run/current-system/sw/bin/sudo must be owned by uid 0 and have the setuid bit set
machine# [   37.384312] su[1165]: pam_unix(su:session): session closed for user sybil
machine: exit status 1
machine: output: 
error: command `su - sybil -c 'sudo true'' did not succeed (exit code 1)

@domenkozar
Copy link
Member Author

Choose a reason for hiding this comment

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

Might be, I'll look into it.

@joachifm
Copy link
Contributor

Choose a reason for hiding this comment

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

On my system the setuid-wrappers directory ends up being inaccessible to ordinary users

@Rotsor
Copy link
Contributor

@Rotsor Rotsor commented on a6670c1 Dec 30, 2016

Choose a reason for hiding this comment

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

This commit makes it so that nixos-rebuild switch to an older nixos version is broken after you switch to this version. The way it's broken is very bad too: after you attempt to switch to the old version, you lose all your setuid binaries, therefore losing access to sudo, so you often can't fix this without reboot. Is this an expected breakage? (I hope not) Can anything be done to prevent similar breakage in the future?

Please sign in to comment.