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

Fix Size type in IntegerOverflow #39

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Fix Size type in IntegerOverflow #39

merged 1 commit into from
Aug 13, 2020

Conversation

takubokudori
Copy link
Contributor

SIZE_T is defined as ULONG_PTR.
ULONG_PTR is defined as unsigned long on x86, but as unsigned __int64 on x64.
Also InputBufferLength is defined as ULONG(unsigned long) on x86 and x64.

So when InputBufferLength is copied to Size in a driver built on x64, an implicit type conversion occurs and it becomes unsigned __int64 type, so even if InputBufferLength is passed 0xFFFFFFFF, it is still an integer overflow will not occur.

This pull request is to fix the issue.

@hacksysteam
Copy link
Owner

Hi @takubokudori

Thank you so much for the report.

SIZE_T is defined as ULONG_PTR. ULONG_PTR is defined as unsigned long on x86, but as unsigned __int64 on x64. Also InputBufferLength is defined as ULONG(unsigned long) on x86 and x64.

Correct

So when InputBufferLength is copied to Size in a driver built on x64, an implicit type conversion occurs and it becomes unsigned __int64 type, so even if InputBufferLength is passed 0xFFFFFFFF, it is still an integer overflow will not occur.

This is also correct. However, what will happen when we pass 0xFFFFFFFFFFFFFFFF as the InputBufferLength? The overflow still will happen and this was how the challenge was designed. Yeah, I need to update the comments to reflect x64 as well.

What do you think? Do let me your thoughts.

@takubokudori
Copy link
Contributor Author

Thank you for your reply.

The type of nInBufferSize of DeviceIoControl (and NtDeviceIoControl) is also defined as DWORD(unsigned long).
So even if you pass 0xFFFFFFFFFFFFFFFF to nInBufferSize, a cast conversion from unsigned long long to unsigned long occurs, and eventually 0xFFFFFFFF will be passed to IrpSp->Parameters.DeviceIoControl.InputBufferLength (this is also ULONG, so it can store up to 0xFFFFFFFF), so BSOD will not occur.

I confirmed that the following program (and HEVDExploit.exe with 0xFFFFFFFFFFFFFFFF) didn’t cause BSOD before applying this patch and that BSOD occurred after this patch:

#include <stdio.h>
#include <windows.h>

#define HACKSYS_EVD_IOCTL_INTEGER_OVERFLOW CTL_CODE(FILE_DEVICE_UNKNOWN, 0x809, METHOD_NEITHER, FILE_ANY_ACCESS)

int main(void)
{
    HANDLE hFile = CreateFile(L"\\\\.\\HackSysExtremeVulnerableDriver",
        GENERIC_READ | GENERIC_WRITE,
        FILE_SHARE_READ | FILE_SHARE_WRITE,
        NULL,
        OPEN_EXISTING,
        FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OVERLAPPED,
        NULL);
    if (hFile == INVALID_HANDLE_VALUE)
    {
        printf("Failed to CreateFile\n");
        return 1;
    }
    char in_buf[1000] = { 0 };
    DWORD a;
    const unsigned long long x = 0xFFFFFFFFFFFFFFFF;
    DeviceIoControl(hFile,
        HACKSYS_EVD_IOCTL_INTEGER_OVERFLOW,
        (LPVOID)in_buf,
        x,
        NULL,
        0,
        &a,
        NULL);
    return 0;
}

@hacksysteam
Copy link
Owner

@takubokudori yes, I verified and you are correct. Bummer, I didn't consider that InputBufferLength is DWORD.

Thank you so much for the verification and explanation.

Very nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants