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

Never-ending initialization for composite readers like Rocketek #849

Closed
emaxx-google opened this issue Jul 25, 2023 · 1 comment · Fixed by #843
Closed

Never-ending initialization for composite readers like Rocketek #849

emaxx-google opened this issue Jul 25, 2023 · 1 comment · Fixed by #843
Assignees
Labels

Comments

@emaxx-google
Copy link
Collaborator

emaxx-google commented Jul 25, 2023

We received a bug report about broken support of a Rocketek reader:

[230718 13:51:40.76] [NaclModule.Wrapper.ReaderTracker] [INFO] The reader "Generic USB2.0-CRW" (port 2097152, device "usb:0bda/0169:libusb-1.0:1:6:0") was successfully initialized
<...>
[230718 13:51:41.38] [NaclModule.Wrapper.ReaderTracker] [INFO] Information about readers returned by PC/SC: "Generic USB2.0-CRW 00 00" (with inserted card)
<...>
[230718 13:51:43.35] [NaclModule] [INFO] 00022100 ../../src/src/ccid_usb.c:205:close_libusb_if_needed() libusb_exit
[230718 13:51:43.35] [NaclModule.Wrapper.ReaderTracker] [INFO] The reader "Generic USB2.0-CRW" (port 2097153) was removed
[230718 13:51:43.37] [NaclModule.Wrapper.ReaderTracker] [INFO] Information about readers returned by PC/SC: no readers

The investigation showed it's because of behavior change in ChromeOS 105, in which permission_broker started disallowing openDevice calls for already-claimed composite devices. In combination with our heuristic "if opening fails, simulate that the device reappears under a different bus number later", we essentially enter a never-ending loop: "initialize smart card USB interface => try opening the device again for another interface => react to the error by aborting opened connections => back to the initial step".

More details at https://b.corp.google.com/issues/291810618.

@emaxx-google emaxx-google self-assigned this Jul 25, 2023
emaxx-google added a commit that referenced this issue Jul 26, 2023
Add a basic unit test for our libusb_get_active_config_descriptor()
implementation (a.k.a LibusbJsProxy::LibusbGetActiveConfigDescriptor()).

The input parameters for the test are taken from an arbitrary real-world
reader (SCM SCR 3310).

This commit is preparation for regression testing of #849.
emaxx-google added a commit that referenced this issue Jul 26, 2023
Only let the PC/SC see those USB device interfaces that have suitable
properties. The exact criteria is replicated from what the CCID
driver does internally: either the interface class should be "11"
("Smart Card") or both it should be "255" ("Vendor Specific") and the
extra data length should be "54" (which is the standard length of the
CCID descriptor).

The primary goal is to workaround issues with specific composite devices
on modern ChromeOS versions. Before this commit, PC/SC would see all
interfaces and attempt to connect via each of them. This used to cause
no harm, except spurious errors (which we suppressed heuristically).

However, ChromeOS version 105 introduced behavior change, not allowing
to connect to such composite devices more than once. In combination with
our previously built heuristic "if connection fails, simulate that the
whole device reappears under a different bus number" this results in
some devices not being able to initialize at all. This commit fixes such
issues (we're still discussing if ChromeOS permission_broker could
behave better in this case). As a bonus, we reduce log spam for all
composite devices.

This fixes #849. More context in https://b.corp.google.com/issues/291810618.
@emaxx-google emaxx-google reopened this Jul 26, 2023
@emaxx-google
Copy link
Collaborator Author

It looks like we also need to patch the CCID Driver accordingly, because apparently it's doing null dereferences in some cases (which sometimes happen to work, but sometimes cause crashes).

emaxx-google added a commit that referenced this issue Jul 26, 2023
Cherry-pick the "Check num_altsetting before dereffing altsetting"
commit in the CCID Driver repository.

This is supposed to fix the occasional crashes we observe after
applying the mitigation from #849.
emaxx-google added a commit that referenced this issue Jul 26, 2023
Cherry-pick the "Check num_altsetting before dereffing altsetting"
commit in the CCID Driver repository.

This is supposed to fix the occasional crashes we observe after
applying the mitigation from #849.
emaxx-google added a commit that referenced this issue Jul 27, 2023
Cherry-pick the "Check num_altsetting before dereffing altsetting"
commit in the CCID Driver repository.

This is supposed to fix the occasional crashes we observe after
applying the mitigation from #849.
emaxx-google added a commit that referenced this issue Jul 27, 2023
Cherry-pick the "Check num_altsetting before dereffing altsetting"
commit in the CCID Driver repository.

This is supposed to fix the occasional crashes we observe after
applying the mitigation from #849.
emaxx-google added a commit that referenced this issue Jul 27, 2023
Add a new test for the Smart Card Connector's PC/SC implementation
that's parameterized by the simulated reader type. This lets us test
some basic scenarios work correctly for a range of different devices.

This is a regression test for #849, because one of the simulated devices
used by the new test has the smart card interface number greater than
zero.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant