Repository: google/OpenSK
File: src/ctap/mod.rs (Rust)

Code before:
    let cbor_public_key = cbor_read(public_key_bytes)?;
    let cose_key = CoseKey::try_from(cbor_public_key)?;
    let public_key = ecdsa::PubKey::try_from(cose_key)?;
    if !public_key.verify_hash_vartime(signed_hash, &signature) {
        return Err(Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE);
    }
    Ok(())
}

// Sends keepalive packet during user presence checking. If user agent replies with CANCEL response,
// returns Err(UserPresenceError::Canceled).
fn send_keepalive_up_needed(
    env: &mut impl Env,
    channel: Channel,
    timeout: Milliseconds<ClockInt>,
) -> Result<(), UserPresenceError> {
    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::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) {
            Ok(SendOrRecvStatus::Timeout) => {
                debug_ctap!(env, "Sending a KEEPALIVE packet timed out");
                // TODO: abort user presence test?
            }
            Err(_) => panic!("Error sending KEEPALIVE packet"),
            Ok(SendOrRecvStatus::Sent) => {
                debug_ctap!(env, "Sent KEEPALIVE packet");
            }
            Ok(SendOrRecvStatus::Received(_)) => {
                // TODO(liamjm): Support receiving packets on both interfaces.
                // We only parse one packet, because we only care about CANCEL.
                let (received_cid, processed_packet) = CtapHid::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);

Review comment (security):
Maybe we have to add more HID tests? The most common situation with unexpected OUT packets (and therefore OUT and IN at the same time) is probably a CANCEL during user presence checks. Should be something like a MakeCredential request, and while user presence is checked, spam it with packets, and see if those are treated correctly.

Code after:
        ecdsa::Signature::try_from(signature.ok_or(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)?)?;
    let cbor_public_key = cbor_read(public_key_bytes)?;
    let cose_key = CoseKey::try_from(cbor_public_key)?;
    let public_key = ecdsa::PubKey::try_from(cose_key)?;
    if !public_key.verify_hash_vartime(signed_hash, &signature) {
        return Err(Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE);
    }
    Ok(())
}

// Sends keepalive packet during user presence checking. If user agent replies with CANCEL response,
// returns Err(UserPresenceError::Canceled).
fn send_keepalive_up_needed(
    env: &mut impl Env,
    channel: Channel,
    timeout: Milliseconds<ClockInt>,
) -> Result<(), UserPresenceError> {
    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::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) {
            Ok(SendOrRecvStatus::Timeout) => {
                debug_ctap!(env, "Sending a KEEPALIVE packet timed out");
                // TODO: abort user presence test?
            }
            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;
                }