Bugzilla Bug 214403

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.

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."

public via http://www.us.debian.org/security/2008/dsa-1531

(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