communication/u2fhid: prepend 0 byte to packet #73
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Issue opened upstream: karalabe/usb#30 |
Beerosagos
reviewed
Oct 25, 2022
communication/u2fhid/u2fhid.go
Outdated
| if runtime.GOOS != "windows" { | ||
| // packets have a 0 byte report ID in front. The karalabe usb library adds it | ||
| // automatically for windows, and not for unix, as there, it is stripped by the signal11 | ||
| // hid library. We have to add it (to be stripped by signal11), otherwise a a zero that |
There was a problem hiding this comment.
small typo otherwise a a zero
|
tACK on Linux! 👍 |
If a USB packet starts with a zero byte, that byte will be stripped by the C USB library (i.e. signal11 on Linux), leading to a broken package. We only need to add it if not on Windows, as karalabel/usb prepends this zero byte already for Windows (I think this is a bug in karalabe/usb and karalabe/hid, which didn't consider that a packet could start with a zero byte right after the report ID). Currently we don't observe a bug because the first byte in each packet is the `cid` (channel identifier), which is harddcoded to `0xff000000`. If we start using a random CID, and it starts with a zero byte, the packet will be corrupted and the BitBox02 will not be able to parse it. The same fix was also added to the bitbox01 in the BitBoxApp (https://github.com/digitalbitbox/bitbox-wallet-app/blob/bd70f6c400268355cee94891397447796f02fbde/backend/devices/bitbox/communication.go#L235-L241), where it was crucial because packets in the bitbox01 are padded with zero bytes to the left.
The CID in the response is compared against the hardcoded 0xff000000 CID. It should however parse and compare it to the const `cid`. This allows us to change the CID (i.e. make it random) and not break parsing.
|
After more consideration, I think this is the wrong location to add the fix. This library doesn't even depend on If the fix was merged here, I assume bitbox02-api-js via WebHID would break: https://github.com/digitalbitbox/bitbox02-api-js/blob/8205a813a58fa3effb86be6ce5c99a3c0561e551/gowrapper/main.go#L183 |
|
Split the first commit into a new PR, as that one is still useful: #75 |
|
Closing in favor of BitBoxSwiss/bitbox-wallet-app#1809 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a USB packet starts with a zero byte, that byte will be stripped by the C USB library (i.e. signal11 on Linux), leading to a broken package.
We only need to add it if not on Windows, as karalabel/usb prepends this zero byte already for Windows (I think this is a bug in karalabe/usb and karalabe/hid, which didn't consider that a packet could start with a zero byte right after the report ID).
Currently we don't observe a bug because the first byte in each packet is the
cid(channel identifier), which is harddcoded to0xff000000. If we start using a random CID, and it starts with a zero byte, the packet will be corrupted and the BitBox02 will not be able to parse it.The same fix was also added to the bitbox01 in the BitBoxApp (https://github.com/digitalbitbox/bitbox-wallet-app/blob/bd70f6c400268355cee94891397447796f02fbde/backend/devices/bitbox/communication.go#L235-L241), where it was crucial because packets in the bitbox01 are padded with zero bytes to the left.