Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ppltasks.cpp: Breaking change - New OLE32.dll import prevents process mitigations #3257

Closed
fbrosseau opened this issue Dec 2, 2022 · 4 comments · Fixed by #3607
Closed
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@fbrosseau
Copy link

fbrosseau commented Dec 2, 2022

Hello,

Commit 80ebe87 introduced a new import for ole32.dll in shipped msvcp140.dll.

This is a problem for us, because this import of ole32 cascades into eventually loading user32, immediately at process startup.
This is indirectly a breaking change, because our program needs to enable process mitigations (hardening) on itself, and the system will refuse to enable anti-win32k mitigations if user32 is already loaded in the process.

Workarounds possibly exist for our specific usecase (such as not enabling the mitigation on ourselves and instead having a parent process enable them on a child at the time of CreateProcess), but it remains a breaking change - SetProcessMitigationPolicy is explicitly designed to set mitigations on yourself.

Could ole32 be delay-loaded only when ppltasks are used?

Edit: the commit I linked does mention the intent was that ole32 should be delay-loaded. This does not appear to be the case in msvcp140 14.34.31931.0 (what I currently have on latest VS) and beyond?

This is also DevCom-10217462.

@sylveon
Copy link
Contributor

sylveon commented Dec 2, 2022

This is weird, I'm able to enable DisallowWin32kSystemCalls even when user32.dll is loaded (my process directly links user32.dll and enables said policy), the API does not return an error. Do you have a minimum repro?

@fbrosseau
Copy link
Author

fbrosseau commented Dec 3, 2022

Sure. Please ignore the messy code, it's not easy to avoid using the CRT : )

// cl.exe -nologo -Gm- -GR- -EHa- -Oi -Zi test.cpp -link -nodefaultlib -subsystem:console kernel32.lib -debug:full

#include <Windows.h>

HANDLE g_stdout;

PCWSTR strchr(PCWSTR str, WCHAR find)
{
    while (*str != find)
    {
        ++str;
    }

    return str;
}

void WriteStdout(PCWSTR str)
{
    while (*str)
    {
        DWORD written;
        WriteFile(g_stdout, str, sizeof(WCHAR), &written, nullptr);
        ++str;
    }
}

int mainCRTStartup()
{
    g_stdout = GetStdHandle(STD_OUTPUT_HANDLE);

    PCWSTR libraryToLoad = strchr(GetCommandLineW(), ' ') + 1;
    while (*libraryToLoad == ' ')
    {
        ++libraryToLoad;
    }

    HMODULE mod = LoadLibraryW(libraryToLoad);

    WriteStdout(mod ? L"Loaded ": L"Failed to load ");
    WriteStdout(libraryToLoad);
    WriteStdout(L"\n");

    HMODULE user32 = GetModuleHandleW(L"user32.dll");
    WriteStdout(user32 ? L"User32 loaded\n" : L"User32 NOT loaded\n");

    PROCESS_MITIGATION_SYSTEM_CALL_DISABLE_POLICY SystemCallPolicy{};
    SystemCallPolicy.DisallowWin32kSystemCalls = TRUE;

    if (!SetProcessMitigationPolicy(ProcessSystemCallDisablePolicy,
        &SystemCallPolicy,
        sizeof(SystemCallPolicy)))
    {
        DWORD err = GetLastError();
        WriteStdout(L"FAIL\n");
        return err;
    }

    WriteStdout(L"OK\n");
    return 0;
}
test.exe kernel32.dll -> ok (it's already loaded anyway)
test.exe user32.dll -> not ok
test.exe ole32.dll -> not ok (implies user32.dll)
test.exe msvcp140.dll -> not ok (implies ole32.dll)

If you try an older version (we currently use 14.32.31332.0 for example), it does load and it passes (due to not touching ole32).

As far as the mitigation failing once user32.dll is loaded, I repro this on both latest win11 and win10 (and I'm pretty sure this is by design of the mitigation)

Obviously, the fact ole32 ends up loading user32 is outside of msvcp140's control, but what msvcp140 can do is delay load it, which does not appear to be the case today??

@sylveon
Copy link
Contributor

sylveon commented Dec 3, 2022

Ah, my bad. Looks like my code wasn't actually enabling this, even though I thought I recalled it did. Your code does repro the problem.

For reference, build by doing

cl /c test.cpp
link /NODEFAULTLIB /SUBSYSTEM:CONSOLE test.obj kernel32.lib

I don't see anywhere in the commited code where it would imply delay loading. However, I found this funny outdated comment:
image

@CaseyCarter CaseyCarter added the bug Something isn't working label Dec 6, 2022
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting, and we're leaning towards entirely reverting the change that caused this (#2654). We avoided delay-loading because it introduces significant additional complexity and could cause further problems that we don't fully understand here.

We'll need to double-check that there's a viable workaround for the original customer that needed #2654.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants