From: Mark Wooding <mdw@distorted.org.uk>
Date: Fri, 14 Apr 2006 20:18:47 +0000 (+0100)
Subject: qmail-valid-addresses security: scan read dot-qmail files as owner
X-Git-Tag: 1.03-6~3
X-Git-Url: https://git.distorted.org.uk/~mdw/qmail/commitdiff_plain/19de3a724957f431fcee39ab79d158e5d279e188?hp=897b03dfcf776b99815eaddde4d58479f05ba166

qmail-valid-addresses security: scan read dot-qmail files as owner

Usually, qmail-valid-addresses is run as root, and it scans for and
reads all the dot-qmail files as root.  This is bad: if a user creates
(say) a symlink to a read-sensitive device with the right name, then
root will open and read the device, causing it to do weird things.
There are also other problems to do with leaking the existence of files
in directories unreadable to the user in question.  Even if we were to
check with lstat(2) before reading the file, there's an unavoidable race
between the lstat(2) and the open(2), during which a malicious user
could switch in a link.

The new implementation has qmail-valid-addresses switching effective uid
before scanning that user's home directory.  Since all the file I/O is
done as the user who (presumably) owns the files, this can't do any evil
things that user couldn't have done on his own.

This change also fixes a bug which failed to scan dot-qmail files in
subdirectories.
---

diff --git a/qmail-valid-addresses b/qmail-valid-addresses
index d43cf85..8a36f58 100644
--- a/qmail-valid-addresses
+++ b/qmail-valid-addresses
@@ -43,34 +43,61 @@ del udb
 map = {}
 def addlocal(u, p, l, forcep = False):
   l = 'L' + l
+  p = os.path.join(u.home, p)
   if not os.path.exists(p):
     if forcep:
       map[l] = '+'
     return
-  f = open(p)
-  top = f.readline()
-  f.close()
-  if len(top) > 1 and top[0:2] == '#!':
+  qm = file(os.path.join(u.home, p)).readline()
+  if len(qm) > 1 and qm[0:2] == '#!':
     map[l] = '-'
-  elif len(top) > 1 and top[0:2] == '#?':
-    name = u.name
-    if name[-1] == '-':
-      name = name[:-1]
+  elif len(qm) > 1 and qm[0:2] == '#?':
+    name = u.user
     map[l] = '?' + name
   else:
     map[l] = '+'
+
+def allfiles(pre, dir):
+  for f in os.listdir(dir):
+    ff = os.path.join(dir, f)
+    if os.path.isdir(ff):
+      for sub in allfiles(f, ff):
+        yield sub
+    elif os.path.isfile(ff):
+      yield f
+    
+def qmfiles(pre, dir):
+  for f in os.listdir(dir):
+    if not f.startswith(pre):
+      continue
+    ff = os.path.join(dir, f)
+    if os.path.isdir(ff):
+      for sub in allfiles(f, ff):
+        yield sub
+    elif os.path.isfile(ff):
+      yield f
+
 for k in sort(umap.keys()):
+  ouid = os.geteuid()
   u = umap[k]
-  qm = '.qmail' + u.dash + u.pre
-  qmlen = len(qm)
-  if u.wild:
-    for p in os.listdir(u.home):
-      if not p.startswith(qm):
-        continue
-      ext = p[qmlen:]
-      addlocal(u, os.path.join(u.home, p), u.name + ext)
-  else:
-    addlocal(u, os.path.join(u.home, qm), u.name, u.dash == '')
+  try:
+    try:
+      os.seteuid(u.uid)
+    except OSError:
+      pass
+    qm = '.qmail' + u.dash + u.pre
+    qmlen = len(qm)
+    if u.wild:
+      try:
+        for p in qmfiles(qm, u.home):
+          ext = p[qmlen:]
+          addlocal(u, p, u.name + ext)
+      except IOError:
+        pass
+    else:
+      addlocal(u, qm, u.name, u.dash == '')
+  finally:
+    os.seteuid(ouid)
 
 me = open('control/me').readline()