[poppler] pdftohtml lets you run random shell commands

[Ihar `Philips` Filipau]

On 4/19/12, suzuki toshiya <mpsuzuki at hiroshima-u.ac.jp> wrote:
> Ahh, indeed, I think you say that "pdftohtml is dangerous as find or xargs
> commands that can invoke any other command, but these are security issue?".
> Umm.

Something like this.

"Injecting stuff is bad" is clearly speak of security types. So the
follow up logical question: do we have any security concerns here?
Answer is "no." Thus injecting stuff is not security problem, but
rather usability problem of passing complex arguments. Usability
question: does one really need to put anything special into the device
string? Answer is again "no," - it is a plain string where "gs"
doesn't even expect spaces.

Thus to me, in the end, such toying with -dev parameter is nothing
major and is of "doctor it hurts! don't do it then" type of problem.

I doubt my voice should count (because I work with a rather limited
subset of PDFs) but I too do not mind removal of the stuff since I
never used it. But as a software developer, neither I see any risk of
it being left as it is.

wbr.

> Ihar `Philips` Filipau wrote:
>> On 4/19/12, Albert Astals Cid <aacid at kde.org> wrote:
>>> You can do
>>>  pdftohtml -c -dev 'jpeg /dev/null;cat /etc/passwd;#'
>>> /path/to/some/pdf/fil
>>> and voila, you'll get your /etc/passwd printed on screen
>>>
>>> Definitely not nice.
>>>
>>> This is because we are using plain system() to run the gs command and
>>> it's
>>> easy to inject stuff there
>>>
>>
>> My 0.02€
>>
>> So what? User already can print /etc/passwd.
>>
>> The problem of system() call is only relevant when the command is
>> installed suid-root(*). And pretty much all systems install only
>> required minimum of commands as suid-root. ((*) Or user convinces
>> admin to run something as root in his own terminal - but you can't
>> really do anything against idiot admins.)
>>
>>> The real solution is moving to a fork+exec solution (path attached).
>>
>> You use execvp() - that doesn't improve anything: the 'p' letter in
>> execvp() stands for "path resolution", meaning that user can still add
>> its own wrapper for "gs" command, adjust the $PATH and circumvent
>> whatever you intended to prevent with the patch.
>>
>> IOW, the exec*p*() functions are as insecure as the system() - unless
>> of course you use absolute path for "gs" (what I gather would cause
>> troubles for portability).
>>
>> Simpler /fix/ would be to make the programs not runnable by root -
>> `geteuid() != 0`. That would also cover the case of idiot admins. :)
>>
>> Otherwise, in the patch, if one would replace the kinky va_list stuff
>> with a GooList() of `char *` (iow, pack the command line onto the list
>> (and add an accessor for the GooList::data)) the change would easily
>> come off as a clean up. ;)
>>
>>> The problem with that is that we loose support for platforms with
>>> system()
>>> and without fork+exec (Windows).
>>
>> The problem doesn't exist on Windows, since it doesn't have anything
>> like suid. It's either user or Administrator(**). And if user can run
>> a program as an administrator, then all bets are off. (The same case
>> as an idiot *nix admin.)
>>
>> (**) Win Vista/7 have something similar, but UAC would bark at it. So
>> it doesn't change the parity.
>> _______________________________________________
>> poppler mailing list
>> poppler at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/poppler
>
>


-- 
Don't walk behind me, I may not lead.
Don't walk in front of me, I may not follow.
Just walk beside me and be my friend.
    -- Albert Camus (attributed to)