SharkCagey / SharkCage

1 stars 3 forks source link

fixes issues found by static analysis tools #94

Closed DonatJR closed 6 years ago

DonatJR commented 6 years ago

@bencikpeter There were some issues found in the tokenLib as well, here are all issues which I fixed in this PR: tokenlib1 tokenlib2 tokenlib4a tokenlib4b

There were also two issues I did not yet fix (as I wasn't sure how to proceed): tokenlib3 not fixed tokenlib5 not fixed

As you can see the handles returned from OpenProcess are not closed if hasSeCreateTokenPrivilege or processIsLocalSystem throw an exception. I was not sure if you did not catch those on purpose. Any suggestions on the easiest way to deal with these issues? Do you want to do it yourself? (Maybe they are not even relevant anymore after #91?)

bencikpeter commented 6 years ago

@DonatJR I´ll have a look at those issues and push directly into this PR if I may 😉

DonatJR commented 6 years ago

@bencikpeter Sure, go ahead

bencikpeter commented 6 years ago

@DonatJR could you please send me a full report? (I am not a HTWG student anymore, so no VPN 😞). I don´t quite understand what exactly the error is trying to say... I don´t throw any exeptions manually with hasPrilivege functions and WinAPI is C-oriented and does not throw as well.. that leads me to believe that only exceptions that can occur are runtime (like allocation error) but in that case, resoruce leaks should be reported here as well, since if constructor throws, destructor is not called and memory leak happens... but Coverity ignores it so it might mean something else? 🤔


tokenTemplate::tokenTemplate(HANDLE &userToken) {

    //load internal NtCreateToken function
    HMODULE hModule = LoadLibrary(L"ntdll.dll");
    NtCreateToken = (NT_CREATE_TOKEN)GetProcAddress(hModule, "NtCreateToken");

    //parse token
    DWORD bufferSize = 0;
    GetTokenInformation(userToken, TokenType, NULL, 0, &bufferSize);
    SetLastError(0);
    GetTokenInformation(userToken, TokenType, (LPVOID)&tokenType, bufferSize, &bufferSize);
    if (GetLastError() != 0)
    {
        throw TokenParsingException();
    }

    bufferSize = 0;
    GetTokenInformation(userToken, TokenUser, NULL, 0, &bufferSize);
    SetLastError(0);
    tokenUser = (PTOKEN_USER) new BYTE[bufferSize];
    GetTokenInformation(userToken, TokenUser, (LPVOID)tokenUser, bufferSize, &bufferSize);
    if (GetLastError() != 0)
    {
        throw TokenParsingException();
    }
bencikpeter commented 6 years ago

Since probable nature of exceptions will be runtime and I believe should not be handled directly inside the function, to address the resource leak, this should suffice:

    for (auto const& processPid : processes)
    {
        HANDLE processHandle;
        if ((processHandle = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, processPid)) == NULL) continue;
        try
        {
            if (processIsLocalSystem(processHandle))
            {
                return processHandle;
            }
        }
        catch (const std::exception&)
        {
            CloseHandle(processHandle);
            throw;
        }
        CloseHandle(processHandle);
    }
DonatJR commented 6 years ago

@bencikpeter Unfortunately, I don't know how to get a pdf report from the website based static analysis tool these errors are from, sorry. :/

The exception probably stems from a call to std::cout with an integer-value. Your solution to free'ing the handle (and stopping the resource leak) looks good to me, I would have handled it the same way.