
GraphicsMagick and ImageMagick popen() shell vulnerability via filename - marksamman
http://permalink.gmane.org/gmane.comp.security.oss.general/19669
======
_jomo
Thanks HN, I have now lost my Twitter profile image and can't upload a new
one:

[https://twitter.com/_jom0/status/737081667215601664](https://twitter.com/_jom0/status/737081667215601664)

~~~
keyle
I love that you did that. Never be afraid of breaking things. In this case
it's amusing but imagine you would have done this in a banking application and
you'd probably be arrested for hacking.

~~~
bigiain
[https://twitter.com/ajlobster/status/735240869859753985](https://twitter.com/ajlobster/status/735240869859753985)

:-)

------
voltagex_
The current situation reminds me of OpenSSL after Heartbleed - researchers
start paying lots of attention and finding other issues after the "main" one.
Sad that this is what it takes for a big, old project like ImageMagick to get
some TLC.

~~~
ndj7
There's a crucial difference; OpenSSL was designed to be secure. ImageMagick
was designed to support hundreds of codecs and thousands of operations.

You can't pivot design on 1.25 million lines of code (est.) very quickly after
25 years of feature-first development process.

I'm a bit skeptical it will happen at all; it would be a multi-million dollar
undertaking. Better to describe its design scope and tradeoffs, and get people
to use it properly - sandbox it if it's on the server.

I started Imageflow to solve that problem - a server-focused design with
strict design rules: security is first, followed by visual quality, followed
by performance. We've kept the codebase to just 10kloc and we are 50% feature
complete, but need to get a lot more momentum on Kickstarter to make
completion possible. See [https://www.imageflow.io](https://www.imageflow.io)

------
zerocrates
The particularly troublesome part of this issue (and the previous big one) is
the interaction between all the "power" ImageMagick provides in its filename
parsing and its MVG and SVG decoders.

You'd think that just having applications not use user-provided filenames
during the conversion process would prevent much of what's been disclosed
lately, but no: the MVG decoder exposes all those features to a user that
merely controls the content of the input file.

The mitigations from the last big vulnerability will still work against this,
but people who merely updated to an ImageMagick that fixed the curl command
escaping would be in trouble.

~~~
userbinator
The biggest question I have is why filenames would even need to be treated
specially by ImageMagick anyway. Almost all command-line utilities that are
designed to be used in pipelines have the convention of using stdin/stdout,
and do so without requiring they spawn shells themselves.

~~~
zerocrates
"Why?" can be an interesting question with ImageMagick.

Does it _need_ a prefix you can put on a filename that makes the file
automatically get deleted after it's read? I don't think so, but there it is.

------
wfunction
> The simple solution to the problem is to disable the popen support
> (HAVE_POPEN)

Is it just me or is this the wrong way to tackle this? The question to me is
why the file name being interpreted in some way in the first place, not why
popen is being used on the result of the interpretation.

Also, why are pipes even being allowed in the file name in the first place?
(I'm asking about POSIX/*nix here, not about ImageMagick.)

~~~
AgentME
Linux allows any byte to be a part of a filename except for the null byte and
'/'. It's not the kernel or filesystem's fault if some program like sh treats
some characters like '|' specially.

~~~
lloeki
Interestingly enough, Cocoa on OS X allows '/' but disallows ':', and maps
them back and forth when going down to the Darwin layer, as ':' used to be the
(always invisible in the UI) path separator pre-OS X.

~~~
bigiain
I'm wondering now if you're as old as I am (old enough to have written code
for OS9), or if you're younger but curious about archeological wierdnesses in
modern OSs...

------
0942v8653
Wow. This is really scary. Especially the SVG issue. Is it common to support
xlink:href on servers that use ImageMagick? It seems like that would allow you
to read any readable image on the FS anyway, which is a vulnerability in
itself.

~~~
zerocrates
> It seems like that would allow you to read any readable image on the FS
> anyway, which is a vulnerability in itself.

It does. You can trivially use the same feature to render the contents of a
readable text file into the output image, too.

------
yxhuvud
This is not the first time popen with | has led to security issues. At the
same time it doesn't have any legitimate use cases that I can think of. Is
there any good reason to not patch popen and remove the support for |s
instead?

~~~
MereInterest
It can be useful if you want to offload some of the processing to other
processes.

    
    
        // Open a gzipped file, display progress bar
        snprintf(buf, sizeof(buf), "pv %s | zcat", filename);
        FILE* f = popen(buf, "r");
    
        // Add an additional processing step when reading file
        FILE* f = NULL;
        if(do_extra_step) {
            snprintf(buf, sizeof(buf), "zcat %s | extra_step", filename);
            f = popen(buf, "r");
        } else {
            snprintf(buf, sizeof(buf), "zcat %s", filename);
            f = popen(buf, "r");
        }
    

The first example could be replicated by using zlib and reproducing pv's
progress bar in-app. More work, but nothing especially tricky about it.

For the second example, it would be more difficult, for two reasons. First,
popen() can open either a read or a write pipe, but not both. In order to
write to extra_step's stdin, then read from stdout, you'll need to switch over
to fork/exec. Second, you'll have to handle all the buffering between zcat and
extra_step yourself, rather than getting it for free.

------
0xmohit
Unfortunately, this is not the only one. In fact, there are so many that there
is a dedicate site:

[https://imagetragick.com/](https://imagetragick.com/)

It also contains several PoC.

------
kyledrake
Does this affect non-SVG (and other weird format I've never heard of) images?
(such as jpg, png, bmp, gif)

I'm trying to figure out a way to patch this that doesn't involve recompiling
imagemagick. Like most people I get it from dist, compiling it is kindof a
PITA.

------
Mojah
In case anyone is interested, here's another mirror of the CVE:
[https://marc.ttias.be/oss-
security/2016-05/msg00255.php](https://marc.ttias.be/oss-
security/2016-05/msg00255.php)

------
0x0
So, just like Perl?

~~~
joosters
Only badly-written perl.

It's a bit like claiming that SQL allows for SQL injection attacks.

~~~
0x0
Well, the default open() method do perform pipe execs if you prepend or append
"|", just like here.

[http://docstore.mik.ua/orelly/perl3/prog/ch16_03.htm](http://docstore.mik.ua/orelly/perl3/prog/ch16_03.htm)

~~~
labster
If you're not using two-argument open() in Perl 5, you deserve what you get.

~~~
bigiain
"If you're not using two-argument open() in Perl 5, you deserve what you get."

Pretty sure that's not quite what you meant to say...

[http://modernperlbooks.com/mt/2010/04/three-arg-open-
migrati...](http://modernperlbooks.com/mt/2010/04/three-arg-open-migrating-to-
modern-perl.html)

If you _are_ using two-arg open, or if you're _not_ using three-arg open.

(Or, the more general case: "If you're using Perl you deserve what you get -
if you don't understand all the language features you're using." (And feel
free to substitute whatever language will start the argument in place of Perl
in that claim...)

------
trollian
Who on earth still uses ImageMagick? Security holes in it are old news. Like
15 year old news.

~~~
Bromskloss
What replacement would you suggest?

~~~
mappu
Most basic image manipulation can be performed using only the standard library
packages, if your language includes such (PHP's bundled `libgd2` fork /
Golang's `image` package / Qt QImage+QPixMap ). For Python `pillow` is
popular.

~~~
mschuster91
I believe php uses imagick under the hood, and even if it does not, I'd rather
rely on imagick than on PHP code when accepting exotic formats.

~~~
voltagex_
From a security standpoint, wouldn't it be better to rely on PHP (first)? Are
you more or less likely to get RCE from PHP or ImageMagick? I'd say PHP has
had a lot more eyes on it, security wise.

~~~
cyphar
> From a security standpoint, wouldn't it be better to rely on PHP (first)?

You're joking, right? Popping PHP boxes is script kiddie 101. You might argue
that "bad developers make insecure software" but when you're given a toolbox
with nothing but oddly shaped hammers it's very hard to make a house.

~~~
lillesvin
That's rarely PHP failing but whatever framework/CMS is used. That's like
saying C++ is inherently insecure because there exists a lot of software
written in C++ with security holes.

~~~
cyphar
Did you read the second sentence I wrote? Just because you can write safe code
in a language that tries to make you footgun at every turn, doesn't mean that
language is a good language. Pick a language that doesn't suffer from
crippling security issues (which, sure, you can workaround -- but why use a
language that requires workarounds to get security?).

~~~
ndj7
We started Imageflow ([https://www.imageflow.io](https://www.imageflow.io) )
in C to design the C ABI for interop with all the host languages, but we're
swapping out components for Rust every time it's possible/practical.

