Hacker News Reader

Explain to me like I'm 5 years old: why is an Image utility invoking shell code and other applications?

Shouldn't this type of utility rather be a library if it's to be used from other applications such as web apps?

It's both a library and a series of command-line tools.

So is git, nuget etc. Either something is just a library, or it's just a command line tool, or it's a command line tool that uses a library (i.e. the command line tool is a simple wrapper that turns command line arguments into function call parameters basically).

I have never heard of a program where the command line program is at the heart, and a library is wrapped around it. I can't even imagine how such code would be structured.

It's really a command-line tool at its heart. The library is essentially just a wrapper around this command-line tool.

You could feasibly do the same stuff in an interpreted language, but (I suspect) it would be substantially slower than directly running those compiled command-line tools underlying it.

> The library is essentially just a wrapper around this command-line tool.

That sounds bass ackwards if you ask me. A command line tool is usually a thin wrapper around a library, for a reason.

I realize it might seem crazy to duplicate functionality from gs into your library, but then the tool should link against those code bases as libraries instead of calling some gs executable (which of course in turn is just a wrapper around a gs library and so on).

Making a library call to re-scale an image or convert from formatA to formatB shouldn't be a shell execute call to begin with, and it shouldn't be making any shell exec calls of its own.

> That sounds bass ackwards if you ask me. A command line tool is usually a thin wrapper around a library, for a reason.

It's historically grown. The tool came first, the library was tacked on later.

And yes, it's a mess. That's why the much saner GraphicsMagick was forked off it… in 2002. People just never migrated.

GraphicsMagick is pretty lacking in terms of features compared to ImageMagick though, even to this day. I tried using it in a project that had just slightly unconventional image processing requirements and GM just didn't have the capabilities to handle it, whereas ImageMagick did.

It is a bit backwards, yes. As other users pointed out, this is for historical reasons.

That being said, I wouldn't be surprised if porting those things over from compiled languages to be a part of the library would prove to be substantially slower. For high-volume applications, it would very possibly make the library totally unusable.

I agree that it's a backwards way of doing things in a perfect world, but the perfect is the enemy of the good. Sometimes you've gotta make compromises for reality's sake.

> That being said, I wouldn't be surprised if porting those things over from compiled languages to be a part of the library would prove to be substantially slower.

I must have missed something, I thought ImageMagick was a collection of C libraries and C programs, which in turn occasionally called other libraries and programs (such as gs).

What I meant was that a graphics utility should be a c library that exports functions like

    scale_image(source_image, target_image, scale)

to be used instead of running a command line utility

    convert image.jpg -resize 0.5

That can't ever be slower, but some times likely faster (e.g. to recode an image before inserting as a blob into a db or sending over a netwoek you wouldn't even write it to disk).

ImageMagick shells out to "delegates" to convert to/from many of the formats it supports: reading in a PDF results in a call to "gs", reading in a .docx results in a call to "soffice --headless", etc.

As for the wisdom of this approach...

This must expose an absolutely massive attack surface. Time to disable all these "delegates".

Because we can never just let documents simply be documents.

ImageMagick shouldn't have been using /bin/sh or system() in the first place. Instead it should use fork+exec or libpipeline.

http://bonedaddy.net/pabs3/log/2014/02/17/pid-preservation-s... http://libpipeline.nongnu.org/

I expect there may also be option injection vulnerabilities in the code.

http://www.defensecode.com/public/DefenseCode_Unix_WildCards...

Both don't exist on non-POSIX systems, like NT. ImageMagick supports those natively, however.

ISTR NT has other mechanisms for that.

Security is a spectrum, beware of falling into a https://en.wikipedia.org/wiki/False_dilemma

So? Use CreateProcess on NT then

Edit: Actually it already does: https://github.com/ImageMagick/ImageMagick/blob/master/Magic...

It seems like CreateProcess uses the one-long-string for the command rather than the POSIX model of one string per argument. Is there any way to run a process on Windows using one string per argument?

Actually, it's (normally) two strings, one for the program image filename and one for the command tail.

