Fix touch responsiveness and USB HID issues#777
Conversation
pwnall
force-pushed
the
fix-artemis-touch-usb
branch
from
acf9e87 to
a05c40b
Compare
|
Coverage is 97.274% — pwnall:fix-artemis-touch-usb into google:develop. No base build found for |
pwnall
force-pushed
the
fix-artemis-touch-usb
branch
2 times, most recently
from
3065d91 to
f64ddf4
Compare
pwnall
changed the title
ia0
left a comment
ia0
left a comment
There was a problem hiding this comment.
Thanks for the PR! I'll only comment on the Touch issue and let @kaczmarczyck address the second point.
| if let Some(state) = this.take() { | ||
| state.button.stop(); | ||
| state.touched.store(true, Relaxed); | ||
| } |
There was a problem hiding this comment.
We should not reach unreachable!(). I wonder if you had to add this because you unconditionally reset the STATE in Drop for Touch. In theory, state.button.stop() will start dropping future events and clear any existing pending events. If that's not the case it's a bug in Wasefire.
That said, the applet could be resilient against Wasefire bugs. I don't think it's a good idea because (at least for development) we want to fail as soon as something impossible happens. We could have a resilient cargo feature to avoid crashes as much as possible, which would be used for production keys (there could be security concerns though, it's a trade-off with availability). @kaczmarczyck what do you think?
There was a problem hiding this comment.
Thank you very much for explaining!
I removed this diff. I don't want to make development more difficult 😄
There was a problem hiding this comment.
We should double check how we deal with parallel connections, and that we don't miss anything else. We didn't test enough since the Wasefire refactoring.
Touches should never be used twice. CTAP2 library code should already ensure this, but the combination with CTAP1 can work in one direction: First requesting on CTAP1, then going into the CTAP2 UP loop. We should drop CTAP1 then. But doesn't need to be done here.
There was a problem hiding this comment.
And to answer the question, once it is impossible, we can fail when the impossible happens.
There was a problem hiding this comment.
Thank you very much for your thoughts!
I'll try to address this in a separate PR.
There was a problem hiding this comment.
I don't think there's anything to do here in the short term:
- We need to check whether OpenSK can create 2 Touch objects. If no, simplify the code, but I can do it.
- We need to run enough tests to make sure the unreachable is correct.
pwnall
force-pushed
the
fix-artemis-touch-usb
branch
from
f64ddf4 to
ff90a86
Compare
ia0
left a comment
ia0
left a comment
There was a problem hiding this comment.
LGTM for src/touch.rs. Is there a reason the PR is still a draft?
kaczmarczyck
left a comment
kaczmarczyck
left a comment
There was a problem hiding this comment.
Approving for HID library code
This commit addresses responsiveness issues and CTAP protocol deviations in the USB HID transport and user presence handling: 1. User Presence State Leak: When a user presence request is interrupted (e.g., by a timeout or an incoming USB packet), the touch state was incorrectly left running in the background. This prevented subsequent LED blinking and caused a panic if hardware interrupts (bounces) were queued. The `Touch` struct now correctly implements `Drop` to clear the state, and event handling safely ignores stale interrupts. 2. Transaction Timeout Race Condition: Following a transaction timeout (CTAP 2.1 Section 11.2.5.2), the firmware incorrectly dropped the next incoming packet while returning a `MsgTimeout` error. If the host initiated a new transaction with a `CTAPHID_INIT` packet immediately after a timeout, the new initialization packet was discarded, leaving the host waiting indefinitely. The timeout logic now preserves new initialization packets and only returns a timeout error if the host sends a continuation packet for the aborted message.
pwnall
force-pushed
the
fix-artemis-touch-usb
branch
from
ff90a86 to
5f347c4
Compare
|
@ia0 @kaczmarczyck PTAL? Apologies if I was supposed to leave the PR as-is rather than add the tests. |
No I think that's fine. I'll let @kaczmarczyck merge. |
4 participants
This PR addresses two responsiveness issues that may cause the device to hang or drop connections:
DropforTouchto correctly clear theSTATEwhen a user presence request is interrupted (e.g., by a USB packet or timeout). Added safe extraction inState::touchto handle queued hardware button interrupts (bounces) without hittingunreachable!()and panicking.MessageAssembler::parse_packetto allow any valid initialization packet (not justCTAPHID_INIT) to abort a hung transaction, in accordance with the CTAP 2.1 specification.MsgTimeouterror.These fixes prevent the firmware from entering unrecoverable states where the LED fails to blink or the device stops responding to the host.