Hi, * Solar Designer <so...@openwall.com> [2009-08-14 18:54]: > Thank you for forwarding this info in here - it has helped. > > Have you notified Werner LEMBERG, the upstream maintainer?
No I just dumped this on the list so far and hope that our maintainer did this. However I CCed groff@gnu.org now. > > I have some comments on "the first bug" and on groff's temporary file > handling in general: > > On Sun, Aug 09, 2009 at 03:48:17PM +0200, Nico Golde wrote: > > First one: > > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=538330 > > pdfroff tool of groff is creating files in a insecure manner > > in the /tmp directory. > > There's a mitigating factor: pdfroff will use $TMPDIR (or one of three > other env vars that it checks) if set. On Owl, pam_mktemp sets $TMPDIR > to point to the user's private temporary file directory upon PAM session > setup, which is invoked on both remote and console logins, and on cron > job invocations. Yes I saw this, unfortunately those variables aren't set on Debian. > I found your trivial patch (pdfroff.sh.diff, 389 bytes) a bit unfinished: > > - WRKFILE=${GROFF_TMPDIR=${TMPDIR-${TMP-${TEMP-"."}}}}/pdf$$.tmp > + WRKFILE=${GROFF_TMPDIR=$(mktemp -t -d groffXXXXXX)}/pdf$$.tmp True this is quick and dirty and can't be considered complete. > - no check for a possible mktemp error; > - will leave the directory around upon pdfroff termination; True, I didn't do this as groff removes the files via a trap before the patch and I missed that GROFF_TMPDIR is not pointing to a directory that was created for this usage. > - changes semantics, but does not update documentation (the man page is > explicit on what temporary file directories pdfroff uses). Ok, I didn't read the manpage while writing the patch. > I included more elaborate changes to pdfroff (both the script and the > man page) in groff-1.20.1-owl-tmp.diff available here: > > http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/groff/ The patch looks great and way more clean than my quick-and-dirty one. Thanks! I forwarded this to the Debian bug report. [not stripping this because of the new CC] Can someone please assign CVE ids to these issues? > Other issues I found (and patched in groff-1.20.1-owl-tmp.diff) are: > > Bugs in eqn2graph.sh, grap2graph.sh, pic2graph.sh (duplicated code) > where these scripts would fail to properly handle the case when all > attempts to create a temporary directory fail. In that case, they would > proceed to use the last-tried pathname. Hopefully, this would just fail > in some weird way, but it is also possible that the directory would in > fact exist and be under someone else's control. Since this was the last > thing I dealt with today and I was already tired, I opted for the > simplest fix, which was to reset the variable at the end of each loop > iteration. At a later time, I may drop all non-mktemp code from these > scripts, like I did in other places. > > gendef.sh and doc/fixinfo.sh created files in $TMPDIR (if defined) or in > the current directory without any precautions. Not an issue if $TMPDIR > is safe or is not set, and these are only used during groff build, but > it is an issue if someone sets $TMPDIR to /tmp or to another shared > directory. I've patched these to use mktemp(1). gendef.sh is on my > list of files accessed during groff build (which means that it could > have been run but this is not certain); fixinfo.sh is not (so it was > certainly not run during my build but this does not mean much for other > builds). > > contrib/gdiffmk/tests/runtests.in created files in /tmp in the worst way > possible. I've patched it to use mktemp(1), although maybe it should > just use the current directory since it does so in other places anyway. > It is on my list of files accessed during groff build. > > contrib/groffer/perl/groffer.pl and contrib/groffer/perl/roff2.pl used > only four X'es in filename patterns. The reasonable minimum is > considered 6; we typically use 10. Patched to use 10. > > doc/groff.texinfo (source) and doc/groff.info-2 (pre-compiled) contained > an example for invoking an external program with ".sy", saving its > output to a file under /tmp. Patched to use a file in the current > directory instead, even though this is not perfect. > > Finally, we're patching configure and config.guess to use the > mktemp-only code just to be fail-close, even though I understand the > rationale behind the original portable code. I am also adding a #error > comment to src/roff/groff/pipeline.c, just to save a few minutes next > time I or someone else looks at this code. > > That's all for the patch. However, my grep's also identified potential > issues in install-sh and contrib/groffer/shell/*. Since these files > were not used during our groff package builds, I opted to remove rather > than patch them. So I added: > > # Remove/disable unused files with temporary file handling issues in them to > # make sure that these are in fact unused. > rm -r contrib/groffer/shell > echo -e '#!/bin/sh\nexit 1' > install-sh > > to our groff.spec, and I also had to add > groff-1.20.1-owl-groffer-Makefile.diff to allow groff to build without > the contrib/groffer/shell/* files (their presence was checked by make > even though they were not read). On systems with Perl, groff uses the > version of groffer written in Perl instead of this shell version, but > there's some risk of it falling back to the shell version if Perl can't > be run for whatever reason. > > A related detail is that we force the configure script to detect the > presence of mkstemp(3), we don't take chances: > > export ac_cv_func_mkstemp=yes \ > %configure > > In the case of groff, the fallback code (when mkstemp(3) is not present) > is not that bad, though. > > Speaking of my grep's, I used: > > fgrep -rl /tmp . > fgrep -rl mktemp . > fgrep -rl tmpnam . > fgrep -rl tempnam . > fgrep -rl TMPDIR . > > Then I briefly reviewed the files identified in this way. Of course, I > could have missed something, yet this would have caught the pdfroff > issue. We updated Owl-current to this version of groff with pdfroff in > it just 8 days ago, and we were not as careful right away... > > Alexander Cheers Nico -- Nico Golde - http://www.ngolde.de - n...@jabber.ccc.de - GPG: 0xA0A0AAAA For security reasons, all text in this mail is double-rot13 encrypted.
pgpU5NNpNoymw.pgp
Description: PGP signature
