| Summary: | mail-filter/policyd-weight <0.1.14.17 temporary file vulnerability (CVE-2008-{1569,1670}) | ||
|---|---|---|---|
| Product: | Gentoo Security | Reporter: | Matthias Geerdsen (RETIRED) <vorlon> |
| Component: | Vulnerabilities | Assignee: | Gentoo Security <security> |
| Status: | RESOLVED FIXED | ||
| Severity: | minor | CC: | chris, robtone, thijs, ticho, waja |
| Priority: | High | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | B3 [glsa] | ||
| Package list: | Runtime testing required: | --- | |
|
Description
Matthias Geerdsen (RETIRED)
2008-03-23 16:30:47 UTC
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 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! 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! 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. 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." (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. (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 (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" ) ) { 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. 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. 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. 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. Arches, please test and mark stable: =mail-filter/policyd-weight-0.1.14.17 Target keywords : "release x86" (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. (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. x86 stable I vote YES for a GLSA. Voting YES and filing request. Fixed in release snapshot. GLSA 200804-11 |