cvs-syncmail-talk — User & developer discussions for the "syncmail" CVS checkin notification tool

Re: [Cvs-syncmail] patch: security fix

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

Whoops.  The security patch was strict on what a legitimate revision number 
looks like.  Here's a patch that fixes that.

Regards,

Zooko

---
                 zooko.com
Security and Distributed Systems Engineering
---
Index: syncmail
===================================================================
RCS file: /cvsroot/mnet/CVSROOT/syncmail,v
retrieving revision 1.17
diff -u -d -r1.17 syncmail
--- syncmail    17 Mar 2002 15:17:03 -0000      1.17
+++ syncmail    17 Mar 2002 15:32:21 -0000
@@ -92,7 +92,7 @@
 
 
 ^L
-REV_RE=re.compile("^[0-9.]+$")
+REV_RE=re.compile("^(NONE|[0-9.]+)$")
 def calculate_diff(filespec, contextlines):
     try:
         file, oldrev, newrev = string.split(filespec, ',')

[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()

[Cvs-syncmail] patch: subject lines from log entries

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

This patch makes the subject lines reflect the log entry and not the file name
and version number.

See the difference between these two mail archives:

http://www.geocrawler.com/archives/3/7495/2001/10/0/
http://www.geocrawler.com/archives/3/7495/2001/11/0/

Regards,

Zooko

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

Index: syncmail
===================================================================
RCS file: /cvsroot/mnet/CVSROOT/syncmail,v
retrieving revision 1.9
retrieving revision 1.14
diff -u -r1.9 -r1.14
--- syncmail	16 Mar 2002 14:25:15 -0000	1.9
+++ syncmail	17 Mar 2002 14:26:25 -0000	1.14
@@ -68,7 +68,7 @@
 import getopt
 
 # Notification command
-MAILCMD = '/bin/mail -s "CVS: %(SUBJECT)s" %(PEOPLE)s 2>&1 > /dev/null'
+MAILCMD = "/bin/mail -s 'CVS: %(SUBJECT)s' %(PEOPLE)s 2>&1 > /dev/null"
 
 # Diff trimming stuff
 DIFF_HEAD_LINES = 20
@@ -142,7 +142,7 @@
 
 
 
-def blast_mail(mailcmd, filestodiff, contextlines):
+def blast_mail(mailcmd, filestodiff, contextlines, logentry):
     # cannot wait for child process or that will cause parent to retain cvs
     # lock for too long.  Urg!
     if not os.fork():
@@ -150,7 +150,7 @@
         # give up the lock you cvs thang!
         time.sleep(2)
         fp = os.popen(mailcmd, 'w')
-        fp.write(sys.stdin.read())
+        fp.write(logentry)
         fp.write('\n')
         # append the diffs if available
         for file in filestodiff:
@@ -160,6 +160,33 @@
         # doesn't matter what code we return, it isn't waited on
         os._exit(0)
 
+
+def abbreviate_logentry_for_subject(logentry):
+    WIDTHLIMIT = 80
+    SUBJECTSIZE = len('Subject: ')
+    LMstr='Log Message:'
+    i = string.find(logentry, LMstr)
+    if i == -1:
+        # Whoops.  Whatever.
+        # the caller will fallback to old-style when it sees `None'
+        return None
+    subject = logentry[i+len(LMstr):]
+    while (len(subject) > 0) and (subject[0] not in string.letters+string.digits):
+        subject = subject[1:] # I have the feeling that there's a nicer way to do this...
+    subject = string.replace(subject, "'", '"')
+    # Now break at the first line break
+    i = string.find(subject, '\n')
+    if i == -1:
+        i = len(subject)
+    j = string.find(subject, '\r')
+    if j == -1:
+        j = len(subject)
+    subject = subject[:min(i,j)]
+
+    if len(subject) < (WIDTHLIMIT - SUBJECTSIZE):
+        return subject
+    else:
+        return subject[:(WIDTHLIMIT - SUBJECTSIZE - 3)] + "..."
 
 
 # scan args for options
@@ -185,13 +212,18 @@
         elif opt == '-u':
             contextlines = 0
 
+    # The log entry is on stdin.  --Zooko 2001-11-21
+    logentry = sys.stdin.read()
+    SUBJECT = abbreviate_logentry_for_subject(logentry)
+    if SUBJECT is None:
+        SUBJECT = args[0]
+
     # What follows is the specification containing the files that were
     # modified.  The argument actually must be split, with the first component
     # containing the directory the checkin is being made in, relative to
     # $CVSROOT, followed by the list of files that are changing.
     if not args:
         usage(1, 'No CVS module specified')
-    SUBJECT = args[0]
     specs = string.split(args[0])
     del args[0]
 
@@ -218,7 +250,8 @@
                 L.append(s)
         specs = L
     print 'Generating notification message...'
-    blast_mail(mailcmd, specs[1:], contextlines)
+
+    blast_mail(mailcmd, specs[1:], contextlines, logentry)
     print 'Generating notification message... done.'