* https://msdn.microsoft.com/en-gb/library/windows/desktop/ms6...

A single command tail is the Win32 model at base, as it was the DOS API model before it. (The OS/2 model was multiple command tails, although in practice most programs read no more than just one.) exec*() implementations are layered on top of this, and the argv[] notion is a shared fiction maintained by the runtime libraries of C and C++ implementations.

I'm sure there's buffer overflows in ImageMagick - probably lots of them - but yeah, you definitely don't need them. ImageMagick has been so buggy for so long, i'm pretty sure an accidentally corrupted image of a kitten sitting on a dog's head would gain root, dump your database and mail it to full-disclosure.

Perhaps a silly question, but if address space layout randomization (ASLR) is enabled, should it no protect against buffer overflows?

It protects against trivial ones, but advanced buffer overflows can work around ASLR and other protections.

That's where I'm a little confused. Some of the changes (like the ones you've called out here) look pretty clearly like they're designed to counteract maliciously-constructed filenames, but the PR-type page talks instead about testing magic numbers as the way to avoid the issue.

This one [1] seems to deal with ImageMagick's feature that reads the contents of a file into the command line: "The special character '@' at the start of a filename, means replace the filename, with contents of the given file. That is you can read a file containing a list of files!" Again though, that's a filename-based problem, not something you'd use magic number checking to defend against.

Edit: I guess with the "@filename"-based one, you could defend on the basis that the payload will be a text file of filenames, but that seems rickety at best.

1: https://github.com/ImageMagick/ImageMagick/commit/58a2ce1638...

I think they mean use the magic numbers so you can limit to common file formats like jpeg/png/gif/bmp/tiff/etc instead of just dumping everything to imagemagick, which has the side effect of allowing "weirder" things like MVG/MSL which are imagemagick-specific macro languages which let you do things like wget a remote URL.

I get the general idea of doing that, and it makes sense, but it doesn't seem to necessarily match up with what's in ImageMagick's commit history or in their forum post... but would make sense with using the "weird" formats as the initial payload, I suppose.

In particular, ImageMagick accepting MSL directly into convert seems like an extremely straightforward exploit path, so much so that it actually seems unlikely. Their documentation makes it seem like it's designed to use a separate command "conjure," but... some combination of factors is at play here, anyway.

So, given the details now, it seems that the reason the IM commits don't seem to match up is that they didn't really squarely address the problem (in my understanding).

There is no excuse for shell escaping bugs like this. 100% safe and reliable shell escaping is trivial:

s/'/'\''/g

s/^/'/

s/$/'/

Can you provide a demonstration for why this is adequate? It seems like it might still possible to get single quotes in the input to be interpreted by the shell by putting a backslash in the input.

The shell doesn't interpret double-backslash in a single-quoted string. Therefore, this will indeed do.

See the man page for a POSIX-compliant shell, like dash[0], and find the section on single-quoted strings. For example:

> Enclosing characters in single quotes preserves the literal meaning of all the characters (except single quotes, making it impossible to put single-quotes in a single-quoted string).

Note also the following examples, with double-quoted strings:

  $ echo a b c
  a b c
  $ echo "a" "b" "c"
  a b c
  $ echo "a""b""c"
  abc
  $ echo "a" b "c"
  a b c
  $ echo "a"b"c"
  abc

The same concatenation rules apply to single-quoted strings. So, by putting single quotes at the beginning and end of a string, you only need to worry about single quotes within the string. You can "escape" those with '\'', where the first single quote terminates the preceding single-quoted string, the backslash+single-quote pair is a literal unquoted/escaped single quote in the shell, and the final single quote begins single-quoting again for the rest of the string. The three parts are then dequoted and concatenated together back into your original string by the shell.

http://linux.die.net/man/1/dash

I knew about the concatenation rules, but I was missing the fact that backslash does nothing in single-quoted strings.

  $ echo 'hi\'
  hi\

`` can invoke subshells

Thar she blows. I've updated my comment, seems like that's the RCE.

wait, why wouldn't you want your image processing library to have the ability to run random processes / shell commands? Oof.

In the meantime, maybe a DONT_RUN_COMMANDS ifdef around every call to fork/system/exec is merited...

fork() and exec() are fine. As long as you aren't running arbitrary binaries, it's pretty hard to mess up (as far as RCE goes). system(), however, is a RCE foot-cannon just waiting to happen. Don't ever use system() unless every argument is static. And even then think long and hard.

libpipeline is a good replacement for the more complex uses of system():

http://libpipeline.nongnu.org/

Thanks for linking to libpipeline; I didn't know about it. The API looks good. However, since it is licensed under the GNU GPL (the initial version was based on code in GNU troff) non-copyleft libraries like ImageMagick are unlikely to adopt it.

If you are looking for a small, standalone, non-copyleft alternative, the closest I can think of is the [exec] command in the Jim Tcl [1] embedded scripting language.

Here is an example (without the error checks you'd have in production code):

  #include "jim.h"

  int main(int argc, char const *argv[])
  {
      Jim_Interp *interp;
      int error;
      Jim_Obj *cmd;

      interp = Jim_CreateInterp();
      Jim_RegisterCoreCommands(interp);
      Jim_InitStaticExtensions(interp);

      // The input redirect below does *not* invoke the POSIX shell. It is handled by Jim Tcl itself.
      cmd = Jim_NewListObj(interp, NULL, 0);
      Jim_ListAppendElement(interp, cmd, Jim_NewStringObj(interp, "exec", -1));
      Jim_ListAppendElement(interp, cmd, Jim_NewStringObj(interp, "awk", -1));
      Jim_ListAppendElement(interp, cmd, Jim_NewStringObj(interp, "1", -1));
      Jim_ListAppendElement(interp, cmd, Jim_NewStringObj(interp, "<", -1));
      Jim_ListAppendElement(interp, cmd, Jim_NewStringObj(interp, "/etc/passwd", -1));

      error = Jim_EvalObj(interp, cmd);
      if (error != JIM_ERR) {
          printf("%s\n", Jim_String(Jim_GetResult(interp)));
      }

      Jim_FreeInterp(interp);
      return error;
  }

While I am a fan of the language and of the "hard and soft layers" approach in general [2], it is a commitment: it requires you to embed the language runtime in your program and learn the basics of the scripting language itself and its C API. The upside is that Jim Tcl's [exec] works on Windows, too (and you get other goodies with Jim like a fast, high quality implementation of strings and hash maps).

Alternatively, you can use the "big" Tcl [3] as a C library. It's larger but more mature and is available in every major Linux distribution.

[1] http://jim.tcl.tk/fossil/doc/trunk/Tcl_shipped.html#_exec

[2] http://c2.com/cgi/wiki?AlternateHardAndSoftLayers

[3] https://tcl.wiki/2074

To elaborate on the above a bit, for me the choice between Jim Tcl and the "big" Tcl 8.x comes down to choosing between vendoring the dependencies and embedding the runtime (Jim Tcl) vs. using the distribution libraries and extending the runtime (Tcl 8).

Either way you get a very fine C library that prevents you from succumbing to Greenspun's tenth rule, so I can only recommend it for most C programs and libraries of sufficient size and complexity.

Jim Tcl is available in the distros; at least Debian, Ubuntu, OpenBSD, FreeBSD, NetBSD, openSUSE, Source Mage, Fedora and Gentoo have it.

Oh yes, it is pretty widely available. What I meant was that it was smaller and easier to vendor: for example, you can produce an SQLite-style amalgamation file for it.

Embedding other languages in C seems like a path worse than system(). You are better off just writing in another language completely.

In my opinion Tcl really straddles the line between a language and a library. I've heard it favorably compared to GLib. When used from C it feels a lot more like a library than other scripting languages do (Tcl 8 even more so than Jim due to a greater C API surface).

I agree with you on writing in another language but presumably you wouldn't be considering libpipeline anyway unless you had to write C. My default approach when I need C to access some API is to write an extension to an interpreter from which to access it. I leave embedding an interpreter in C for when you can't do that.