14

Since the latest Windows 10 1809 update we can no longer open a USB HID keyboard-like device of ours using CreateFile. We reduced the problem to this minimal example:

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

void bad(const char *msg) {
    DWORD w = GetLastError();
    fprintf(stderr, "bad: %s, GetLastError() == 0x%08x\n", msg, (unsigned)w);
}

int main(void) {
    int i;
    GUID hidGuid;
    HDEVINFO deviceInfoList;
    const size_t DEVICE_DETAILS_SIZE = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA) + MAX_PATH;
    SP_DEVICE_INTERFACE_DETAIL_DATA *deviceDetails = alloca(DEVICE_DETAILS_SIZE);
    deviceDetails->cbSize = sizeof(*deviceDetails);

    HidD_GetHidGuid(&hidGuid);
    deviceInfoList = SetupDiGetClassDevs(&hidGuid, NULL, NULL,
                                         DIGCF_PRESENT | DIGCF_INTERFACEDEVICE);
    if(deviceInfoList == INVALID_HANDLE_VALUE) {
        bad("SetupDiGetClassDevs");
        return 1;
    }

    for (i = 0; ; ++i) {
        SP_DEVICE_INTERFACE_DATA deviceInfo;
        DWORD size = DEVICE_DETAILS_SIZE;
        HIDD_ATTRIBUTES deviceAttributes;
        HANDLE hDev = INVALID_HANDLE_VALUE;

        fprintf(stderr, "Trying device %d\n", i);
        deviceInfo.cbSize = sizeof(deviceInfo);
        if (!SetupDiEnumDeviceInterfaces(deviceInfoList, 0, &hidGuid, i,
                                         &deviceInfo)) {
            if (GetLastError() == ERROR_NO_MORE_ITEMS) {
                break;
            } else {
                bad("SetupDiEnumDeviceInterfaces");
                continue;
            }
        }

        if(!SetupDiGetDeviceInterfaceDetail(deviceInfoList, &deviceInfo,
                                        deviceDetails, size, &size, NULL)) {
            bad("SetupDiGetDeviceInterfaceDetail");
            continue;
        }

        fprintf(stderr, "Opening device %s\n", deviceDetails->DevicePath);
        hDev = CreateFile(deviceDetails->DevicePath, 0,
                          FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
                          OPEN_EXISTING, 0, NULL);
        if(hDev == INVALID_HANDLE_VALUE) {
            bad("CreateFile");
            continue;
        }

        deviceAttributes.Size = sizeof(deviceAttributes);
        if(HidD_GetAttributes(hDev, &deviceAttributes)) {
            fprintf(stderr, "VID = %04x PID = %04x\n", (unsigned)deviceAttributes.VendorID, (unsigned)deviceAttributes.ProductID);
        } else {
            bad("HidD_GetAttributes");
        }
        CloseHandle(hDev);
    }

    SetupDiDestroyDeviceInfoList(deviceInfoList);
    return 0;
}

It enumerates all HID devices, trying to obtain the vendor ID/product ID for each one using CreateFile over the path provided by SetupDiGetDeviceInterfaceDetail and then calling HidD_GetAttributes.

This code runs without problems on previous Windows versions (tested on Windows 7, Windows 10 1709 and 1803, and the original code from which this was extracted works since always from XP onwards), but with the latest update (1809) all keyboard devices (including ours) cannot be opened, as CreateFile fails with access denied (GetLastError() == 5). Running the program as administrator doesn't have any effect.

Comparing the output before and after the update, I noticed that the devices that now cannot be opened gained a trailing \kbd in the device path, i.e. what previously was

\\?\hid#vid_24d6&pid_8000&mi_00#7&294a3305&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}

now is

\\?\hid#vid_24d6&pid_8000&mi_00#7&294a3305&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}\kbd

Is it a bug/new security restriction in the latest Windows 10 version? Was this code always wrong and it worked by chance before? Can this be fixed?


Update

As a desperate attempt, we tried to remove the \kbd from the returned string... and CreateFile now works! So, now we have a workaround, but it would be interesting to understand if that's a bug in SetupDiGetDeviceInterfaceDetail, if it's intentional and if this workaround is actually the correct thing to do.

