On Mon, Oct 25, 2004 at 04:13:54PM -0400, Joey Hess wrote:
> Package: groff
> Version: 1.18.1.1-1
> Severity: serious
> Tags: security
> 
> CAN-2004-0969 reported that groffer used temporary files in an
> explitable manner. This version of groff seems to be vulnerable. A patch
> is here:
> http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=136313

I've backported this patch as follows:

--- groff-1.18.1.1.orig/debian/changelog
+++ groff-1.18.1.1/debian/changelog
@@ -1,3 +1,10 @@
+groff (1.18.1.1-2) unstable; urgency=high
+
+  * [SECURITY] Fix a race condition in groffer leading to a temporary file
+    handling vulnerability (closes: #278265).
+
+ -- Colin Watson <email address hidden>  Tue, 26 Oct 2004 23:52:13 +0100
+
 groff (1.18.1.1-1) unstable; urgency=low
 
   * The "Death! Ride, ride to ruin and the world's ending!" release.
--- groff-1.18.1.1.orig/contrib/groffer/groffer.sh
+++ groff-1.18.1.1/contrib/groffer/groffer.sh
@@ -3228,17 +3228,12 @@
   do
     if is_not_empty "$d"; then
       if obj d is_dir && obj d is_writable; then
-        _TMP_DIR="${d}/${_PROGRAM_NAME}${_PROCESS_ID}";
-        if obj _TMP_DIR is_dir; then
-	  rm -f "${_TMP_DIR}"/*;
+        _TMP_DIR="$(mktemp -d "${d}/${_PROGRAM_NAME}.XXXXXX")"
+        if test $? = 0; then
           break;
         else
-          mkdir "${_TMP_DIR}";
-          if obj _TMP_DIR is_not_dir; then
-            _TMP_DIR='';
-	    continue;
-          fi;
-          break;
+          _TMP_DIR='';
+	  continue;
   	fi;
       fi;
       if obj _TMP_DIR is_not_writable; then

Werner, Bernd, I'm not sure if you've been informed of this, but the
current groff CVS still seems to be vulnerable. The referenced Trustix
advisory doesn't go into detail, but there are two problems that I see:

  * groffer accepts a temporary directory that already exists and simply
    removes its contents. This is very unwise, especially since it does
    not check the permissions of that directory to make sure that nobody
    else has write access to it. The usual secure approach is only to
    accept a temporary directory that you have just created, so that you
    can be sure of its permissions. Trying to check is too fragile, and
    unnecessary.

  * groffer does not check the exit code of mkdir. It should do so,
    otherwise even if the above hole is plugged an attacker can create a
    world-writable directory between the is_dir check and the mkdir;
    since process IDs are predictable on most systems, this race
    condition is easy to win.

Following the patch in Red Hat's Bugzilla, I've simply used 'mktemp -d'
to fix this, because I know that's correct; however, it isn't portable,
so I suspect the patch above would not be accepted into groffer
upstream. Removing the code that accepts an existing temporary directory
and removes its contents, and then checking the exit code of mkdir,
ought to be sufficient; but I haven't tried this, and somebody should
audit it.

Even checking mkdir will still allow a denial-of-service attack, so this
would seem like a very good application for a helper written in C or C++
if you're unwilling to use mktemp when it's available.

Thanks,

-- 
Colin Watson                                       [<email address hidden>]