Solar Designer groff tempfile audit

[Ingo Schwarze]

Hi,

i just committed the follwing to the OpenBSD ports tree,
where i'm maintaining a port of groff-1.21.

This posting includes a commit message and a patch at the end.

Even though the audit by Solar Designer did not, unless i missed
something, reveal any vulnerabilities that persist in groff-1.21,
i think many of his patches should be considered from the point
of view of a proactive stance on security:  Don't just fix what
is proven exploitable, rather fix everything that is buggy or even
potentially dangerous.

In particular, in pdfroff.sh, using mktemp -d for security reasons,
then falling back to something else (which is very unsafe) in case
of failure looks dangerous.  At the very least, you should use many
XXXXXXXXXXs and not just the $$ when falling back.
So i fixed that one even in our port.

All the rest is not worth patches in a port, but should, i think,
all the same be considered upstream.

Yours,
  Ingo


----- Forwarded message from <address@hidden> -----

From: Ingo Schwarze <address@hidden>
Date: Thu, 23 Jun 2011 06:14:51 -0600 (MDT)
To: address@hidden
Subject: CVS: cvs.openbsd.org: ports

CVSROOT:        /cvs
Module name:    ports
Changes by:     address@hidden  2011/06/23 06:14:51

Modified files:
        textproc/groff : Makefile 
Added files:
        textproc/groff/patches: patch-contrib_pdfmark_pdfroff_sh 

Log message:
Following http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=538330,
Solar Designer did an audit of temp file handling in groff-1.20.
He found and fixed *lots* of ugliness, but most does not look
exploitable and some was already improved in groff-1.21.

This is my own fix for the only one that, with a huge amount of extra
paranoia, might be worth patching.  To mount an exploit, the attacker
would need to trick root into setting an unusable TMPDIR (or similar)
variable in the environment such that mktemp -d fails, then convince
root to run pdfroff (*you* don't run that as root, do you?), then
handle a race condition to find the PID and predict the temp file
name to mount a symlink attack.

"I think we should still go for the fix" jasper@

----- End forwarded message -----


--- contrib/pdfmark/pdfroff.sh.orig     Fri Dec 31 08:33:09 2010
+++ contrib/pdfmark/pdfroff.sh  Wed Jun 22 01:37:47 2011
@@ -153,11 +153,10 @@
   else
   #
   # Creation of a private temporary directory was unsuccessful;
-  # fall back to user nominated directory, (using current directory
-  # as default), and schedule removal of only the temporary files.
-  #
-    GROFF_TMPDIR=${TMPDIR}
-    trap "rm -f ${GROFF_TMPDIR}/pdf$$.*" 0
+  # DO NOT fall back to user nominated directory,
+  # because that would allow symlink attacks.
+    echo >&2 "$CMD: mktemp(1) -d failure"
+    exit 1
   fi
   #
   # In the case of abnormal termination events, we force an exit