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

Yubikey works #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

paweljasinski
Copy link

@paweljasinski paweljasinski commented Oct 28, 2023

Here are 2 fixes which make it possible to use yubikey remotely.

  1. First fix prevents from reporting incoming packet size as outgoing packet and is probably as universal as I can imagine.
  2. The second one is a bit tricky and I am not convinced that this is the end of the story.
    GPT suggested that detecting the end of bulk transfer is not universal condition and can depend on a device.
    One of the logic listed was exactly what the original code did. If api gives back less than what max packet size is, that is the end of transmission. However, for yubikey it is actually more than max packet size. This hints at possibility that android api reports wrong max size or android api reassemble packets before giving it back or reported max size is wrong or something else. For now this is the fix. I will keep digging - have to look at android api source and probably check what yubikey tools are doing.

@@ -52,9 +52,10 @@ public static int doBulkTransfer(UsbDeviceConnection devConn, UsbEndpoint endpoi

bytesTransferred += res;

if (res < endpoint.getMaxPacketSize()) {
if (res != endpoint.getMaxPacketSize()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if this is needed or if the while condition is sufficient. Does it work properly if you remove this check entirely?

Copy link
Author

@paweljasinski paweljasinski Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all the request comes with buff.length (expected) size of 65536. The only reasonable interpretation is maximum size. So we can't check buff.length vs. transferred size. Yubikey is sending back a certificate which is a variable size.
Second, I know this code is by definition wrong, because I can manipulate certificate to be exactly max packet size.

Now back to your question.
If I take the condition out it will terminate with timeout and return error.
Perhaps if there is nothing more, it would return right away. Nop, it is a real timeout with 1000ms.
I am reading the server part of the linux driver to figure out how it is done. First impression, things depend on transfer flags.

@paweljasinski
Copy link
Author

@cgutman, what are the original target devices? I assumed the primary use was a USB memory stick.
How can I trigger a transfer which will produce one or more packets with size matching max packet size and after a packet shorter than that?

@cgutman
Copy link
Owner

cgutman commented Nov 2, 2023

what are the original target devices?

It's been so long that I don't remember. Maybe a USB drive and gamepad?

How can I trigger a transfer which will produce one or more packets with size matching max packet size and after a packet shorter than that?

I'm not sure. A bulk transfer from a mass-storage device seems like it would work.

@paweljasinski
Copy link
Author

I have tested with a couple of memory sticks and in all cases all communication is completed with one call to bulkTransfer. I was not able to trigger any communication which would require 2 calls to bulkTransfer.
Because I have no access to gamepad with usb, I have took the best available approximation, keyboard. The standard PC keyboard was captured by android, so I used mac keyboard. The test didn't hit code path in question.

How about removing the while loop?

Can you think of any other devices with bulk transfer to get more testing?

@cgutman
Copy link
Owner

cgutman commented Nov 10, 2023

How about removing the while loop?

Yeah, I guess that's probably fine.

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