[WIP] Adding WebUSB + WinUSB

[karelbilek]

WebUSB is a web standard, that is pushed by Chrome and so far only implemented by Chrome, that allows webpages direct access to USB.

https://wicg.github.io/webusb/

WebUSB allows us to "cut" bridge and chrome extension; since Chrome Extension is actually a Chrome App, and Google will start cutting those off soon, and they don't offer chrome.hid API to extensions (other than built-in Google extensions like the hidden built-in U2F extension), this is a little pressing issue. Another plus is seemless integration into Android - it actually works on Android Chrome, and you can actually run full Web Wallet. (Of course the GUI is terrible there. But still, current web wallet with a small change works on Android with no hassle :) that is surprising)

WebUSB cannot access HID devices directly, at least not on Android and Linux, where the HID interfaces are claimed by the kernel. For that reason, we need another interface.

@saleemrashid originally wanted to have the two interfaces to share the same endpoints, but it seems that's not possible, since this causes kernel panic on some linux systems because of some sysfs issues. ( See this outdated branch https://github.com/trezor/trezor-mcu/commits/webusb-wip )

WebUSB needs binary object storage and usb2.1, which @prusnak added, mostly from https://github.com/devanlai/dap42/tree/winusb-wip/src/USB ; that is used for adding a whitelist of domains (webusb doesn't use it, however, when used directly and everything is whitelisted; however, it is used when used in iframe, which we want to use in future Trezor Connect versions), and an URL that user sees when connected the device (however, that does work only on Linux and macOS so far).

However, the bigger issue is Windows, plus Google's buggy implementation of libusb on Windows.

If we want Trezor to be usable on windows without installing any drivers, we need trezor to also implement WinUSB and additional descriptors. I have added MS OS 1.0 descriptors, mostly from https://github.com/devanlai/dapboot , and also from documentation here https://github.com/pbatard/libwdi/wiki/WCID-Devices . We also need DeviceGUIDs because of some libusb issues on chrome, also see below (plus the discussions on bottom).

A smaller issue with winusb is that it requires us to use a fork of libopencm3 with usbd_register_extra_string. I linked that on my github, maybe it would be better to have it on /trezor/ github instead. - https://github.com/runn1ng/libopencm3/commits/trezor - there is a PR here libopencm3/libopencm3#849 (I do not understand it enough to comment on that :D )

A bigger issue is that some bug in libusb code on Chrome on Windows prevents us from having WebUSB interface to be third, instead it has to be the first with index 0. When I did not switch the interfaces, I kept seeing "U2F Interface" and "TREZOR Interface" (that is, the two HID interfaces) in the WebUSB Chrome picker, and those did not work - it usually crashes Chrome outright (!!!), or just doesn't work. Interestingly sometimes the device works and correctly shows "TREZOR" in the picker and the interfaces work, even with WebUSB being the third, but in about half of the instances it doesn't work and crashes the browser.

So I switched the interfaces and put webusb first. Unfortunately that will break some other integrations - notably, bridge and python-trezor. Extension still works, mycelium and Android manager on Android also works. I would be glad if this wasn't necessary, but I could not get it working reliably.

I have a simple webpage for testing webusb here - https://estimatesmartfee.com/webusb.html (sorry for the unrelated URL, but it is the simpliest way for me to put pages on https, and webusb needs https :)). If it correcly dumps trezor-looking data, webusb is working. (It just dumps raw result from Initialize call.) It's helpful for quick debugging.

Related discussions:

• Not finding devices webusb/arduino#4
• Better reference documentation on descriptors WICG/webusb#75 (very helpful, describes all the windows issues and warts)

[karelbilek]

Oh, and I have not added support or tested anything on bootloader, since I have no way of testing that.

[saleemrashid]

@Runn1ng To test the bootloader, you can do make -C fastflash && make -C firmware FASTFLASH=1. At the unofficial firmware warning, hold down the left button while you press the right button.

[karelbilek]

@saleemrashid OK, and I need to first do make -C bootloader before, right?

[saleemrashid]

@Runn1ng No

[karelbilek]

Anyway, if you find out why exactly is webusb/winusb not working on windows chrome if not first, I will be happy to hear it and fix it, but the linked webusb discussion seems like it's not a solved problem (Chrome would need to patch its libusb library, and they are instead writing their own USB backend).

Also @saleemrashid had an idea to use two configurations. I have tried that and it didn't work (at all, even on Linux/Android), PC/phone didn't see the phone at all. I could not find any example for libopencm3 with more configurations, so maybe I did it wrong; you are welcome to try. Also I am not sure if it would actually fix the error with webusb/winusb/libusb that forces the new interface to be first; I think it wouldn't

[karelbilek]

I have also tried to use three device GUIDs instead of one (one for each interface), in the winusb.c / winusb_defs.c, but that didn't fix the issue either.

[saleemrashid]

If we are not able to keep HID on the first interface, I think it should be removed entirely. By moving the interface, we have broken compatibility with everything that relied on it using HID anyway so we can move away from hidapi and move to libusb.

@Runn1ng What configurations did you try and have you tried using an interface with alternate settings?

[karelbilek]

Switching interface ID from 0 to 2 based on bcdDevice (or something like that) is easier than switching libraries... Trezor Chrome Extension seems to work and that works through HID, android works (but that might work via USB and not hid).

I tried putting the two HIDs into one configuration and the one WebUSB into the other

I have not tried alternate interfaces

[saleemrashid]

As far as I know, python-trezor, trezord and the Chrome extension are the only software using HID (and those last two will be obsolete). Android uses the USB API, not HID.

Anyway, trying alternate interfaces could be useful. That way, people only have issues if they use WebUSB.

[karelbilek]

Webusb seems to allow alternate interfaces. I will try that if I have time

[karelbilek]

I again don't see many libopencm3 examples for that :/ and I don't want to reinwent a wheel

Seems usbd_set_altsetting_callback could work

[saleemrashid]

@karel-3d It should be as "easy" as changing WebUSB to use the same interface and endpoint number as HID, moving hid_iface_webusb[0] to hid_iface[1] and changing num_altsetting in ifaces[].

[saleemrashid]

A smaller issue with winusb is that it requires us to use a fork of libopencm3 with usbd_register_extra_string.

Can that not be implemented with usbd_register_control_callback?

[karelbilek]

from WCID docs

The first time an USB device is plugged in, the Microsoft USB port driver (usbport.sys) will read the standard USB Descriptors, which includes the standard Device, Configuration, Interface and String Descriptors. It will also attempt to read an additional String Descriptor located at index 0xEE. This String Descriptor, which is not mandated by the USB specifications, is a Microsoft extension called an OS String Descriptor and it is the first of two elements that establish whether a device is WCID.

Not sure if it could be done using that

[saleemrashid]

@karel-3d Can't you just handle USB_REQ_GET_DESCRIPTOR for wValue = (USB_DT_STRING << 8 | 0xEE)?

[karelbilek]

Thanks for the tip, I will try later

[karelbilek]

Btw, I am now reading a WinUSB documentation and it seems it needs to be the first configuration.

WinUSB supports only the first configuration.

https://msdn.microsoft.com/en-us/library/windows/hardware/ff537060(v=vs.85).aspx

[karelbilek]

I have tried the two altsettings, and it failed very quickly.

Branch here

https://github.com/karel-3d/trezor-mcu/tree/altsettings

Windows writes a connection error right after connecting the device, and I see in the Device manager an error

Unknown USB Device (Device Descriptor Request Failed)

edit: ....oh, that is because I have 2 interfaces, but bNumInterfaces in the main config is still 3. :) ok again

edit2: with that fixed (see branch), windows now fails with

This device cannot start.

{Operation: Failed}

The requested operation was unsuccessful.

[karelbilek]

Ad extra string via handling control request in winusb.c

I got stuck on trying to port this line

https://github.com/karel-3d/libopencm3/commit/852acf5a3f824e0d632ca455b9c1a80b74f51435#diff-8bcbc0caefe848931a908f3b2b39f4d9R167

It seems I cannot dereference this

sd = (struct usb_string_descriptor *)usbd_dev->ctrl_buf;

