Re: [groff] [PATCH] Avoid Perl's unsafe "<>" operator

[Colin Watson]

Thanks for the review.

On Fri, Mar 01, 2019 at 04:41:36PM +0000, Deri wrote:
> On Thursday, 28 February 2019 19:42:45 GMT Colin Watson wrote:
> > On Thu, Jan 24, 2019 at 02:34:35PM +0000, Colin Watson wrote:
> > > The "<>" operator is implemented using the two-argument form of "open",
> > > which interprets magic such as pipe characters, allowing execution of
> > > arbitrary commands which is unlikely to be expected.  Perl >= 5.22 has a
> > > "<<>>" operator which avoids this, but also forbids the use of "-" to
> > > mean the standard input, which is a facility that the affected groff
> > > programs document.
> > 
> > [...]
> > 
> > Has anyone had a chance to review this patch (also in
> > https://savannah.gnu.org/bugs/?55557, after Deri's suggestion)?  Should
> > I just go ahead and commit it?
> > 
> > I'm going to upload this patch to Debian unstable shortly in the cause
> > of getting release-critical bug fixes in ahead of our upcoming full
> > freeze, but it would be better to get it into upstream as well.
> 
> Hi Colin,
> 
> There appear to be a lot of extra changes in the patch which are not to do 
> with what we are trying to fix.

No, despite appearances there are no unrelated changes here.  I
explained what's going on in detail in the patch's commit message:
because my change involves introducing an extra level of indentation, it
looks like there's a lot going on when you look at the raw patch, but
the commit message explains the actual effective changes, and if you
apply it to a local branch and use something like "git show -b" you
should see something that's easier to review.

> There may also be a problem with the gropdf patch. One aspect of using "<>" 
> is 
> that if there are multiple files on the command line an eof is not signalled 
> between the files, i.e. after reading the last line of the first file the 
> next 
> read will be the first line of the next file. This may not have an impact but 
> the read in the LoadAhead subroutine may be done on a file which is at eof, 
> rather than the first line of the next file. I admit this may not cause an 
> issue in normal operation but is a change in behaviour.

Hm, OK.  Could you suggest some example input that I can use to provoke
this?  gropdf is the only groff program that splits up the input reading
in such a way as to have this kind of problem, but I'd be happy to try
to work out a fix given a test case.

> I prefer the first solution you suggested, upon which my code was based, 
> because there will be no change of behaviour. I have been unable to find a 
> way 
> of defeating this protection method to make "<>" safe. Do you know of a way 
> to 
> circumvent it?

I don't have a specific way to circumvent it, and it may well be correct
in most cases.  It's true that it's an easier change.  However, I think
the phrasing of your question puts the burden of proof in the wrong
place.  The problem at hand is that argument handling is more magical
than expected.  Can you *prove* (not just by absence of evidence) that
the approach of pre-escaping @ARGV effectively disables the dangerous
parts of <>'s magic in the fact of arbitrary malicious arguments,
ensuring that each argument is only ever interpreted as a file of the
exact same name, or standard input in the case of "-"?  Is such a proof
likely to be robust against future innocent changes to the code by
people who don't remember this thread?  Perl's "open" documentation even
notes for the "< $file\0" trick that "this may not work on some bizarre
filesystems", which suggests to me that such a robust proof isn't
possible.

What we have here is a particularly magical (in the sense often used by
the Perl documentation) language facility with an assortment of special
behaviours, and we're trying to come up with ways to add extra
characters to its input that suppress that magic.  I argue that - if
we're trying to construct a secure system, which I hope we are - this is
fundamentally the wrong approach.  It's instead much better to use
language facilities that don't have these magical behaviours in the
first place.  This is true even if it takes a little extra effort to
come up with something that exactly matches the subset of the magical
behaviours that we actually want, because then at least our uses of
those behaviours will be explicit and we'll be able to understand them.

An analogy: by now it's fairly well-understood that Perl programmers
should avoid "system $string" (which passes $string to "sh -c") and
prefer "system @list" (which doesn't).  The argument is the same: to
make "system $string" safe, you have to make sure that you've escaped
all the things that the shell is going to unescape.  Of course it's
possible to do this, and I've done that sort of thing myself when the
situation called for it, but it's always better not to have to do that
extra layer of escaping in the first place, because it's error-prone and
it's hard to prove that you got it right.

-- 
Colin Watson                                       address@hidden