[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#700280: tpu: dnsmasq: chowning pid directory and writing there as root may lead to security issue



Package: release.debian.org
Severity: normal
User: release.debian.org@packages.debian.org
Usertags: tpu

Hi,

I prepared a tpu update for dnsmasq to isolate the fix for this
problem. It would not be appropriate for me to clear this for
upload myself; please could someone review and approve it.

Debdiff attached, versioned 2.62-3+deb70u1.
(Ignore the changelog.rej noise, it will not be in the upload.)

Maintainer: this is notice of intent to NMU this fix in to
wheezy, so I will delay by two days to give you chance to
object.

Thanks,

-- 
Jonathan Wiltshire                                      jmw@debian.org
Debian Developer                         http://people.debian.org/~jmw

4096R: 0xD3524C51 / 0A55 B7C5 1223 3942 86EC  74C3 5394 479D D352 4C51

<directhex> i have six years of solaris sysadmin experience, from
            8->10. i am well qualified to say it is made from bonghits
			layered on top of bonghits
diff -u dnsmasq-2.62/debian/changelog dnsmasq-2.62/debian/changelog
--- dnsmasq-2.62/debian/changelog
+++ dnsmasq-2.62/debian/changelog
@@ -1,3 +1,11 @@
+dnsmasq (2.62-3+deb70u1) testing-proposed-updates; urgency=low
+
+  * Non-maintainer upload.
+  * Backport fix for symlink attacks from 2.63-4
+    (Closes: #686484)
+
+ -- Jonathan Wiltshire <jmw@debian.org>  Sun, 10 Feb 2013 21:07:17 +0000
+
 dnsmasq (2.62-3) unstable; urgency=low
 
    * Do resolvconf and /etc/default startup logic when
only in patch2:
unchanged:
--- dnsmasq-2.62.orig/src/dnsmasq.c
+++ dnsmasq-2.62/src/dnsmasq.c
@@ -371,15 +371,48 @@
       /* write pidfile _after_ forking ! */
       if (daemon->runfile)
 	{
-	  FILE *pidfile;
+	  int fd, err = 0;
+
+	  sprintf(daemon->namebuff, "%d\n", (int) getpid());
+
+	  /* Explanation: Some installations of dnsmasq (eg Debian/Ubuntu) locate the pid-file
+	     in a directory which is writable by the non-privileged user that dnsmasq runs as. This
+	     allows the daemon to delete the file as part of its shutdown. This is a security hole to the 
+	     extent that an attacker running as the unprivileged  user could replace the pidfile with a 
+	     symlink, and have the target of that symlink overwritten as root next time dnsmasq starts. 
+
+	     The folowing code first deletes any existing file, and then opens it with the O_EXCL flag,
+	     ensuring that the open() fails should there be any existing file (because the unlink() failed, 
+	     or an attacker exploited the race between unlink() and open()). This ensures that no symlink
+	     attack can succeed. 
+
+	     Any compromise of the non-privileged user still theoretically allows the pid-file to be
+	     replaced whilst dnsmasq is running. The worst that could allow is that the usual 
+	     "shutdown dnsmasq" shell command could be tricked into stopping any other process.
+
+	     Note that if dnsmasq is started as non-root (eg for testing) it silently ignores 
+	     failure to write the pid-file.
+	  */
+
+	  unlink(daemon->runfile); 
 	  
-	  /* only complain if started as root */
-	  if ((pidfile = fopen(daemon->runfile, "w")))
+	  if ((fd = open(daemon->runfile, O_WRONLY|O_CREAT|O_TRUNC|O_EXCL, S_IWUSR|S_IRUSR|S_IRGRP|S_IROTH)) == -1)
 	    {
-	      fprintf(pidfile, "%d\n", (int) getpid());
-	      fclose(pidfile);
+	      /* only complain if started as root */
+	      if (getuid() == 0)
+		err = 1;
 	    }
-	  else if (getuid() == 0)
+	  else
+	    {
+	      if (!read_write(fd, (unsigned char *)daemon->namebuff, strlen(daemon->namebuff), 0))
+		err = 1;
+	      
+	      while (!err && close(fd) == -1)
+		if (!retry_send())
+		  err = 1;
+	    }
+
+	  if (err)
 	    {
 	      send_event(err_pipe[1], EVENT_PIDFILE, errno, daemon->runfile);
 	      _exit(0);
only in patch2:
unchanged:
--- dnsmasq-2.62.orig/debian/changelog.rej
+++ dnsmasq-2.62/debian/changelog.rej
@@ -0,0 +1,12 @@
+--- debian/changelog
++++ debian/changelog
+@@ -1,3 +1,9 @@
++dnsmasq (2.63-4) unstable; urgency=low
++
++   * Make pid-file creation immune to symlink attacks. (closes: #686484)
++	
++ -- Simon Kelley <simon@thekelleys.org.uk>  Fri, 21 Sep 2012 17:16:34 +0000
++
+ dnsmasq (2.63-3) unstable; urgency=low
+ 
+    * Move adduser dependency to dnsmasq-base. (closes: #686694)

Attachment: signature.asc
Description: Digital signature


Reply to: