Fancy-Viewer 1.2.0.8

Fancy-Viewer 1.2.0.8 Released: https://www.fancy-viewer.com/

Changes:

• updated ImageMagick to some fresh version 7.1.1-40;
• fixed a crash in libmagic (file type detection library);
• fixed issue with old FTP servers support (for those who doesn't understand MLSD, like my TP-LINK router);
• improved security sandbox of host-process for ImageMagick;
• improved performance;
• did some other bugfix;

 

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.

UI Windows Sandboxing

What I love about programming is that system programming can strike you back even if you are writing a simple desktop UI tool. For example, if you want it to be more secure, as I do. (it is Fancy Viewer tool in my case https://www.fancy-viewer.com/)

The tool uses ImageMagick library (plus plugins) which I completely trust, but vulnerabilities happen and it is better to run the parsers in some isolated environment, i.e. sandbox. 

There are some Windows API functions for that:

CreateRestrictedToken

CreateProcessAsUserW

which work as charm:

Except you have to rewrite token's default DACL or the app refuses to start on Windows 7 (or on Windows 10 with "run as administrator").

Like here:

BOOL CreateProcessRestrictedW(LPWSTR lpCommandLine,
    LPSTARTUPINFOW lpStartupInfo,
    BOOL bInheritHandles,
    DWORD dwCreationFlags,
    LPVOID lpEnvironment,
    LPCWSTR lpCurrentDirectory,
    OUT LPPROCESS_INFORMATION lpProcessInformation)
{
    HANDLE hProcessToken = 0, hRestrictedToken = 0;

    if (!OpenProcessToken(GetCurrentProcess(),
        TOKEN_ALL_ACCESS | TOKEN_ASSIGN_PRIMARY | TOKEN_DUPLICATE | TOKEN_QUERY,
        &hProcessToken))
    {
        return FALSE;
    }
    lsvu::HandleGuard processTokenGuard(hProcessToken);

    // get current user sid
    std::vector<char> currentUserSID;
    if (QueryTokenSID_Silent(hProcessToken, &currentUserSID))
    {
        return FALSE;
    }

    // collect other system sids
    std::vector<char> adminSID;
    if (GetWellKnownSid_Silent(WinBuiltinAdministratorsSid, adminSID))
    {
        return FALSE;
    }
    std::vector<char> localSystemSID;
    if (GetWellKnownSid_Silent(WinLocalSystemSid, localSystemSID))
    {
        return FALSE;
    }

    SID_AND_ATTRIBUTES sidToDisable = { adminSID.data(), 0 };

    // create the restricted token
    if (!CreateRestrictedToken(hProcessToken,
        DISABLE_MAX_PRIVILEGE | LUA_TOKEN,
        1, &sidToDisable,
        0, 0,
        0, 0,
        &hRestrictedToken))
    {
        return FALSE;
    }
    lsvu::HandleGuard restrictedTokenGuard(hRestrictedToken);

    // Set the token to low integrity:
    TOKEN_MANDATORY_LABEL tokenLabel = { 0 };
    tokenLabel.Label.Attributes = SE_GROUP_INTEGRITY;
    if (!ConvertStringSidToSidW(L"S-1-16-4096", &tokenLabel.Label.Sid))
    {
        return FALSE;
    }
    {
        LocalGuard sidLabelGuard(tokenLabel.Label.Sid);
        if (!SetTokenInformation(hRestrictedToken,
            TokenIntegrityLevel,
            &tokenLabel,
            sizeof(tokenLabel) + GetLengthSid(tokenLabel.Label.Sid)))
        {
            return FALSE;
        }
    }

    // Create new DACL
    std::vector<char> dacl;
    if (CreateACL_Silent(dacl, currentUserSID, adminSID, localSystemSID))
    {
        return FALSE;
    }
    
    TOKEN_DEFAULT_DACL newDefaulDACL = { (PACL)dacl.data() };
    if (!SetTokenInformation(hRestrictedToken, TokenDefaultDacl, 
                     &newDefaulDACL, sizeof(newDefaulDACL)))
    {
        return FALSE;
    }

    // Create a new process using the restricted token
    BOOL result = CreateProcessAsUserW(hRestrictedToken,
        NULL,
        lpCommandLine,
        NULL,
        NULL,
        bInheritHandles,
        dwCreationFlags,
        (LPVOID)lpEnvironment,
        lpCurrentDirectory,
        lpStartupInfo,
        lpProcessInformation);
    return result;
}  

It isn't a full compilable source, but demonstrates the idia well enough.

[offtop] UI and Guilty Pleasure

Suddenly started a desktop tool for file viewing purposes:

 https://www.fancy-viewer.com/ 

Yeaaah, just another photo viewing tool: Windows/API, ImageMagick-based, see About->Licenses for licenses.

There are couple of reasons why I did it:

- first, I need a hobby and I'm not in the mood for another low level hobby-security-project; had a lot of that stuff on the job;

- then, the process of testing i.e, going through gigabytes of photos on my hard drive calms me very well, which is good for the mental health. Funny thing, UI bugs don't frustrate me at all (I know a lot of people who hate them terribly);

- I use the tool by myself and I just like it this way: I like the ability of reviewing photos without being limited with fixed-sized thumbnails; I also use built-in FTPS client features, Tags and Favorites features, Dark Theme, etc.

It is still a little bit raw: doesn't have proper auto-update system and code signing (I ordered the cert, but the process is slow as hell). UI obviously lacks of RTL support; multi-language support is not that great, it currently just supports only two of languages: English and Ukrainian. 

P.S: Some UI controls were created completely from scratch, and it was a lot of fun with Win32 stuff which I also love (while 70%-80% of my regular job is about Linux/Unix systems currently)

oh my god! they killed LastAccessTime

Сабж случился 9 лет назад вместе с выходом Windows Vista и остался незаметным для широких масс пользователей программистов до сих пор:

http://blogs.technet.com/b/filecab/archive/2006/11/07/disabling-last-access-time-in-windows-vista-to-improve-ntfs-performance.aspx
https://technet.microsoft.com/en-us/library/cc959914.aspx

Итого, из пяти параметров FILE_BASIC_INFORMATION осталось три полезных, своего рода рекорд.

GetLastError and side effects

Представим себе, что у нас в проекте есть такое чудо:

hSomething = ::CreateSomething(...); if (!hSomething) throw std::runtime_error("Cannot create something: " + some_utils::FormatError(GetLastError())); *

_Winnie C++ Colorizer

Какие проблемы могут быть с этим кодом?

Ну, о том, что вообще-то имеет смысл сообщать об ошибке в виде кода ошибки, а не в виде невнятной строки (непонятно на каком языке) речь даже не идет. Представим, что нам очень нужно записать куда-нибудь именно строку, т.е. на месте throw std::runtime_error вполне мог бы быть некий LogLog.

Возвращаемся к коду. Если бы я увидел такой код на review, я посчитал бы его некорректным, аргументируя тем, что компилятор вполне может расставить вызовы функций в следующей последовательности:

call std::basic_string<char>::std::basic_string<char>(const char *) .... call GetLastError call some_utils::FormatError call _CxxThrowException

_Winnie C++ Colorizer

Не трудно представить, к чему это приведет, учитывая, что конструктор строки вполне может вызвать malloc, а значит и HeapAlloc. (Который пренепременно вызовет SetLastError(NO_ERROR) в случае успешного выделения памяти)

Поэтому, я сам всегда старался писать так, чтобы исключить возможные side effects:

hSomething = ::CreateSomething(...); if (!hSomething) { DWORD dwError = GetLastError(); throw std::runtime_error("Cannot create something: " + some_utils::FormatError(dwError)); }

_Winnie C++ Colorizer

....
До тех пор, пока, совершенно случайно, не наткнулся на тот факт, что утверждение

... HeapAlloc. Который пренепременно вызовет SetLastError(NO_ERROR) в случае успешного выделения памяти.

не является истинным.

MSDN утверждает, что HeapAlloc вообще никогда не зовет SetLastError, а HeapReAlloc зовет ее только в случае неудачного выделения памяти. Тестами подтверждается.

Да и вообще, как выяснилось, количество API функций, которые устанавливают SetLastError(NO_ERROR) по поводу и без повода не так уж и велико: сюда входит файловое API, функции работы с ini файлами, с ресурсами, GlobalUnlock/LocalUnlock, и, почему-то, EqualSid(???). Все.

Поэтому, перезатереть значение NtCurrentTeb()->LastErrorValue в нашем случае - некому. Минус одна старая параноидальная привычка (когда-то я таки обжегся на таких сайд эффектах, но код уже и не припомню).