Matteo Italia
  • 123,740
  • 17
  • 206
  • 299
  • 1
    this is not bug (`kbd` suffix). you got string registered by `IoRegisterDeviceInterface`. faster this is "by design". – RbMm Dec 13 '18 at 13:18
  • 1
    in `KbdHid_Create` (inside *kbdhid.sys*) added check for `KBD` and return error in this case – RbMm Dec 13 '18 at 13:41
  • 1
    also in *hidclass.sys* inside `HidpRegisterDeviceInterface` added this suffix (*ReferenceString*) (`KBD`) - no this code in previous builds. so this by design – RbMm Dec 13 '18 at 13:45
  • @RbMm: thank you for the investigation, do you have an idea of what's the point of this modification, and what are we doing wrong, then? – Matteo Italia Dec 13 '18 at 14:16
  • 1
    i think you nothing do wrong here. you got interface as is registered by `IoRegisterDeviceInterface`. in new version 1809 by some reason on some hid collection suffix `KBD` added. what is reason do that - require much more research – RbMm Dec 13 '18 at 14:22
  • 1
    *But it's strange anyhow, as I would expect a sharing violation (32) for that* - yes, and must be `STATUS_SHARING_VIOLATION` (`c0000043`) look for 3 screenshot : if not empty name - `c0000022` or `c0000043` (in case `KBD`) will be. but only name is never `KBD`. it `\KBD` in your case. due error compare name you got 5 while must got 32 – RbMm Dec 14 '18 at 01:35
  • 1
    i extend answer - really got `STATUS_SHARING_VIOLATION` on `\kbd` device if use relative file open – RbMm Dec 14 '18 at 09:02

3 Answers3

15

I think this is a new security restriction in the latest Windows 10 version.

I looked for the string KBD (in UTF-16 format) - it exists only in two drivers in version 1809, hidclass.sys and kbdhid.sys, and doesn't exist in version 1709.

In hidclass.sys they changed the HidpRegisterDeviceInterface function. Before this release it called IoRegisterDeviceInterface with GUID_DEVINTERFACE_HID and the ReferenceString pointer set to 0. But in the new version, depending on the result of GetHidClassCollection, it passes KBD as ReferenceString pointer.

Inside kbdhid.sys they changed KbdHid_Create, and here is a check for the KBD string to return errors (access denied or sharing violation).

To understand more exactly why, more research is needed. Some disasm:

enter image description here enter image description here enter image description here


For reference, HidpRegisterDeviceInterface from 1709 build

enter image description here

here ReferenceString == 0 always (xor r8d,r8d), and there's no check cmp word [rbp + a],6 on class collection data


However, KbdHid_Create in 1809 contains a bug. The code is:

NTSTATUS KbdHid_Create(PDEVICE_OBJECT DeviceObject, PIRP Irp)
{
  //...

    PIO_STACK_LOCATION IrpSp = IoGetCurrentIrpStackLocation(Irp);

    if (PFILE_OBJECT FileObject = IrpSp->FileObject)
    {
        PCUNICODE_STRING FileName = &FileObject->FileName;

        if (FileName->Length)
        {
        #if ver == 1809
            UNICODE_STRING KBD = RTL_CONSTANT_STRING(L"KBD"); // !! bug !!

            NTSTATUS status = RtlEqualUnicodeString(FileName, &KBD, FALSE)
                ? STATUS_SHARING_VIOLATION : STATUS_ACCESS_DENIED;
        #else
            NTSTATUS status = STATUS_ACCESS_DENIED;
        #endif

            // log

            Irp->IoStatus.Status = status;
            IofCompleteRequest(Irp, IO_NO_INCREMENT);
            return status;
        }
    }
    // ...
}

What it this function trying to do here? It looks for passed PFILE_OBJECT FileObject from Irp current stack location. It no FileObject is provided or it has an empty name, allow open; otherwise, the open fails.

Before 1809 it always failed with error STATUS_ACCESS_DENIED (0xc0000022), but starting from 1809, the name is checked, and if it's equal to KBD (case sensitive) another error - STATUS_SHARING_VIOLATION is returned. However, name always begins with the \ symbol, so it will never match KBD. It can be \KBD, so, to fix this check, the following line needs to be changed to:

UNICODE_STRING KBD = RTL_CONSTANT_STRING(L"\\KBD");

and perform the comparison with this string. So, by design we should have got a STATUS_SHARING_VIOLATION error when trying to open a keyboard device via *\KBD name, but due to an implementation error we actually got STATUS_ACCESS_DENIED here

enter image description here