since usbd_device is not a complete type (it's in usb_private.h). I don't think I should include libopencm3 usb_private.h in webusb.c... and it seems I even can't

[saleemrashid]

@karel-3d You don't need to use the control buffer directly. You just need to write it into the buffer and length pointer in the handler arguments and return USBD_REQ_HANDLED.

[karelbilek]

@saleemrashid you were right.

I have added another commit with the extra string in descriptor handling (and removed the commit with the repo change), we can still be on libopencp3 upstream.

[saleemrashid]

Use usb_descriptor_type(req->wValue)

[karelbilek]

those are not exported from usb_standard.c; I didn't want to retype them here for 1 use :)

[karelbilek]

but maybe yeah, it will make it more readable

[saleemrashid]

Oops, my bad. I was looking at http://libopencm3.org/docs/latest/stm32f2/html/group__usb__standard__file.html and I didn't see it said static int.

[karelbilek]

I have added those and used them. It doesn't hurt. (Maybe macro would be quicker, but maybe it won't matter.)

[saleemrashid]

Use usb_descriptor_index(req->wValue)

[saleemrashid]

const doesn't make sense here

[karelbilek]

You seem to be right. I copy-pasted this code. But it seems to be useless here. (It compiles though :))

[karelbilek]

Removed

[karelbilek]

removed

[saleemrashid]

Why do you need this check?

[karelbilek]

Not sure (it's from the devanlai library), but this is basically what is done in libopencp3 on here and on similar lines

https://github.com/libopencm3/libopencm3/blob/master/lib/usb/usb_standard.c#L160

(this is basically the same, just written differently)

[saleemrashid]

Perhaps we ought to change

trezor-mcu/firmware/usb.c

Line 322 in bc7c66a

*len = sizeof(hid_report_descriptor_u2f);

[karelbilek]

That should do the same thing as this; and again, this is also using similar style to libopencm3 code that uses the headers, not the structs. It's nicer though, yeah

misread comment

[saleemrashid]

@karel-3d I'm suggesting that we change usb.c to match this (compare *len).

[karelbilek]

Oh OK, I misread the comment. :)

I have cleaned up the code a bit so it now uses MIN so it is perhaps cleaner.

[karelbilek]

I have added this logic to usb.c (both firmware and bootloader, although it probably won't matter there) and also usb21_standard.c, it should be in this PR also

[karelbilek]

I added your MIN/MAXs.

[karelbilek]

This is the libusb issue

libusb/libusb#85

see also
https://sourceforge.net/p/openocd/tickets/96/
https://marc.info/?l=libusb-devel&m=139216876306706&w=2

I don't think we can work around that, other than wait for Chrome to finish its windows backend rewrite :(

(see https://bugs.chromium.org/p/chromium/issues/detail?id=422562 and https://bugs.chromium.org/p/chromium/issues/detail?id=637404 for that, but it is WIP and the development seems stopped at around March this year)

Other suggestions mentioned is using WinUSB for the whole device; I think that in that case HID API stops working since the HID driver is no longer loaded. And we need that HID driver for U2F at least.

[karelbilek]

Hm. For some reason, this branch does not work on Android on existing apps, while this branch

https://github.com/karel-3d/trezor-mcu/tree/switched_ifaces

does.

I will investigate further :)

[karelbilek]

For some reasons, the webusb endpoint needs to be number 1 and the hid point to be number 3 for this firmware to work. I will look at android trezor source code :)

[karelbilek]

I have removed the HID interface after some internal discussion. So we will have 1 webusb interface on id 0, endpoint 1 (and 0x81), and 1 U2F interface (which has to be HID). And 1 debug link interface (that can be either HID or WebUSB, we will discuss internally).

Now the code is not nice, since there are useless HID functions and calls there for the now removed interface. Also the code probably doesn't work at all with debug link enabled.

[karelbilek]

I have cleaned up the commits somhehow (and removed some useless cruft from previous WebUSB drafts); bootloader is not yet finished.

[mcudev]

I am researching the Windows issue with a Windows 10 x64 Home system + Chrome 63 installed. The T1 device is recognized as a USB Composite Device and uses driver "usbccgp". So far I've tested with a regular 1.6.0 firmware and I can reproduce the "U2F Interface" issue in the chooser dialog in the browser. But, even though the label in the dialog uses the wrong interface name, things seem to work. I verified with Wireshark that the endpoints (0x81 and 0x01) of the first interface (aka TREZOR Interface) are the ones used, as my code desired. Does this just boil down to a labeling issue in the Chrome USB device chooser on Windows?

