Re: Fw: address@hidden: Bug#538330: groff: pdfroff uses (and documents!) insecure temporary files]

[Keith Marshall]

On Sunday 16 August 2009 23:13:29 Colin Watson wrote:
> (Any particular reason why this isn't CCed to bug-groff?)

Only because Werner forwarded your original post to me privately, as 
an encapsulated message, and KMail's "Reply-to-All" didn't pick it 
up.  I've added it, this time around.

> On Sun, Aug 16, 2009 at 10:43:44PM +0100, Keith Marshall wrote:
> > On Saturday 15 August 2009 09:26:25 Werner LEMBERG wrote:
> > > On Sat, Jul 25, 2009 at 09:30:18AM +0100, Colin Watson wrote:
> > > > See attached report; this is indeed a standard anti-pattern
> > > > resulting in security vulnerabilities. In Debian I'd be
> > > > rather tempted to use 'mktemp -d' to fix this. What do you
> > > > think?
> > >
> > > Nico Golde points out that Openwall have a patch for this. I'm
> > > applying this to the Debian package:
> >
> > Sorry, but I'm not going to accept this as is; it exhibits
> > several unacceptable portability issues:--
>
> I understand; I hope you can also understand that I would rather
> have the Debian security team not hate me, so I applied it anyway
> pending discussion of something better. :-)

Sure.  What you do for Debian and Ubuntu is entirely your choice; I 
have to consider portability to a wider systems population.

> >   -  WRKFILE=${GROFF_TMPDIR=${TMPDIR-${TMP-${TEMP-"."}}}}/pdf$$.tmp
> >   +  MYTMPDIR=${GROFF_TMPDIR-${TMPDIR-${TMP-${TEMP-"/tmp"}}}}
> >
> > I deliberately used current directory rather than /tmp, as
> > ultimate fallback; /tmp doesn't exist by default, on MS-Windows.
>
> I don't think it makes any difference to security, although I do
> think /tmp should be used as a fallback if it exists since
> otherwise you can't use pdfroff when you don't have write
> privileges to your current directory.

Well, you have to have write permission for the formatted output 
stream, and I'd expect most users to be writing that in current 
directory anyway.  Compared to the destructive effect of an 
arbitrary choice of /tmp when it does not exist, ensuring that 
current directory is writeable, or that an appropriate variable
is set in the environment, is surely a minor inconvenience.

As developer, I can be certain that current directory exists; I 
cannot be certain that /tmp does.  To make an arbitrary choice 
of /tmp -- which may be common, but is by no means ubiquitous -- 
only to then have to follow up by checking that it does, in fact, 
exist as a writeable location just seems ugly and redundant.  Any 
user who needs to run pdfroff in a non-writeable current directory 
can always set TMPDIR=/tmp, (or use any of the other environment 
variables having similar effect), to force the use of /tmp.  The 
fallback to current directory only comes into effect when none of 
those environment variables is set; I'd much rather let the user 
make an informed choice which is appropriate for his system, than 
indulge in arbitrary guesswork.

> >   +  WRKDIR="`unset TMPDIR && mktemp -dp "$MYTMPDIR" 
> > groff-pdfroff.XXXXXXXXXX`" || exit
> >
> > `unset' isn't portable; this kills pdfroff stone dead, for any
> > shell which lacks it.
>
> The autoconf documentation has a portability rune for this:
>
>           # "|| exit" suppresses any "Segmentation fault" message.
>           if ( (MAIL=60; unset MAIL) || exit) >/dev/null 2>&1;
> then unset=unset
>           else
>             unset=false
>           fi
>           $unset PS1 || PS1='$ '
>
> (Perhaps 'unset=true' here.)

I was aware of this, thanks, but surely it is better to just avoid 
using `unset' altogether:

  export TMPDIR
  TMPDIR=${GROFF_TMPDIR-${TMPDIR-${TMP-${TEMP-"."}}}}
  GROFF_TMPDIR=`exec 2>${NULLDEV}; mktemp -dt pdfroff-XXXXXXXXXX` ||
      GROFF_TMPDIR=${TMPDIR}

  WRKFILE=${GROFF_TMPDIR}/pdf$$.tmp

> > Even if `unset' succeeds, it will then die when it tries to
> > execute `mktemp' on any system which doesn't support that, (as
> > is the case on MS-Windows with MSYS -- the very system on which
> > pdfroff is most important to me).
>
> Yes, an equivalent of mktemp will need to be provided. Just hoping
> that the path constructed with $$ doesn't exist is not adequate
> though.
>
> Since mkdir(1) is typically atomic and succeeds only if the
> directory did not already exist, you can probably repeatedly try
> directories of different names until mkdir succeeds. I'd still
> recommend using mktemp(1) if it's available, though - it's
> provided on many systems, and it will be much more efficient than
> a mkdir loop.

Well, the snippet above will use `mktemp', if it is available; it 
will fall back to existing behaviour otherwise.  IMO, this should 
suffice.  Any `mkdir' loop will be ugly and grossly inefficient;
rather let the paranoid use a system which supports `mktemp'.

-- 

Regards,
Keith.