ImageMagick / ImageMagick6

🧙‍♂️ ImageMagick 6
https://legacy.imagemagick.org
Other
194 stars 80 forks source link

`SetMagickSecurityPolicy` always fails #279

Open Lastique opened 8 months ago

Lastique commented 8 months ago

ImageMagick version

6.9.12-97

Operating system

Linux

Operating system, version and so on

Kubuntu 22.04

Description

Calling SetMagickSecurityPolicy with a user-specified security policy seem to always return MagickFalse and not apply the provided policy. Instead, ImageMagick always loads the system-wide policy.xml from /etc.

Steps to Reproduce

Here's a short reproducer:

#include <cstdio>
#include <cstddef>
#include <string>
#include <memory>
#include <iostream>
#include <filesystem>
#include <Magick++.h>

int main()
{
    Magick::InitializeMagick(nullptr);

    std::filesystem::path im_policy_path = std::filesystem::absolute("im_policy.xml");
    std::cout << "Loading policy: " << im_policy_path << std::endl;

    const auto size = std::filesystem::file_size(im_policy_path);
    std::unique_ptr< char[] > buf(new char[size + 1u]);

    std::FILE* file = std::fopen(im_policy_path.c_str(), "r");
    if (!file)
    {
        std::cout << "Failed to open policy file: " << im_policy_path << std::endl;
        return 1;
    }

    std::size_t read_size = std::fread(buf.get(), 1u, static_cast< std::size_t >(size), file);
    buf[read_size] = '\0';

    //std::cout << buf.get() << std::endl;

    MagickCore::ExceptionInfo* exception = MagickCore::AcquireExceptionInfo();
    if (!exception)
    {
        std::cout << "Failed to allocate exception info" << std::endl;
        return 1;
    }

    MagickCore::MagickBooleanType res = MagickCore::SetMagickSecurityPolicy(buf.get(), exception);
    if (res == MagickCore::MagickFalse)
    {
        std::cout << "Failed to set security policy" << std::endl;
        Magick::throwException(exception);
        return 1;
    }

    MagickCore::ListPolicyInfo(stdout, exception);

    return 0;
}

Compile with:

g++ -std=c++17 -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 -I ~/include/ImageMagick-6 -I ~/include/x86_64-linux-gnu/ImageMagick-6 -L ~/lib/x86_64-linux-gnu -o im_policy im_policy.cpp -lMagick++-6.Q16 -lMagickWand-6.Q16 -lMagickCore-6.Q16

im_policy.xml in the current directory exists and contains the following:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE policymap [
  <!ELEMENT policymap (policy)*>
  <!ATTLIST policymap xmlns CDATA #FIXED ''>
  <!ELEMENT policy EMPTY>
  <!ATTLIST policy xmlns CDATA #FIXED '' domain NMTOKEN #REQUIRED
    name NMTOKEN #IMPLIED pattern CDATA #IMPLIED rights NMTOKEN #IMPLIED
    stealth NMTOKEN #IMPLIED value CDATA #IMPLIED>
]>

<policymap>
  <policy domain="path" rights="none" pattern="/home/*"/>
</policymap>

Running the program always ends with "Failed to set security policy" and no exceptions.

Note: I verified that the im_policy.xml is indeed loaded correctly by uncommenting the debug output.

Am I doing something wrong? If so, how one should use SetMagickSecurityPolicy to programmatically set a custom security policy?

Images

No response

Lastique commented 8 months ago