Another change was in HidpRegisterDeviceInterface - before the call to IoRegisterDeviceInterface on the device it queries the GetHidClassCollection result, and if some WORD (2 byte) field in the structure is equal to 6, adds KBD suffix (ReferenceString). I guess (but I'm not sure) that 6 can be the Usage ID for keyboard, and the rationale for this prefix is to set Exclusive access mode


Actually, we can have a FileName begin without \ if we use relative device open via OBJECT_ATTRIBUTES. So, just for test, we can do this: if the interface name ends with \KBD, first open the file without this suffix (so with empty relative device name), and this open must work ok; then, we can try relative open file with name KBD - we must get STATUS_SHARING_VIOLATION in 1809 and STATUS_ACCESS_DENIED in previous builds (but here we will be have no \KBD suffix):

void TestOpen(PWSTR pszDeviceInterface)
{
    HANDLE hFile;

    if (PWSTR c = wcsrchr(pszDeviceInterface, '\\'))
    {
        static const UNICODE_STRING KBD = RTL_CONSTANT_STRING(L"KBD");

        if (!wcscmp(c + 1, KBD.Buffer))
        {
            *c = 0;

            OBJECT_ATTRIBUTES oa = { sizeof(oa), 0, const_cast<PUNICODE_STRING>(&KBD) };

            oa.RootDirectory = CreateFileW(pszDeviceInterface, 0, 
                FILE_SHARE_VALID_FLAGS, 0, OPEN_EXISTING, 0, 0);

            if (oa.RootDirectory != INVALID_HANDLE_VALUE)
            {
                IO_STATUS_BLOCK iosb;

                // will be STATUS_SHARING_VIOLATION (c0000043)
                NTSTATUS status = NtOpenFile(&hFile, SYNCHRONIZE, &oa, &iosb, 
                    FILE_SHARE_VALID_FLAGS, FILE_SYNCHRONOUS_IO_NONALERT);

                CloseHandle(oa.RootDirectory);

                if (0 <= status)
                {
                    PrintAttr(hFile);
                    CloseHandle(hFile);
                }
            }

            return ;
        }
    }

    hFile = CreateFileW(pszDeviceInterface, 0, 
         FILE_SHARE_VALID_FLAGS, 0, OPEN_EXISTING, 0, 0);

    if (hFile != INVALID_HANDLE_VALUE)
    {
        PrintAttr(hFile);
        CloseHandle(hFile);
    }
}
void PrintAttr(HANDLE hFile)
{
    HIDD_ATTRIBUTES deviceAttributes = { sizeof(deviceAttributes) };

    if(HidD_GetAttributes(hFile, &deviceAttributes)) {
        printf("VID = %04x PID = %04x\r\n", 
            (ULONG)deviceAttributes.VendorID, (ULONG)deviceAttributes.ProductID);
    } else {
        bad(L"HidD_GetAttributes");
    }
}

In a test on 1809 I actually got STATUS_SHARING_VIOLATION, that also shows another bug in kbdhid.KbdHid_Create - if we check FileName, we need to check RelatedFileObject - is it 0 or not.


Also, not related to bug, but as suggestion: it is more efficient to use CM_Get_Device_Interface_List instead of the SetupAPI:

volatile UCHAR guz = 0;

CONFIGRET EnumInterfaces(PGUID InterfaceClassGuid)
{
    CONFIGRET err;

    PVOID stack = alloca(guz);
    ULONG BufferLen = 0, NeedLen = 256;

    union {
        PVOID buf;
        PWSTR pszDeviceInterface;
    };

    for(;;) 
    {
        if (BufferLen < NeedLen)
        {
            BufferLen = RtlPointerToOffset(buf = alloca((NeedLen - BufferLen) * sizeof(WCHAR)), stack) / sizeof(WCHAR);
        }

        switch (err = CM_Get_Device_Interface_ListW(InterfaceClassGuid, 
            0, pszDeviceInterface, BufferLen, CM_GET_DEVICE_INTERFACE_LIST_PRESENT))
        {
        case CR_BUFFER_SMALL:
            if (err = CM_Get_Device_Interface_List_SizeW(&NeedLen, InterfaceClassGuid, 
                0, CM_GET_DEVICE_INTERFACE_LIST_PRESENT))
            {
        default:
            return err;
            }
            continue;

        case CR_SUCCESS:

            while (*pszDeviceInterface)
            {
                TestOpen(pszDeviceInterface);

                pszDeviceInterface += 1 + wcslen(pszDeviceInterface);
            }
            return 0;
        }
    }
}

EnumInterfaces(const_cast<PGUID>(&GUID_DEVINTERFACE_HID));
Matteo Italia
  • 123,740
  • 17
  • 206
  • 299
RbMm
  • 31,280
  • 3
  • 35
  • 56
  • This all seems so absurd... returning access denied depending purely on the reference string (as opposed, e.g., to an ACL over the device) seems a hacked job for security, and on the other hand I cannot imagine the reason why this was introduced in first place. Also, I received reports (yet to be confirmed) that a subsequent update fixes this problem, so it really has the feeling of some internal test gone in release by mistake... If it wasn't, some official documentation of this kind of behavior would be welcome, to understand if we have to change how to communicate with our device for good. – Matteo Italia Dec 13 '18 at 23:04
  • @MatteoItalia - `HidpRegisterDeviceInterface` call [`GetHidClassCollection`](https://jira.reactos.org/secure/attachment/48001/48001_0001-Apply-all-Vadim-Galyant-non-merged-patches.patch) and if `WORD` at offset 0xa in `HIDCLASS_COLLECTION` equal to 6 add `KBD` suffix. here question - what is definition of `HIDCLASS_COLLECTION` and what is 6 here – RbMm Dec 13 '18 at 23:14
  • 6 is the HID usage ID for keyboard devices, so it is adding the kdb suffix to all keyboards; indeed it seems like it's a (rudimentary and misguided) way to stop low-level access to keyboard devices. Indeed, several people in my company suggested that this may have been some hacked way to hinder some family of keyloggers. But again, it looks inexplicable, the correct way (compatibility problems notwistanding) would have been to fix the devices ACLs, not to hardcode that keyboard devices are "magic" and cannot be opened through the enumerated path. – Matteo Italia Dec 13 '18 at 23:22
  • @MatteoItalia 6 - if you mean [this](https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/top-level-collections-opened-by-windows-for-system-use) - Keyboard exclusive device by design, but i not do hid programming before. i view only changes between 1709 and 1809. just now hard say about reason – RbMm Dec 13 '18 at 23:26
  • @MatteoItalia - anyway problem not in your code (i prefer use here `CM_Get_Device_Interface_ListW` with `GUID_DEVINTERFACE_HID` instead using setupapi, but this of course give the same interface names) but in new drivers. (*hidclass.sys* and *kbdhid.sys*) – RbMm Dec 13 '18 at 23:32
  • Aaaah that is an interesting lead! But it's strange anyhow, as I would expect a sharing violation (32) for that - and indeed, it's what I saw reported in some HID libraries bug reports for opening keyboard/mice devices with wrong sharing mode (say, `SHARE_READ` only, when the system opened it with `FILE_SHARE_READ | FILE_SHARE_WRITE`). Also, it's interesting that *only keyboards* seems to be impacted - the test program has no problem opening mice (just to query their VID/PID), even though they should be opened exclusively by the system just the same. – Matteo Italia Dec 13 '18 at 23:33
  • Thank you for confirmation, I guess we'll just have to live with the workaround for now, and see if following updates did change this; I'll have a look with IDA as well to see the precise changes in the following version. Still, some documentation about this change is sorely needed! – Matteo Italia Dec 13 '18 at 23:35
  • @MatteoItalia - you also can enumerate keyboard via `GUID_DEVINTERFACE_KEYBOARD` if for hid you got `\\?\hid#vid_24d6&pid_8000&mi_00#7&294a3305&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}\kbd` for `GUID_DEVINTERFACE_KEYBOARD` you must got `\\?\hid#vid_24d6&pid_8000&mi_00#7&294a3305&0&0000#{884b96c3-56ef-11d1-bc8c-00a0c91405dd}` (different only in {guid}) both interface point to the same device. via second interface keyboard open ok and `HidD_GetAttributes` also ok here – RbMm Dec 14 '18 at 00:00
  • @MatteoItalia - however this is really driver bug in `KbdHid_Create` ! i look more carefully to code. if look for `IrpSp->FileObject->FileName` if it empty - allow open. if not empty compare with `KBD` string (case sensetive) and if equal return `c0000043` (share violation) otherwise `c0000022` access denied. so by design must be access denied. but name if not empty will be always begin with \ so name in you case (after trancate device name) will be **\KBD** not **KBD** because this error you got c0000022 instead c0000043 by design – RbMm Dec 14 '18 at 00:35
5

The fix is in this windows update released today (March 1, 2019).

https://support.microsoft.com/en-us/help/4482887/windows-10-update-kb4482887

andrew
  • 66
  • 1
  • 1
  • There's nothing in the changelog that suggests changes to handling of HID devices, are you sure it's this update? – Matteo Italia Mar 01 '19 at 19:06
  • 1
    Indeed several users of ours installed this update and it's fixed. As there's nothing in the changelog, the whole issue remains mysterious. – Matteo Italia Mar 07 '19 at 07:50
-2

You can find a workaround at Delphi-Praxis in German

For short: Change in the Unit JvHidControllerClass

    if not HidD_GetAttributes(HidFileHandle, FAttributes) then
  raise EControllerError.CreateRes(@RsEDeviceCannotBeIdentified);

to

    HidD_GetAttributes(HidFileHandle, FAttributes);

and recompile the Delhi JCL and JCVL Components by running the JEDI Install EXE.

Walter Schrabmair
  • 1,251
  • 2
  • 13
  • 26