Refactors HID connections to support busy response (#699)

* Refactors HID connections to support busy response

The main intended difference is that OpenSK now responds with ERR_CHANNEL_BUSY
when it receives a packet while waiting for touch in in a CTAP2 command.

To support this more elegantly, we changed `HidConnection` to have a
`send` and `recv` instead of `send_and_maybe_recv` for a cleaner API.
This also resolves an older TODO to respond to incoming channels on
the other USB endpoint.

It doesn't process the incoming traffic correctly, but unblock the host
at least, and tells it to come back later. We still only allow one
active channel at the same time.

We now don't need the `TRANSMIT_AND_RECEIVE` syscall anymore. I didn't
remove it from the Tock pack for simplicity, but cleaned up
libtock-drivers.

The Env API changes from having one connection per endpoint, to having
one send function that takes an endpoint, and a receive function that
receives on all endpoints at the same time.
The `HidConnection` already received on all endpoints before, which was
inconsistent with the existance of, e.g., `VendorHidConnection` being
able to receive on the main endpoint.

Since we now have a cleaner send and receive API, we use this in
`src/main.rs` and simplify it a bit, while making it more consistent
with other calls to the HID API.

I found an additional inaccuracy in our implementation while
refactoring: We want to send keepalives every 100 ms. To do so, we first
wait for a button callback for 100 ms. Then we send the keepalive and
check if a cancel packet appeared. What should have happened instead is
that we listen for HID packets and button presses at the same time
during these 100 ms. If nothing happens that stops the loop, we send the
keepalive.

The old implementation would, in some implementations, wait 200 ms for
each keepalive: First 100 for touch, then 100 for `send_and_maybe_receive`.

The new implementation can loop faster in case there is a lot of unrelated
HID traffic. To make the touch timeout last 30 seconds, we introduce an
extra timer and loop until time is up. This might still make us blink
the LEDs too fast, but that is already the case in the main loop, and
generally would need a bigger refactoring.
The only simple workaround to make the LEDs slightly more precise is to
wait for touch and packets until it is time to flip the LEDs, and if we
return too early within some thresholds, wait again. If we care enough,
we can fix that in a later PR.

Not sure how to make the test work that I removed in ctap/mod.rs. I'd
need to advance the time while the loop is running. Setting a user
presence function that advances time seems hard with the Rust borrowing
rules.

* Refactors status and error codes

There are a few small logic fixes hidden in this comit:

- We always call `check_complete`, even if we received a cancel.
- We only accept Cancel packets from the active channel.
- Send timeouts now return `Ctap2StatusCode::CTAP1_ERR_TIMEOUT`.

* Stable blinking pattern

Before, if the device gets a CTAP2 UP request and another channel sends
packets, the blinking pattern would have sped up.

The patch for the Tock USB implementation fixes a panic after a timeout
in send.

* Better TestClock API

libraries/opensk/src/api/connection.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use crate::ctap::status_code::{Ctap2StatusCode, CtapResult};
 use core::convert::TryFrom;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
@@ -22,31 +23,26 @@ pub enum UsbEndpoint {
 }
 
 impl TryFrom<usize> for UsbEndpoint {
-    type Error = SendOrRecvError;
+    type Error = Ctap2StatusCode;
 
-    fn try_from(endpoint_num: usize) -> Result<Self, SendOrRecvError> {
+    fn try_from(endpoint_num: usize) -> CtapResult<Self> {
         match endpoint_num {
             1 => Ok(UsbEndpoint::MainHid),
             #[cfg(feature = "vendor_hid")]
             2 => Ok(UsbEndpoint::VendorHid),
-            _ => Err(SendOrRecvError),
+            _ => Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE),
         }
     }
 }
 
-pub enum SendOrRecvStatus {
+pub enum RecvStatus {
     Timeout,
-    Sent,
     Received(UsbEndpoint),
 }
 
-#[derive(Debug, PartialEq, Eq)]
-pub struct SendOrRecvError;
-
-pub type SendOrRecvResult = Result<SendOrRecvStatus, SendOrRecvError>;
-
 pub trait HidConnection {
-    fn send_and_maybe_recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> SendOrRecvResult;
+    fn send(&mut self, buf: &[u8; 64], endpoint: UsbEndpoint) -> CtapResult<()>;
+    fn recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> CtapResult<RecvStatus>;
 }
 
 #[cfg(test)]
@@ -58,6 +54,9 @@ mod test {
         assert_eq!(UsbEndpoint::try_from(1), Ok(UsbEndpoint::MainHid));
         #[cfg(feature = "vendor_hid")]
         assert_eq!(UsbEndpoint::try_from(2), Ok(UsbEndpoint::VendorHid));
-        assert_eq!(UsbEndpoint::try_from(3), Err(SendOrRecvError));
+        assert_eq!(
+            UsbEndpoint::try_from(3),
+            Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE)
+        );
     }
 }

libraries/opensk/src/api/user_presence.rs

@@ -12,6 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use crate::api::connection::RecvStatus;
+use crate::ctap::status_code::CtapResult;
+
 #[derive(Debug)]
 pub enum UserPresenceError {
     /// User explicitly declined user presence check.
@@ -20,12 +23,11 @@ pub enum UserPresenceError {
     Canceled,
     /// User presence check timed out.
     Timeout,
-    /// Unexpected (e.g., hardware) failures
-    Fail,
 }
-
 pub type UserPresenceResult = Result<(), UserPresenceError>;
 
+pub type UserPresenceWaitResult = CtapResult<(UserPresenceResult, RecvStatus)>;
+
 pub trait UserPresence {
     /// Initializes for a user presence check.
     ///
@@ -35,7 +37,13 @@ pub trait UserPresence {
     /// Waits until user presence is confirmed, rejected, or the given timeout expires.
     ///
     /// Must be called between calls to [`Self::check_init`] and [`Self::check_complete`].
-    fn wait_with_timeout(&mut self, timeout_ms: usize) -> UserPresenceResult;
+    /// The function may write to the packet buffer, if it receives one during the wait.
+    /// If it does, the returned option contains the endpoint it received the data from.
+    fn wait_with_timeout(
+        &mut self,
+        packet: &mut [u8; 64],
+        timeout_ms: usize,
+    ) -> UserPresenceWaitResult;
 
     /// Finalizes a user presence check.
     ///

libraries/opensk/src/ctap/hid/mod.rs

@@ -440,6 +440,11 @@ impl<E: Env> CtapHid<E> {
         })
     }
 
+    /// Generates the HID response packets for a busy error.
+    pub fn busy_error(cid: ChannelID) -> HidPacketIterator {
+        Self::split_message(Self::error_message(cid, CtapHidError::ChannelBusy))
+    }
+
     #[cfg(test)]
     pub fn new_initialized() -> (Self, ChannelID) {
         (
@@ -633,6 +638,25 @@ mod test {
         }
     }
 
+    #[test]
+    fn test_busy_error() {
+        let cid = [0x12, 0x34, 0x56, 0x78];
+        let mut response = CtapHid::<TestEnv>::busy_error(cid);
+        let mut expected_packet = [0x00; 64];
+        expected_packet[..8].copy_from_slice(&[
+            0x12,
+            0x34,
+            0x56,
+            0x78,
+            0xBF,
+            0x00,
+            0x01,
+            CtapHidError::ChannelBusy as u8,
+        ]);
+        assert_eq!(response.next(), Some(expected_packet));
+        assert_eq!(response.next(), None);
+    }
+
     #[test]
     fn test_process_single_packet() {
         let cid = [0x12, 0x34, 0x56, 0x78];

libraries/opensk/src/ctap/mod.rs

@@ -50,7 +50,9 @@ use self::data_formats::{
     PublicKeyCredentialDescriptor, PublicKeyCredentialParameter, PublicKeyCredentialSource,
     PublicKeyCredentialType, PublicKeyCredentialUserEntity, SignatureAlgorithm,
 };
-use self::hid::{ChannelID, CtapHid, CtapHidCommand, KeepaliveStatus, ProcessedPacket};
+use self::hid::{
+    ChannelID, CtapHid, CtapHidCommand, HidPacketIterator, KeepaliveStatus, ProcessedPacket,
+};
 use self::large_blobs::LargeBlobState;
 use self::response::{
     AuthenticatorGetAssertionResponse, AuthenticatorGetInfoResponse,
@@ -61,7 +63,7 @@ use self::status_code::{Ctap2StatusCode, CtapResult};
 #[cfg(feature = "with_ctap1")]
 use self::u2f_up::U2fUserPresenceState;
 use crate::api::clock::Clock;
-use crate::api::connection::{HidConnection, SendOrRecvStatus, UsbEndpoint};
+use crate::api::connection::{HidConnection, RecvStatus, UsbEndpoint};
 use crate::api::crypto::ecdsa::{SecretKey as _, Signature};
 use crate::api::crypto::hkdf256::Hkdf256;
 use crate::api::crypto::sha256::Sha256;
@@ -149,11 +151,11 @@ pub enum Transport {
 }
 
 impl Transport {
-    pub fn hid_connection<E: Env>(self, env: &mut E) -> &mut E::HidConnection {
+    pub fn usb_endpoint(self) -> UsbEndpoint {
         match self {
-            Transport::MainHid => env.main_hid_connection(),
+            Transport::MainHid => UsbEndpoint::MainHid,
             #[cfg(feature = "vendor_hid")]
-            Transport::VendorHid => env.vendor_hid_connection(),
+            Transport::VendorHid => UsbEndpoint::VendorHid,
         }
     }
 }
@@ -255,124 +257,91 @@ fn truncate_to_char_boundary(s: &str, mut max: usize) -> &str {
     }
 }
 
-// Sends keepalive packet during user presence checking. If user agent replies with CANCEL response,
-// returns Err(UserPresenceError::Canceled).
-fn send_keepalive_up_needed<E: Env>(
+/// Send non-critical packets using fire-and-forget.
+fn send_packets<E: Env>(
     env: &mut E,
-    channel: Channel,
-    timeout_ms: usize,
-) -> Result<(), UserPresenceError> {
+    endpoint: UsbEndpoint,
+    packets: HidPacketIterator,
+) -> CtapResult<()> {
+    for pkt in packets {
+        env.hid_connection().send(&pkt, endpoint)?;
+    }
+    Ok(())
+}
+
+fn is_cancel(packet: &ProcessedPacket) -> bool {
+    match packet {
+        ProcessedPacket::InitPacket { cmd, .. } => *cmd == CtapHidCommand::Cancel as u8,
+        ProcessedPacket::ContinuationPacket { .. } => false,
+    }
+}
+
+fn wait_and_respond_busy<E: Env>(env: &mut E, channel: Channel) -> CtapResult<()> {
     let (cid, transport) = match channel {
         Channel::MainHid(cid) => (cid, Transport::MainHid),
         #[cfg(feature = "vendor_hid")]
         Channel::VendorHid(cid) => (cid, Transport::VendorHid),
     };
-    let keepalive_msg = CtapHid::<E>::keepalive(cid, KeepaliveStatus::UpNeeded);
-    for mut pkt in keepalive_msg {
-        let ctap_hid_connection = transport.hid_connection(env);
-        match ctap_hid_connection.send_and_maybe_recv(&mut pkt, timeout_ms) {
-            Ok(SendOrRecvStatus::Timeout) => {
-                debug_ctap!(env, "Sending a KEEPALIVE packet timed out");
-                // The client is likely unresponsive, but let's retry.
-            }
-            Err(_) => panic!("Error sending KEEPALIVE packet"),
-            Ok(SendOrRecvStatus::Sent) => {
-                debug_ctap!(env, "Sent KEEPALIVE packet");
-            }
-            Ok(SendOrRecvStatus::Received(endpoint)) => {
-                let rx_transport = match endpoint {
-                    UsbEndpoint::MainHid => Transport::MainHid,
-                    #[cfg(feature = "vendor_hid")]
-                    UsbEndpoint::VendorHid => Transport::VendorHid,
-                };
-                if rx_transport != transport {
-                    debug_ctap!(
-                        env,
-                        "Received a packet on transport {:?} while sending a KEEPALIVE packet on transport {:?}",
-                         rx_transport, transport
-                    );
-                    // Ignore this packet.
-                    // TODO(liamjm): Support receiving packets on both interfaces.
-                    continue;
-                }
-
-                // We only parse one packet, because we only care about CANCEL.
-                let (received_cid, processed_packet) = CtapHid::<E>::process_single_packet(&pkt);
-                if received_cid != cid {
-                    debug_ctap!(
-                        env,
-                        "Received a packet on channel ID {:?} while sending a KEEPALIVE packet",
-                        received_cid,
-                    );
-                    return Ok(());
-                }
-                match processed_packet {
-                    ProcessedPacket::InitPacket { cmd, .. } => {
-                        if cmd == CtapHidCommand::Cancel as u8 {
-                            // We ignore the payload, we can't answer with an error code anyway.
-                            debug_ctap!(env, "User presence check cancelled");
-                            return Err(UserPresenceError::Canceled);
-                        } else {
-                            debug_ctap!(
-                                env,
-                                "Discarded packet with command {} received while sending a KEEPALIVE packet",
-                                cmd,
-                            );
-                        }
-                    }
-                    ProcessedPacket::ContinuationPacket { .. } => {
-                        debug_ctap!(
-                            env,
-                            "Discarded continuation packet received while sending a KEEPALIVE packet",
-                        );
-                    }
-                }
+    let endpoint = transport.usb_endpoint();
+
+    let mut packet = [0; 64];
+    let (up_status, recv_status) = env
+        .user_presence()
+        .wait_with_timeout(&mut packet, KEEPALIVE_DELAY_MS)?;
+    match recv_status {
+        RecvStatus::Timeout => (),
+        RecvStatus::Received(rx_endpoint) => {
+            let (received_cid, processed_packet) = CtapHid::<E>::process_single_packet(&packet);
+            if rx_endpoint != endpoint || received_cid != cid {
+                debug_ctap!(
+                    env,
+                    "Received an unrelated packet on endpoint {:?} while sending a KEEPALIVE packet",
+                     rx_endpoint,
+                );
+                let busy_error = CtapHid::<E>::busy_error(received_cid);
+                // Don't send errors from other channels on the active channel.
+                let _ = send_packets(env, rx_endpoint, busy_error);
+            } else if is_cancel(&processed_packet) {
+                // Ignored, CANCEL specification says: "the authenticator MUST NOT reply"
+                debug_ctap!(env, "User presence check cancelled");
+                return Err(UserPresenceError::Canceled.into());
+            } else {
+                // Ignored. A client shouldn't try to talk to us on this channel yet.
+                debug_ctap!(env, "Discarded packet while checking user presence.");
             }
         }
     }
-    Ok(())
+    if matches!(up_status, Err(UserPresenceError::Timeout)) {
+        let keepalive_msg = CtapHid::<E>::keepalive(cid, KeepaliveStatus::UpNeeded);
+        send_packets(env, endpoint, keepalive_msg)?;
+    }
+    up_status.map_err(|e| e.into())
 }
 
 /// Blocks for user presence.
 ///
 /// Returns an error in case of timeout, user declining presence request, or keepalive error.
 pub fn check_user_presence<E: Env>(env: &mut E, channel: Channel) -> CtapResult<()> {
-    env.user_presence().check_init();
+    const TIMEOUT_ERROR: Ctap2StatusCode = Ctap2StatusCode::CTAP2_ERR_USER_ACTION_TIMEOUT;
 
-    // The timeout is N times the keepalive delay.
-    const TIMEOUT_ITERATIONS: usize = TOUCH_TIMEOUT_MS / KEEPALIVE_DELAY_MS;
-
-    // All fallible functions are called without '?' operator to always reach
-    // check_complete(...) cleanup function.
-
-    let mut result = Err(UserPresenceError::Timeout);
-    for i in 0..=TIMEOUT_ITERATIONS {
-        // First presence check is made without timeout. That way Env implementation may return
-        // user presence check result immediately to client, without sending any keepalive packets.
-        result = env
-            .user_presence()
-            .wait_with_timeout(if i == 0 { 0 } else { KEEPALIVE_DELAY_MS });
-        if !matches!(result, Err(UserPresenceError::Timeout)) {
-            break;
-        }
-        // TODO: this may take arbitrary time. Next wait's delay should be adjusted
-        // accordingly, so that all wait_with_timeout invocations are separated by
-        // equal time intervals. That way token indicators, such as LEDs, will blink
-        // with a consistent pattern.
-        let keepalive_result = send_keepalive_up_needed(env, channel, KEEPALIVE_DELAY_MS);
-        if keepalive_result.is_err() {
-            debug_ctap!(
-                env,
-                "Sending keepalive failed with error {:?}",
-                keepalive_result.as_ref().unwrap_err()
-            );
-            result = keepalive_result;
-            break;
+    env.user_presence().check_init();
+    let loop_timer = env.clock().make_timer(TOUCH_TIMEOUT_MS);
+
+    // We don't use the '?' operator to always reach check_complete(...).
+    let mut result = Err(TIMEOUT_ERROR);
+    while !env.clock().is_elapsed(&loop_timer) {
+        match wait_and_respond_busy(env, channel) {
+            Err(TIMEOUT_ERROR) => (),
+            r => {
+                result = r;
+                // We want to break on Ok(()) too, indicating touch.
+                break;
+            }
         }
     }
 
     env.user_presence().check_complete();
-    result.map_err(|e| e.into())
+    result
 }
 
 /// Holds data necessary to sign an assertion for a credential.
@@ -1463,7 +1432,6 @@ mod test {
     use crate::api::crypto::ecdh::SecretKey as _;
     use crate::api::customization;
     use crate::api::key_store::CBOR_CREDENTIAL_ID_SIZE;
-    use crate::api::user_presence::UserPresenceResult;
     use crate::ctap::command::AuthenticatorLargeBlobsParameters;
     use crate::env::test::TestEnv;
     use crate::env::EcdhSk;
@@ -3381,13 +3349,12 @@ mod test {
 
     #[test]
     fn test_check_user_presence_timeout() {
-        // This will always return timeout.
-        fn user_presence_timeout() -> UserPresenceResult {
-            Err(UserPresenceError::Timeout)
-        }
-
         let mut env = TestEnv::default();
-        env.user_presence().set(user_presence_timeout);
+        let clock = env.clock().clone();
+        env.user_presence().set(move || {
+            clock.advance(100);
+            Err(UserPresenceError::Timeout)
+        });
         let response = check_user_presence(&mut env, DUMMY_CHANNEL);
         assert!(matches!(
             response,