I believe, this is caused by IsPolicyCacheInstantiated call here. If the cache is not instantiated, it will call AcquirePolicyCache with the default PolicyFilename argument, which will load the default "/etc/ImageMagick-6/policy.xml" file (which is what I'm trying to avoid). Then, here, the check will fail as the policy is already loaded.

urban-warrior commented 8 months ago

What your configure script command-line? We tried your example with 6.9.12-98 and it works for us:

> ./im_policy 
Loading policy: "/home/cristy/ImageMagick-6.9.12-98/im_policy.xml"

Path: /usr/local/etc/ImageMagick-6/policy.xml
  Policy: Undefined
    rights: None 

Path: [built-in]
  Policy: Undefined
    rights: None 

Path: [user-policy]
  Policy: Path
    rights: None 
    pattern: /home/*

Try xmllint im_policy.xml to ensure your policy validates.

Lastique commented 8 months ago

What your configure script command-line?

I'm building Debian packages for Kubuntu and Debian. Configure command line is:

cd debian/build-quantum-q16 && ../../configure --build=x86_64-linux-gnu --prefix=/usr --includedir=\${prefix}/include --mandir=\${prefix}/share/man --infodir=\${prefix}/share/info --sysconfdir=/etc --localstatedir=/var --disable-option-checking --disable-silent-rules --libdir=\${prefix}/lib/x86_64-linux-gnu --libexecdir=\${prefix}/lib/x86_64-linux-gnu --runstatedir=/run --disable-maintainer-mode --disable-dependency-tracking --enable-reproducible-build --prefix=/usr --libdir=/usr/lib/x86_64-linux-gnu --docdir=/usr/share/doc/imagemagick-6-common/html "--with-extra-doc-dir= (on debian system you may install the imagemagick-6 package)" --sysconfdir=/etc --with-includearch-dir=/usr/include/x86_64-linux-gnu/ --mandir=/usr/share/man --infodir=/usr/share/info --with-security-policy=limited --with-modules --with-fontconfig --with-freetype --with-pango --with-gs-font-dir=/usr/share/fonts/type1/gsfonts --with-magick-plus-plus --with-djvu --with-heic --with-png --with-tiff --with-jpeg --with-openjp2 --with-webp --with-wmf --without-gvc --enable-shared --without-dps --without-fpx --with-perl --with-perl-options=INSTALLDIRS=vendor --x-includes=/usr/include/X11 --x-libraries=/usr/lib/X11 --without-rsvg --cache-file=../../config.cache --without-gcc-arch --disable-silent-rules --with-quantum-depth=16 --enable-hdri=no

Here's the full build log:

build.log

Try xmllint im_policy.xml to ensure your policy validates.

The policy is exactly as I posted in the issue, xmllint doesn't show any errors.

I've also added -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 to compile command line to get rid of warnings, but that didn't change the behavior - SetMagickSecurityPolicy still fails. (I've updated the issue.)

I can see in your output that the system-wide policy (in your case, in /usr/local/etc) is still loaded before the user's policy from im_policy.xml. I would expect the user's policy to be loaded instead of the system-wide. Though, of course, the first issue to resolve is why SetMagickSecurityPolicy fails in my case but not in yours.

urban-warrior commented 8 months ago

Due to security considerations, when you initially install ImageMagick, it imposes a specific policy that may be more restrictive than your preferred settings. For instance, if the system policy sets 8GB of memory, SetMagickSecurityPolicy() sets it to 6GB. That works, however, If you attempt to set it to 10GB, ImageMagick will instead limit it to 8GB. To override this policy entirely, you must install ImageMagick using the --disable-installed configuration script command-line.

We will conduct a thorough investigation to understand why the SetMagickSecurityPolicy() function is failing in your case. So far, we have been unable to replicate the issues you're experiencing.

Lastique commented 8 months ago

I think, the problem doesn't reproduce on your end because your system-wide policy is empty:

Path: /usr/local/etc/ImageMagick-6/policy.xml
  Policy: Undefined
    rights: None 

In my case, the system-wide policy is not empty (it's the default policy from Ubuntu packages), it looks like this, verbatim:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE policymap [
  <!ELEMENT policymap (policy)*>
  <!ATTLIST policymap xmlns CDATA #FIXED ''>
  <!ELEMENT policy EMPTY>
  <!ATTLIST policy xmlns CDATA #FIXED '' domain NMTOKEN #REQUIRED
    name NMTOKEN #IMPLIED pattern CDATA #IMPLIED rights NMTOKEN #IMPLIED
    stealth NMTOKEN #IMPLIED value CDATA #IMPLIED>
]>
<!--
  Configure ImageMagick policies.

  Domains include system, delegate, coder, filter, path, or resource.

  Rights include none, read, write, execute and all.  Use | to combine them,
  for example: "read | write" to permit read from, or write to, a path.

  Use a glob expression as a pattern.

  Suppose we do not want users to process MPEG video images:

    <policy domain="delegate" rights="none" pattern="mpeg:decode" />

  Here we do not want users reading images from HTTP:

    <policy domain="coder" rights="none" pattern="HTTP" />

  The /repository file system is restricted to read only.  We use a glob
  expression to match all paths that start with /repository:

    <policy domain="path" rights="read" pattern="/repository/*" />

  Lets prevent users from executing any image filters:

    <policy domain="filter" rights="none" pattern="*" />

  Any large image is cached to disk rather than memory:

    <policy domain="resource" name="area" value="1GP"/>

  Use the default system font unless overwridden by the application:

    <policy domain="system" name="font" value="/usr/share/fonts/favorite.ttf"/>

  Define arguments for the memory, map, area, width, height and disk resources
  with SI prefixes (.e.g 100MB).  In addition, resource policies are maximums
  for each instance of ImageMagick (e.g. policy memory limit 1GB, -limit 2GB
  exceeds policy maximum so memory limit is 1GB).

  Rules are processed in order.  Here we want to restrict ImageMagick to only
  read or write a small subset of proven web-safe image types:

    <policy domain="delegate" rights="none" pattern="*" />
    <policy domain="filter" rights="none" pattern="*" />
    <policy domain="coder" rights="none" pattern="*" />
    <policy domain="coder" rights="read|write" pattern="{GIF,JPEG,PNG,WEBP}" />
-->
<policymap>
  <!-- <policy domain="resource" name="temporary-path" value="/tmp"/> -->
  <policy domain="resource" name="memory" value="256MiB"/>
  <policy domain="resource" name="map" value="512MiB"/>
  <policy domain="resource" name="width" value="16KP"/>
  <policy domain="resource" name="height" value="16KP"/>
  <!-- <policy domain="resource" name="list-length" value="128"/> -->
  <policy domain="resource" name="area" value="128MP"/>
  <policy domain="resource" name="disk" value="1GiB"/>
  <!-- <policy domain="resource" name="file" value="768"/> -->
  <!-- <policy domain="resource" name="thread" value="4"/> -->
  <!-- <policy domain="resource" name="throttle" value="0"/> -->
  <!-- <policy domain="resource" name="time" value="3600"/> -->
  <!-- <policy domain="coder" rights="none" pattern="MVG" /> -->
  <!-- <policy domain="module" rights="none" pattern="{PS,PDF,XPS}" /> -->
  <!-- <policy domain="path" rights="none" pattern="@*" /> -->
  <!-- <policy domain="cache" name="memory-map" value="anonymous"/> -->
  <!-- <policy domain="cache" name="synchronize" value="True"/> -->
  <!-- <policy domain="cache" name="shared-secret" value="passphrase" stealth="true"/>
  <!-- <policy domain="system" name="max-memory-request" value="256MiB"/> -->
  <!-- <policy domain="system" name="shred" value="2"/> -->
  <!-- <policy domain="system" name="precision" value="6"/> -->
  <!-- <policy domain="system" name="font" value="/path/to/font.ttf"/> -->
  <!-- <policy domain="system" name="pixel-cache-memory" value="anonymous"/> -->
  <!-- <policy domain="system" name="shred" value="2"/> -->
  <!-- <policy domain="system" name="precision" value="6"/> -->
  <!-- not needed due to the need to use explicitly by mvg: -->
  <!-- <policy domain="delegate" rights="none" pattern="MVG" /> -->
  <!-- use curl -->
  <policy domain="delegate" rights="none" pattern="URL" />
  <policy domain="delegate" rights="none" pattern="HTTPS" />
  <policy domain="delegate" rights="none" pattern="HTTP" />
  <!-- in order to avoid to get image with password text -->
  <policy domain="path" rights="none" pattern="@*"/>
  <!-- disable ghostscript format types -->
  <policy domain="coder" rights="none" pattern="PS" />
  <policy domain="coder" rights="none" pattern="PS2" />
  <policy domain="coder" rights="none" pattern="PS3" />
  <policy domain="coder" rights="none" pattern="EPS" />
  <policy domain="coder" rights="none" pattern="PDF" />
  <policy domain="coder" rights="none" pattern="XPS" />
</policymap>

Now, if I comment the last return 1; line in my test posted in the original post, so that ListPolicyInfo is called before exit, I get this output:

Loading policy: "/home/lastique/src/tmp/im_policy.xml"
Failed to set security policy

Path: /etc/ImageMagick-6/policy.xml
  Policy: Resource
    name: disk
    value: 1GiB
  Policy: Resource
    name: map
    value: 512MiB
  Policy: Resource
    name: memory
    value: 256MiB
  Policy: Resource
    name: area
    value: 128MP
  Policy: Resource
    name: height
    value: 16KP
  Policy: Resource
    name: width
    value: 16KP
  Policy: Delegate
    rights: None 
    pattern: URL
  Policy: Delegate
    rights: None 
    pattern: HTTPS
  Policy: Delegate
    rights: None 
    pattern: HTTP
  Policy: Path
    rights: None 
    pattern: @*
  Policy: Coder
    rights: None 
    pattern: PS
  Policy: Coder
    rights: None 
    pattern: PS2
  Policy: Coder
    rights: None 
    pattern: PS3
  Policy: Coder
    rights: None 
    pattern: EPS
  Policy: Coder
    rights: None 
    pattern: PDF
  Policy: Coder
    rights: None 
    pattern: XPS

Path: [built-in]
  Policy: Undefined
    rights: None 

Notice that the first policy type is "Resource".

Now, I've debugged the code with test output and in the debugger, and as I said in an earlier comment, SetMagickSecurityPolicy fails at this point:

https://github.com/ImageMagick/ImageMagick6/blob/9808c038346231af525259b1e39fa40a87a3d274/magick/policy.c#L1159

It fails because p->domain is equal to 5 (i.e. ResourcePolicyDomain).

This is why I'm thinking the SetMagickSecurityPolicy method expects that the system policy must not be loaded. But this is impossible to achieve because it will load the system policy in the first call to IsPolicyCacheInstantiated.

Otherwise, if SetMagickSecurityPolicy is supposed to work with the system policy loaded, then the check I pointed to is incorrect.

urban-warrior commented 8 months ago

Thank you for reporting the issue. We have successfully reproduced it and are actively working on a patch to resolve it. You can expect this patch to be merged into the main GIT branch, later today. As part of our commitment to quality, this fix will also be included in the upcoming beta releases of ImageMagick by tomorrow. Your patience and feedback are greatly appreciated.

Any code related to security mandates a second look. We'll review/test and will need a few days to confirm no additional patches are required.