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

Check for active_configuration before using it. #135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasperla
Copy link

Using usbproxy.py with an older Yubikey results in this error when the target system attempts to attach the device via the proxy.

INFO    | logging        | [17:55:07] USBControlRequest(direction=1, type=0, recipient=1, number=6, value=8704, index=0, length=71, data=bytearray(b''), device=USBProxyDevice(name='generic device', device_class=0, device_subclass=0, protocol_revision_number=0, max_packet_size_ep0=64, vendor_id=24843, product_id=18003, manufacturer_string='Facedancer', product_string='Generic USB Device', serial_number_string='S/N 3420E', supported_languages=(<LanguageIDs.ENGLISH_US: 1033>,), device_revision=0, usb_spec_version=512, device_speed=None, requestable_descriptors={<DescriptorTypes.DEVICE: 1>: <function USBBaseDevice.__post_init__.<locals>.<lambda> at 0x7efffe1984a0>, <DescriptorTypes.CONFIGURATION: 2>: <bound method USBBaseDevice.get_configuration_descriptor of ...>, <DescriptorTypes.STRING: 3>: <bound method USBBaseDevice.get_string_descriptor of ...>}, configurations={}, backend=<facedancer.backends.greatdancer.GreatDancerApp object at 0x7efffd0c1590>))
INFO    | logging        | [17:55:07] <: b'\x05\x01\t\x06\xa1\x01\x05\x07\x19\xe0)\xe7\x15\x00%\x01u\x01\x95\x08\x81\x02\x95\x01u\x08\x81\x01\x95\x05u\x01\x05\x08\x19\x01)\x05\x91\x02\x95\x01u\x03\x91\x01\x95\x06u\x08\x15\x00%e\x05\x07\x19\x00)e\x81\x00\t\x03u\x08\x95\x08\xb1\x02\xc0'
TRACE   | greatdancer    | EP0/IN: <- b'\x05\x01\t\x06\xa1\x01\x05\x07\x19\xe0)\xe7\x15\x00%\x01u\x01\x95\x08\x81\x02\x95\x01u\x08\x81\x01\x95\x05u\x01\x05\x08\x19\x01)\x05\x91\x02\x95\x01u\x03\x91\x01\x95\x06u\x08\x15\x00%e\x05\x07\x19\x00)e\x81\x00\t\x03u\x08\x95\x08\xb1\x02\xc0'
INFO    | greatdancer    | Disconnecting from host.
Traceback (most recent call last):
  File "/home/user/hack/usb/facedancer/examples/usbproxy.py", line 29, in <module>
    main(proxy)
  File "/home/user/hack/usb/facedancer/facedancer/devices/__init__.py", line 44, in default_main
    device.emulate(*coroutines)
  File "/home/user/hack/usb/facedancer/facedancer/device.py", line 234, in emulate
    self.run_with(*coroutines)
  File "/home/user/hack/usb/facedancer/facedancer/device.py", line 219, in run_with
    asyncio.run(inner())
  File "/usr/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/user/hack/usb/facedancer/facedancer/device.py", line 217, in inner
    await asyncio.gather(self.run(), *coroutines)
  File "/home/user/hack/usb/facedancer/facedancer/device.py", line 206, in run
    self.backend.service_irqs()
  File "/home/user/hack/usb/facedancer/facedancer/backends/greatdancer.py", line 792, in service_irqs
    self._handle_transfer_events()
  File "/home/user/hack/usb/facedancer/facedancer/backends/greatdancer.py", line 496, in _handle_transfer_events
    self._handle_transfer_readiness()
  File "/home/user/hack/usb/facedancer/facedancer/backends/greatdancer.py", line 642, in _handle_transfer_readiness
    self.connected_device.handle_buffer_available(endpoint.number)
  File "/home/user/hack/usb/facedancer/facedancer/device.py", line 383, in handle_buffer_available
    endpoint = self.get_endpoint(ep_num, USBDirection.IN)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/hack/usb/facedancer/facedancer/device.py", line 344, in get_endpoint
    endpoint = self.configuration.get_endpoint(endpoint_number, direction)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/hack/usb/facedancer/facedancer/configuration.py", line 151, in get_endpoint
    for interface in self.active_interfaces.values():
                     ^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'USBConfiguration' object has no attribute 'active_interfaces'. Did you mean: 'get_interfaces'?

Proxying system is Linux and target system is macOS.

The issue seems to be that the USBConfiguration class assumes active_interfaces always exists. However that not always the case it seems. Adding an explicit check for it resolves the issue for me and the proxied device works as expected.

@martinling
Copy link
Member

There is indeed a bug here, but this isn't the right way to fix it.

The underlying problem here is that I broke USBProxy when I added alternate interface support in PR #122. 😢

The active_interfaces attribute needs to be initialized to default values when the configuration is applied with a SET_CONFIGURATION request, and also needs to be updated when alternate interface settings are applied with a SET_INTERFACE request. I did that in the standard request handlers on USBDevice, but those aren't used by the proxy code.

So we need some changes to USBProxy to do this. I've just opened a draft PR #137, which implements the SET_CONFIGURATION part of this and may work for you.

But it doesn't yet deal with intercepting SET_INTERFACE requests and updating the active_interfaces, and actually that opens a bigger can of worms, because on backends with a fixed number of hardware endpoints, we're sometimes going to need to reallocate those on a SET_INTERFACE request, because that action changes the set of available endpoints that the host can use.

PR #122 didn't address that, and addressing it properly is going to require new handling in all of those backends. 😭

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