wavestone-cdt / EDRSandblast

1.51k stars 278 forks source link

Fix potential buffer overrun in credguard disable #2

Closed JohnLaTwC closed 2 years ago

JohnLaTwC commented 2 years ago

The call to GetModuleFileNameEx passes in sizeof(szModulename) for the size parameter. The documentation for that API says the size parameter is a character count, not a byte count ("The size of the lpFilename buffer, in characters."). Since the code currently passes in a byte count, this opens up the possibility for a stack buffer overrun on UNICODE compilations of this tool where the szModulename buffer will be MAX_PATH characters (2 MAX_PATH bytes), but the size parameter wil indicate it is 2MAX_PATH characters which GetModuleFileNameEx will interpret as a character count and potentially write up to 2*2*MAX_PATH bytes into the buffer. Fix by passing in a character count. You could also use a macro like ARRAYSIZE(szModulename).

    TCHAR szModulename[MAX_PATH];
    for (DWORD i = 0; i < (lpcbNeeded / sizeof(HMODULE)); i++) {
        if (hModulesArray[i] && !GetModuleFileNameEx(hLsass, hModulesArray[i], szModulename, sizeof(szModulename))) {
...        }

[1] Docs for GetModuleFileNameEx are here (https://docs.microsoft.com/en-us/windows/win32/api/psapi/nf-psapi-getmodulefilenameexa)

themaks commented 2 years ago

You are right, thanks a lot for the catch ! I think we used the _countof() macro in the rest of the code: would you mind replacing MAX_PATH by _countof(szModulename) for consistency purposes?

themaks commented 2 years ago

Thanks again for your contribution !