14 ms·

So judging by this commit[1] and this line[2] I reckon you could somehow escape the "wget" command (assuming that's what it invokes here[3]). The following char

• x0x0 11y ago
  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...
  • __david__ 11y ago
    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.
    • deleted 11y ago
      [deleted]
    • ashitlerferad 11y ago
      libpipeline is a good replacement for the more complex uses of system(): http://libpipeline.nongnu.org/ http://libpipeline.nongnu.org/
      • networked 11y ago
        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 http://jim.tcl.tk/fossil/doc/trunk/Tcl_shipped.html#_exec [2] http://c2.com/cgi/wiki?AlternateHardAndSoftLayers http://c2.com/cgi/wiki?AlternateHardAndSoftLayers [3] https://tcl.wiki/2074 https://tcl.wiki/2074
• moloch 11y ago
  `` can invoke subshells
  • orf 11y ago
    Thar she blows. I've updated my comment, seems like that's the RCE.
• zerocrates 11y ago
  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/58a2ce1638c7cca4db9c9a70ea841b34c8932490 https://github.com/ImageMagick/ImageMagick/commit/58a2ce1638...
  • foone 11y ago
    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.
    • zerocrates 11y ago
      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.
      • zerocrates 11y ago
        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).
• peterwwillis 11y ago
  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.
  • RachelF 11y ago
    Perhaps a silly question, but if address space layout randomization (ASLR) is enabled, should it no protect against buffer overflows?
    • peterwwillis 11y ago
      It protects against trivial ones, but advanced buffer overflows can work around ASLR and other protections.
• alkonaut 11y ago
  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?
  • astrodust 11y ago
    It's both a library and a series of command-line tools.
    • alkonaut 11y ago
      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.
  • ryangittins 11y ago
    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.
    • alkonaut 11y ago
      > 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.
• ashitlerferad 11y ago
  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-society/ http://bonedaddy.net/pabs3/log/2014/02/17/pid-preservation-s... http://libpipeline.nongnu.org/ http://libpipeline.nongnu.org/ I expect there may also be option injection vulnerabilities in the code. http://www.defensecode.com/public/DefenseCode_Unix_WildCards_Gone_Wild.txt http://www.defensecode.com/public/DefenseCode_Unix_WildCards...
  • ntauthority 11y ago
    Both don't exist on non-POSIX systems, like NT. ImageMagick supports those natively, however.
    • ashitlerferad 11y ago
      ISTR NT has other mechanisms for that.
    • sitkack 11y ago
      Security is a spectrum, beware of falling into a https://en.wikipedia.org/wiki/False_dilemma https://en.wikipedia.org/wiki/False_dilemma
    • poizan42 11y ago
      So? Use CreateProcess on NT then Edit: Actually it already does: https://github.com/ImageMagick/ImageMagick/blob/master/MagickCore/nt-base.c#L2243 https://github.com/ImageMagick/ImageMagick/blob/master/Magic...
      • ashitlerferad 11y ago
        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?
        • 11y ago
• dalias 11y ago
  There is no excuse for shell escaping bugs like this. 100% safe and reliable shell escaping is trivial: s/'/'\''/g s/^/'/ s/$/'/
  • nitrogen 11y ago
    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.
    • tomsmeding 11y ago
      The shell doesn't interpret double-backslash in a single-quoted string. Therefore, this will indeed do.
    • Karellen 11y ago
      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 http://linux.die.net/man/1/dash
      • nitrogen 11y ago
        I knew about the concatenation rules, but I was missing the fact that backslash does nothing in single-quoted strings. $ echo 'hi\' hi\