Control: tag -1 +patch Hi Salvatore,
On 2017-06-09 20:23, Salvatore Bonaccorso wrote: > On Fri, Jun 09, 2017 at 07:40:18AM +0200, Salvatore Bonaccorso wrote: >> There is reported a group crontab to root escalation via the postinst >> in Debian and Ubuntu, as stated in the oss-security post: >> >> http://www.openwall.com/lists/oss-security/2017/06/08/3 >> >> Our postinst contains: >> >> | # Fixup crontab , directory and files for new group 'crontab'. >> | # Can't use dpkg-statoverride for this because it doesn't cooperate nicely >> | # with cron alternatives such as bcron >> | if [ -d $crondir/crontabs ] ; then >> | chown root:crontab $crondir/crontabs >> | chmod 1730 $crondir/crontabs >> | # This used to be done conditionally. For versions prior to "3.0pl1-81" >> | # It has been disabled to suit cron alternative such as bcron. >> | cd $crondir/crontabs >> | set +e >> | ls -1 | xargs -r -n 1 --replace=xxx chown 'xxx:crontab' 'xxx' >> | ls -1 | xargs -r -n 1 chmod 600 >> | set -e >> | fi >> >> which can be used for group-crontab-to-root escalation of privileges >> as described by Qualys team in the above reference. > > This has been assigned CVE-2017-9525. Please find attached a first draft of a (so far only rudimentally tested) patch for this issue. I replace the unconditional chown/chgrp of everything under /var/spool/cron/crontabs with a conditional solution. A file in that directory must now satisfy the following requirements: 1. It must be a regular file 2. It must have a hard link count of exactly 1 3. It's name must match its owner (the daemon expects this) We cannot really add a test for the group because the intention is to change it from whatever it currently is to what we expect. Adding a test for non-executability is something that could be considered. We'd have to check interoperability with other cron implementations. Please let me know what you think. Regards, Christian
diff --git a/debian/postinst b/debian/postinst
index ac97c9e..edc767f 100644
--- a/debian/postinst
+++ b/debian/postinst
@@ -60,8 +60,32 @@ if [ -d $crondir/crontabs ] ; then
# It has been disabled to suit cron alternative such as bcron.
cd $crondir/crontabs
set +e
- ls -1 | xargs -r -n 1 --replace=xxx chown 'xxx:crontab' 'xxx'
- ls -1 | xargs -r -n 1 chmod 600
+
+ # Iterate over each entry in the spool directory, perform some sanity
+ # checks (see CVE-2017-9525), and chown/chgroup the crontabs
+ for tab_name in $crondir/crontabs
+ do
+ tab_type=`stat -c '%F' "$tab_name"`
+ tab_links=`stat -c '%h' "$tab_name"`
+ tab_owner=`stat -c '%U' "$tab_name"`
+
+ if [ "$tab_type" != "regular file" -a "$tab_type" != "regular empty file" ]
+ then
+ echo "Warning: $tab_name is not a regular file!"
+ continue
+ elif [ "$tab_links" -ne 1 ]
+ then
+ echo "Warning: $tab_name has more than one hard link!"
+ continue
+ elif [ "$tab_name" != "$tab_owner" ]
+ then
+ echo "Warning: $tab_name name differs from owner $tab_owner!"
+ continue
+ fi
+
+ chown "$tab_owner:crontab" "$tab_name"
+ chmod 600 "$tab_name"
+ done
set -e
fi
signature.asc
Description: OpenPGP digital signature