[karelbilek]

It's a snarl of OS issues :)

We cannot "just" use the current T1 HID interface with WebUSB, since on linux, we need a separate, non-HID interface, otherwise HID is claimed by kernel (the same issue is on Android and I think - not sure - macOS).

For that reason, I want to use non-HID interface. (That's the entire reason of not having HID interface - so it is usable on Linux and Android and probably macOS, but I am not sure about that now)

However, if it is is not HID, it needs to be WinUSB, so it is correctly seen by Windows and the driver installed.

If it uses WinUSB, the HID interface cannot be first, otherwise it reliably crashes on device.reset() - I have submitted the crash as a bug report to Chrome team, it is not public for some reason, but the crash stack is as follows

Crash stack (same for both reports):

0x00007ffd6f6a300e	(WinUSB.DLL + 0x0000300e )	WinUsb_AbortPipe
0x00007ffd4a148ed1	(chrome.dll -windows_usb.c:3121 )	winusbx_reset_device
0x00007ffd4a14b945	(chrome.dll -windows_usb.c:4402 )	composite_reset_device
0x00007ffd4a13e9b9	(chrome.dll -usb_device_handle_impl.cc:940 )	device::UsbDeviceHandleImpl::ResetDeviceOnBlockingThread(base::OnceCallback<void >)
0x00007ffd48fa9820	(chrome.dll -bind_internal.h:318 )	base::internal::Invoker<base::internal::BindState<void ( content::BackgroundSyncContext::*)(scoped_refptr<content::ServiceWorkerContextWrapper>),scoped_refptr<content::BackgroundSyncContext>,scoped_refptr<content::ServiceWorkerContextWrapper> >,void >::RunOnce(base::internal::BindStateBase *)
0x00007ffd4979c637	(chrome.dll -task_annotator.cc:57 )	base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask *)
0x00007ffd49795c76	(chrome.dll -task_tracker.cc:411 )	base::internal::TaskTracker::RunOrSkipTask(std::unique_ptr<base::internal::Task,std::default_delete<base::internal::Task> >,base::internal::Sequence *,bool)
0x00007ffd497954c3	(chrome.dll -task_tracker.cc:311 )	base::internal::TaskTracker::RunNextTask(scoped_refptr<base::internal::Sequence>,base::internal::CanScheduleSequenceObserver *)
0x00007ffd497a4250	(chrome.dll -scheduler_worker.cc:72 )	base::internal::SchedulerWorker::Thread::ThreadMain()
0x00007ffd4972324e	(chrome.dll -platform_thread_win.cc:89 )	base::`anonymous namespace'::ThreadFunc
0x00007ffd77481fe3	(KERNEL32.DLL + 0x00011fe3 )	BaseThreadInitThunk
0x00007ffd7836ef90	(ntdll.dll + 0x0006ef90 )	RtlUserThreadStart

you can follow the code on chrome source code - it's a LibUSB issue. Also the device is wrongly displayed, as you noted. Both issues are prevented if WebUSB is first.

[karelbilek]

Oh and we do need to have a HID interface, since U2F only works through HID. (It needs to be the second one though, not the first one, because the libusb issue on Windows)

[karelbilek]

In theory, we could add WebUSB third after existing ones, then detect OS in browser and on Windows use WebUSB with the first HID interface and on Linux with the third, non-HID WebUSB interface, and make sure never to call device.reset() on Windows (since without device.reset() it seems to work fine-ish, only device.reset crashes), but I don't feel very secure with that solution...

[mcudev]

we could add winusb third after existing ones

I was just thinking that. I was thinking though that you'd probably wind up with multiple interfaces in the chooser, which could be confusing.

What you've said makes sense. I'll tinker more later.

[karelbilek]

That was the original plan, I have it in some of my branches on my repo (if you can orient in the mess - sorry :)), but still, I don't feel very secure to have WinUSB second after HID, if we know it has issues on Windows (it was reported by more users than us, see WICG/webusb#75 ) and can even crash Chrome itself in some cases

Also @prusnak doesn't want to maintain two interfaces that do the same thing :)

[prusnak]

Yes, I don't want to have 2 (HID + WebUSB) interfaces for the same thing.

[mcudev]

What's proposed here looks like the best option that I can see given the circumstances for Windows, Linux, Mac OS X, and Android (hopefully iOS doesn't add more restrictions).

I know you're not done with the bootloader part yet. Are you going to make the bootloader interface similar to the first non-bootloader interface? Existing flash locked devices are stuck on HID. Is it worth upgrading the bootloader interface on new T1 devices and having to support multiple different firmware upgrade processes?

Two minor things if/when you get to that: 1) add #include "winusb.h" to bootloader/usb.c (you probably left out on purpose) and 2) update the bootloader version.

[prusnak]

Are you going to make the bootloader interface similar to the first non-bootloader interface?
Yes.

Existing flash locked devices are stuck on HID. Is it worth upgrading the bootloader interface on new T1 devices and having to support multiple different firmware upgrade processes?

We cannot upgrade the old bootloaders, but it is worthwhile fixing this for the future. T2 will use WebUSB exclusively (it does not atm).

Two minor things if/when you get to that: 1) add #include "winusb.h" to bootloader/usb.c (you probably left out on purpose) and 2) update the bootloader version.

Sure thing, we need to do both.

[saleemrashid]

having to support multiple different firmware upgrade processes

WebUSB would be useless if not implemented in the bootloader because the user would still need the Extension or Bridge to do firmware upgrades.

1. add #include "winusb.h" to bootloader/usb.c (you probably left out on purpose)

I think @karel-3d is working on it in 7f95073

[prusnak]

the user would still need the Extension or Bridge to do firmware upgrades.

Which is still true for already released bootloaders. We plan to create a dedicated application just to do that one thing. We are currently rewriting the bridge to Go and this would act either as a bridge or as a standalone updater app or both.

[karelbilek]

hopefully iOS doesn't add more restrictions

I have no idea about webusb in iOS; I don't think it is, or ever will be, supported; since Safari doesn't seem to support it currently on macOS or iOS and Chrome on iOS is just a wrapper around native webkit (from what I understand).

But I suppose if it ever is supported, it will be the same on macOS Safari and iOS Safari.

[karelbilek]

I will prepare the bootloader, resolve the conflicts and then this would be ready for merging, right?

[karelbilek]

Bootloader finished and tested, conflicts resolved.

I still wish to know why we have bInterval=2 here and if it is necessary

06ae04b#r26615948

[saleemrashid]

@karel-3d bInterval is just the interrupt polling interval in frames (1ms for full-speed). I was following the convention the other endpoints use (where the main endpoint has bInterval = 1 and the others have bInterval = 2).

If WebUSB will become the main interface, it would be wise to change that to bInterval = 1.

[saleemrashid]

By the way, has anyone tested this on Windows 7? Everyone seems to be testing on Windows 10, which has many WinUSB changes from Windows 8.1 but we should definitely support Windows 7 (Windows XP support would be good ducks)

[karelbilek]

Windows 8 should work, since I used MSOS1.0 WinUSB descriptors, but I haven't tested it

Windows 7 + Vista might work, but not sure. This says you need to be online when you connect the device and the driver is downloaded using Windows Update - https://github.com/pbatard/libwdi/wiki/WCID-Devices .

On Windows XP SP2+, you need to manually install the WCID driver, and I am too lazy to find out how to do it, since Chrome won't run there anyway :D

[karelbilek]

It took me 2 days to get Windows 7 working on my notebook (what a waste of time!), but I got it working - and yeah, it works there, but you need to be online, since the driver is downloaded from the internet.

However, there is an issue, which is a libusb issue I have seen mentioned elsewhere - Chrome doesn't show "TREZOR" in the picker, but it shows the description of the last interface - that is, "U2F Interface".

I will fix it by just adding "TREZOR" as the string description of all the interfaces. It is ugly, but it seems to fix this.

edit: nevermind, I tried some old version by mistake. Also I have added "TREZOR" to the U2F interface description string, so if Chrome shows it, people still know it's trezor, so it's fine.

OK ready to merge :)

[prusnak]

LGTM

Pulled it into https://github.com/trezor/trezor-mcu/tree/webusb for now!

Thank you all for hard work that went into this! Hopefully we'll have other parts of infrastructure ready (web wallet, python-trezor) and I'll merge this into master.