214403 2008-03-23 16:30 0000 mail-filter/policyd-weight <0.1.14.17 temporary file vulnerability (CVE-2008-{1569,1670}) 2008-04-11 17:10:37 0000 1 1 1 Unclassified Gentoo Security Vulnerabilities unspecified All Linux RESOLVED FIXED B3 [glsa] P2 minor --- 1 vorlon@gentoo.org security@gentoo.org chris@chrishowells.co.uk robtone@ek-muc.de thijs@debian.org ticho@gentoo.org waja@cyconet.org vorlon@gentoo.org 2008-03-23 16:30:47 0000 The following has been reported to us by Chris Howells (CC'ed), thanks for that. This bug is confidential for now. Chris, besides security@debian.org did you also try to contact upstream? Hi, I believe I have discovered an insecure temporary file vulnerability in policyd-weight, which is in the repositories of Debian etch. Snippets of code from /usr/sbin/policyd-weight my $LOCKPATH = '/tmp/.policyd-weight/'; Then the function create_lockpath chown's lockpath: sub create_lockpath { my $who = shift(@_); if(!( -d $LOCKPATH)) { mkdir $LOCKPATH or die "$who: error while creating $LOCKPATH: $!"; } my $tuid = $USER; if($USER =~ /[^0-9]/) { if( !(defined( $tuid = getpwnam($USER) ) ) ) { mylog(warning=>"User $USER doesn't exist, create it, or set \$USER"); } } if( !(chown ($tuid, -1, $LOCKPATH)) ) { mylog(warning=>"Couldn't chown $LOCKPATH to $USER: $!"); } if( !(chmod (0700, $LOCKPATH)) ) { mylog(warning=>"Couldn't set permissions on $LOCKPATH: $!"); } } I have verified that by doing something like this: mkdir /home/chris/foo ln -s /home/chris/foo /tmp/.policyd-weight and starting policyd-weight you can change the ownership or any arbitrary directory around the system - /home/chris/foo got chown'ed to polw:root. I'm sure you can also play tricks with the creation and deletion of the socket too as they get unlink'ed in a few places, but haven't bothered trying to exploit them as I think this is plenty to be sure that there is a problem. vorlon@gentoo.org 2008-03-23 17:31:05 0000 from Chris, who currently can't post to this bug cause of bugzilla trouble: "No, I have not contacted upstream, it doesn't seem to be very actively developed any more. I have however reported it to the FreeBSD security team in addition to you and Debian." There is a note on the website btw: Policyd-weight is not actively maintained as of 09th Feb 2008. linking to: http://news.gmane.org/find-root.php?group=gmane.mail.postfix.policyd-weight&article=810 ticho@gentoo.org 2008-03-24 02:54:01 0000 Last relevant e-mails on upstream mailinglist mention new developers being added to policyd-weight sf.net project. Currently there are four, two of them with admin rights (including old maintainer, who is thought to be inactive). I have sent them an e-mail with Chris' report (Chris was also CCd). Let's see if they respond. Thanks for the report, Chris! ticho@gentoo.org 2008-03-27 09:15:56 0000 Old maintainer responded on policyd-weight mailing list, releasing a new version (0.1.14.15), which should have this issue fixed. I just committed it to portage. Can someone please verify if the fix is working? I have to head to work now and will not be able to test until tonight. Thanks! ticho@gentoo.org 2008-03-27 22:53:35 0000 OK, looks like latest version is no good - I was still able to change ownership of /root to polw:root. I reported this upstream, let's wait for them. rbu@gentoo.org 2008-03-28 00:20:19 0000 The patch introduces a symlink check, but I do not see how that is safe to race attacks. Looking at the "create_lockpath" function, it should be possible for an attacker to previously create the $LOCKPATH directory, and replace it (after the symlink check) by a symlink. The socket and data files should be moved to /var/run The website also states [1]: "Workaround: change $LOCKPATH from /tmp/.policyd-weight to /var/run/.policyd-weight. You need to manually kill policyd-weight and to remove the old directory." rbu@gentoo.org 2008-03-28 00:21:01 0000 public via http://www.us.debian.org/security/2008/dsa-1531 thijs@debian.org 2008-03-28 09:15:43 0000 (In reply to comment #6) > public via http://www.us.debian.org/security/2008/dsa-1531 Just to note that this was already publicised on the upstream mailinglist last Sunday, the DSA itself did not make it public. robtone@ek-muc.de 2008-03-28 13:23:48 0000 (In reply to comment #5) > The patch introduces a symlink check, but I do not see how that is safe to race > attacks. Looking at the "create_lockpath" function, it should be possible for > an attacker to previously create the $LOCKPATH directory, and replace it (after > the symlink check) by a symlink. > > The socket and data files should be moved to /var/run > The website also states [1]: "Workaround: change $LOCKPATH from > /tmp/.policyd-weight to /var/run/.policyd-weight. You need to manually kill > policyd-weight and to remove the old directory." I'd like to change the working directory after handling of such /tmp issues has been sorted out; would the following patch suffice?: --- old/policyd-weight Fri Mar 28 10:06:46 2008 +++ new/policyd-weight Fri Mar 28 14:08:30 2008 @@ -23,9 +23,9 @@ # see http://spf.pobox.com/ # # AUTHOR: r.felber@ek-muc.de -# DATE: Fri Mar 28 10:08:42 CET 2008 +# DATE: Fri Mar 28 14:08:20 CET 2008 # NAME: policyd-weight -# VERSION: 0.1.14 beta-16 +# VERSION: 0.1.14 beta-17 # URL: http://www.policyd-weight.org/ @@ -65,6 +65,7 @@ # begin use strict; use Fcntl; +use File::Spec; use Sys::Syslog qw(:DEFAULT setlogsock); use Net::DNS; use Net::DNS::Packet qw(dn_expand); @@ -78,7 +79,7 @@ use vars qw($csock $s $tcp_socket $sock $new_sock $old_mtime); -our $VERSION = "0.1.14 beta-16"; +our $VERSION = "0.1.14 beta-17"; our $CVERSION = 5; # cache interface version our $CMD_DEBUG = 0; # -d switch our $KILL; # -k switch @@ -3614,6 +3615,9 @@ } + + + # # usage: check_symlnk($caller, @files) # @@ -3625,17 +3629,53 @@ for ( @_ ) { - # strip trailing '/' - # perl and test(1) ignore the request for -l/-L and - # do a lstat with S_IFDIR (added in 0.1.14 beta-16) - s/\/+$//; + my $file = File::Spec->canonpath($_); + + my @stat = lstat( $file ); - if ( -l $_ ) + if(! -e _) { - fatal_exit("$who: $_ is a symbolic link. Symbolic links are not expected and not allowed within policyd-weight. Exiting!"); + next; } + + + # first, file must not be a symlink + if ( -l _ ) + { + fatal_exit("$who: $file is a symbolic link. Symbolic links are not expected and not allowed within policyd-weight. Exiting!"); + } + + + # second, file must be owned by uid root or $USER and + # gid root/wheel or $USER + if(! + ( + $stat[4] == getpwnam($USER) || + $stat[4] == "0" + ) && + ( + $stat[5] == getgrnam($GROUP) || + $stat[5] == "0" + ) + ) + { + fatal_exit("$who: $file is owned by UID $stat[4], GID $stat[5]. Exiting!"); + } + + + # second the file/dir must not be world writeable + if(sprintf("%04o",$stat[2]) =~ /(7|6|3|2)$/) + { + fatal_exit("$who: $file is world writeable. Exiting!"); + } + + } } + + + + # function for sanitizing floating point output robtone@ek-muc.de 2008-03-28 14:07:47 0000 (In reply to comment #8) > (In reply to comment #5) > > The patch introduces a symlink check, but I do not see how that is safe to race > > attacks. Looking at the "create_lockpath" function, it should be possible for > > an attacker to previously create the $LOCKPATH directory, and replace it (after > > the symlink check) by a symlink. > > > > The socket and data files should be moved to /var/run > > The website also states [1]: "Workaround: change $LOCKPATH from > > /tmp/.policyd-weight to /var/run/.policyd-weight. You need to manually kill > > policyd-weight and to remove the old directory." > > I'd like to change the working directory after handling of such /tmp issues has > been sorted out; would the following patch suffice?: Previous patch was buggy (missing two brackets), here is a patch for the patch: --- old/policyd-weight Fri Mar 28 15:01:29 2008 +++ new/policyd-weight Fri Mar 28 14:45:38 2008 @@ -3648,15 +3648,14 @@ # second, file must be owned by uid root or $USER and # gid root/wheel or $USER - if(!( - ( - $stat[4] == getpwnam($USER) || - $stat[4] == "0" - ) && - ( - $stat[5] == getgrnam($GROUP) || - $stat[5] == "0" - ) + if(! + ( + $stat[4] == getpwnam($USER) || + $stat[4] == "0" + ) && + ( + $stat[5] == getgrnam($GROUP) || + $stat[5] == "0" ) ) { ticho@gentoo.org 2008-03-28 18:22:03 0000 Robert, 1. Your patch is based on version 0.1.14 beta-16, which I did not even know was released 2. Please do not post patches inline, as it tends to break patch format (line breaks, whitespace) after copy&pasting. Instead, please attach the patch as text/plain. That said, thanks for the time you are still putting into policyd-weight. ticho@gentoo.org 2008-03-28 18:28:38 0000 The newly released 0.1.14 beta-17 (fresh in portage as policyd-weight-0.1.14.17) seems like it's checking for symlinks correctly. @rbu: Perhaps it's time to involve arch teams to stabilize this version. ticho@gentoo.org 2008-03-29 00:16:58 0000 I have just updated the ebuild to change default $LOCKPATH to /var/run/policyd-weight, which is created with 0700 permissions and owned by polw:root. @security: Now would be the best time to involve arch teams. rbu@gentoo.org 2008-03-29 00:56:00 0000 Robert, the check via lstat seems plausibly secure to me. Please consider using the /var/run directory though, as that is what the directory is intended for. rbu@gentoo.org 2008-03-29 00:57:55 0000 Arches, please test and mark stable: =mail-filter/policyd-weight-0.1.14.17 Target keywords : "release x86" robtone@ek-muc.de 2008-03-29 09:03:28 0000 (In reply to comment #10) > Robert, > > 1. Your patch is based on version 0.1.14 beta-16, which I did not even know was > released I published a patch containing version information on the ML. Although I didn't release it. > 2. Please do not post patches inline, as it tends to break patch format (line > breaks, whitespace) after copy&pasting. Instead, please attach the patch as > text/plain. Sorry. Haven't seen the box for attachments. I apologize. > That said, thanks for the time you are still putting into policyd-weight. I'll do so also in the future (at least for security related stuff and code for which I was responsible). I personally still use it, too. But I don't to development on it anymore. robtone@ek-muc.de 2008-03-29 09:08:52 0000 (In reply to comment #13) > Robert, the check via lstat seems plausibly secure to me. Please consider using > the /var/run directory though, as that is what the directory is intended for. > Yes. Will do. This requires some preparation for the unaware user, though. Also I wanted to sort this out, instead of run away to other file locations while keeping the initial checking issues. I wanted first to sort this out to protect users wich change a possible secure temporarily working directory to an insecure directory like /tmp. fauli@gentoo.org 2008-03-29 18:18:45 0000 x86 stable rbu@gentoo.org 2008-03-29 19:47:44 0000 I vote YES for a GLSA. keytoaster@gentoo.org 2008-03-29 20:06:16 0000 Voting YES and filing request. pva@gentoo.org 2008-03-30 11:13:43 0000 Fixed in release snapshot. rbu@gentoo.org 2008-04-11 17:10:37 0000 GLSA 200804-11