Wednesday, November 23, 2022

ImageMagick sandboxing issue

And you know what? Of course I introduced a bug while implementing sandboxing feature for Fancy Viewer: the support of some raw images was broken from 1.0.2.10 till 1.0.2.13, because of Image Magick requires the use of a filesystem to load these raw images (actually their codecs just doesn't support Blobs).

The root cause of the bug is that Low Integrity application obviously can't access any general-purpose temporary folder; there are some folders that Image Magick knows about, and FOLDERID_LocalAppDataLow is not in the list:

 // ImageMagick-Windows\ImageMagick\MagickCore\resource.c
MagickExport MagickBooleanType GetPathTemplate(char *path)
{
............. (void) FormatLocaleString(path,MagickPathExtent,"magick-" MagickPathTemplate);
  exception=AcquireExceptionInfo();
  directory=(char *) GetImageRegistry(StringRegistryType,"temporary-path",
    exception);
  exception=DestroyExceptionInfo(exception);
  if (directory == (char *) NULL)
    directory=GetEnvironmentValue("MAGICK_TEMPORARY_PATH");
  if (directory == (char *) NULL)
    directory=GetEnvironmentValue("MAGICK_TMPDIR");
  if (directory == (char *) NULL)
    directory=GetEnvironmentValue("TMPDIR");
#if defined(MAGICKCORE_WINDOWS_SUPPORT) || defined(__OS2__) || defined(__CYGWIN__)
  if (directory == (char *) NULL)
    directory=GetEnvironmentValue("TMP");
  if (directory == (char *) NULL)
    directory=GetEnvironmentValue("TEMP");
#endif
#if defined(__VMS)
  if (directory == (char *) NULL)
    directory=GetEnvironmentValue("MTMPDIR");
#endif
#if defined(P_tmpdir)
  if (directory == (char *) NULL)
    directory=ConstantString(P_tmpdir);
#endif
.............
#endif
  return(MagickTrue);
} 

So, I set MAGICK_TEMPORARY_PATH environment variable pointing to some sub-directory of LocalLow folder (acquired by SHGetKnownFolderPath(..FOLDERID_LocalAppDataLow,,), and decided that the fix is done, a trivial one.

Except I found that nothing has changed.
That's why:
1) I set environment variable with SetEnvironmentVariable WinAPI function
2) ImageMagick uses getenv CRT function, which is part of UCRT on Windows

And Windows UCRT keeps own copy of all process environment variables without any synchronization with process environment block: it just copies all the variables while initialization and then uses a separate data structure that clearly resembles Unix'es "environ", except it is properly synchronized and has a longer name:

 // Windows Kits\10\Source\10.0.10240.0\ucrt\env\getenv.cpp


// These functions search the environment for a variable with the given name.
// If such a variable is found, a pointer to its value is returned.  Otherwise,
// nullptr is returned.  Note that if the environment is access and manipulated
// from multiple threads, this function cannot be safely used:  the returned
// pointer may not be valid when the function returns.
template <typename Character>
static Character* __cdecl common_getenv_nolock(Character const* const name) throw()
{
    typedef __crt_char_traits<Character> traits;
    
    Character** const environment = traits::get_or_create_environment_nolock();
    if (environment == nullptr || name == nullptr)
        return nullptr;

    size_t const name_length = traits::tcslen(name);

    for (Character** current = environment; *current; ++current)
    {
        if (traits::tcslen(*current) <= name_length)
            continue;

        if (*(*current + name_length) != '=')
            continue;

        if (traits::tcsnicoll(*current, name, name_length) != 0)
            continue;

        // Internal consistency check:  The environment string should never use
        // a bigger buffer than _MAX_ENV.  See also the SetEnvironmentVariable
        // SDK function.
        _ASSERTE(traits::tcsnlen(*current + name_length + 1, _MAX_ENV) < _MAX_ENV);

        return *current + name_length + 1;
    }

    return nullptr;
}

 So,

 _putenv_s("MAGICK_TEMPORARY_PATH", fvMagickAppDataPath.c_str());

did the trick.

No comments: