[Cvs-syncmail] patch: security fix

[Zooko <zo...@zo...>]

The shell command invoked for sending mail is:

/bin/mail -s "CVS: %(SUBJECT)s" %(PEOPLE)s 2>&1 > /dev/null

SUBJECT is determined from the file name, version number, and tags.

Are you entirely sure that the file name, version number, and tags, couldn't 
contain a string like:

`rm -rf /`

or:

";cat /etc/passwd

?

I'm not entirely sure of that.  Anyway, with my patch for subject lines derived 
from log entries (in another message), it is *definitely* possible that the 
SUBJECT string could have such stuff.  My fix is to change the command to:

/bin/mail -s 'CVS: %(SUBJECT)s' %(PEOPLE)s 2>&1 > /dev/null

And to run SUBJECT through:

SUBJECT = string.replace(SUBJECT, "'", '"')

My reading of the bash 2.05 man page tells me that this should render the 
contents of SUBJECT inert when used in that command.  I hope the same applies to 
Bourne shell.

This much was already in the "subject lines from log entries" patch that 
I already submitted, but I subsequently went hunting for similar problems and 
appended is a patch which ensures that no other such sneakiness could get in 
to the other commands that syncmail invokes.  As a bonus, it also makes syncmail 
handle filenames with spaces (and all other kinds of weird character, except 
for '\').

Regards,

Zooko

---
                 zooko.com
Security and Distributed Systems Engineering
---

Index: syncmail
===================================================================
RCS file: /cvsroot/mnet/CVSROOT/syncmail,v
retrieving revision 1.14
retrieving revision 1.17
diff -u -r1.14 -r1.17
--- syncmail	17 Mar 2002 14:26:25 -0000	1.14
+++ syncmail	17 Mar 2002 15:17:03 -0000	1.17
@@ -61,11 +61,13 @@
 
 """
 
+# standard Python modules
+import getopt
 import os
-import sys
+import re
 import string
+import sys
 import time
-import getopt
 
 # Notification command
 MAILCMD = "/bin/mail -s 'CVS: %(SUBJECT)s' %(PEOPLE)s 2>&1 > /dev/null"
@@ -90,9 +92,29 @@
 
 
 
+REV_RE=re.compile("^[0-9.]+$")
 def calculate_diff(filespec, contextlines):
     try:
         file, oldrev, newrev = string.split(filespec, ',')
+        if not REV_RE.match(oldrev):
+            raise ValueError
+        if not REV_RE.match(newrev):
+            raise ValueError
+        if string.find(file, '\\') != -1:
+            # I'm sorry, a file name that contains a backslash is just too much.
+            # XXX if someone wants to figure out how to escape the backslashes in a safe way to allow filenames containing backslashes, this is the place to do it.  --Zooko 2002-03-17
+            raise ValueError
+        if string.find(file, "'") != -1:
+            # Those crazy users put single-quotes in their file names!
+            # Now we have to escape everything that is meaningful inside double-quotes.
+            filestr = string.replace(file, '`', '\`')
+            filestr = string.replace(filestr, '"', '\"')
+            filestr = string.replace(filestr, '$', '\$')
+            # and quote it with double-quotes.
+            filestr = '"' + filestr + '"'
+        else:
+            # quote it with single-quotes.
+            filestr = "'" + file + "'"
     except ValueError:
         # No diff to report
         return '***** Bogus filespec: %s' % filespec
@@ -101,7 +123,7 @@
             if os.path.exists(file):
                 fp = open(file)
             else:
-                update_cmd = 'cvs -fn update -r %s -p %s' % (newrev, file)
+                update_cmd = "cvs -fn update -r %s -p %s" % (newrev, filestr)
                 fp = os.popen(update_cmd)
             lines = fp.readlines()
             fp.close()
@@ -125,8 +147,7 @@
             difftype = "-C " + str(contextlines)
         else:
             difftype = "-u"
-        diffcmd = "/usr/bin/cvs -f diff -kk %s --minimal -r %s -r %s '%s'" % (
-            difftype, oldrev, newrev, file)
+        diffcmd = "/usr/bin/cvs -f diff -kk %s --minimal -r %s -r %s %s" % (difftype, oldrev, newrev, filestr)
         fp = os.popen(diffcmd)
         lines = fp.readlines()
         sts = fp.close()