Merge pull request #97 from LedgerHQ/better-commands-parsing

Improvements in commands parsing support

ledger_device_sdk/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ledger_device_sdk"
-version = "1.1.0"
+version = "1.2.0"
 authors = ["yhql", "yogh333"]
 edition = "2021"
 license.workspace = true

ledger_device_sdk/src/io.rs

@@ -7,7 +7,7 @@ use ledger_secure_sdk_sys::*;
 #[cfg(feature = "ccid")]
 use crate::ccid;
 use crate::seph;
-use core::convert::TryFrom;
+use core::convert::{Infallible, TryFrom};
 use core::ops::{Index, IndexMut};
 
 #[derive(Copy, Clone)]
@@ -74,6 +74,15 @@ impl From<SyscallError> for Reply {
     }
 }
 
+// Needed because some methods use `TryFrom<ApduHeader>::Error`, and for `ApduHeader` we have
+// `Error` as `Infallible`. Since we need to convert such error in a status word (`Reply`) we need
+// to implement this trait here.
+impl From<Infallible> for Reply {
+    fn from(value: Infallible) -> Self {
+        Reply(0x9000)
+    }
+}
+
 extern "C" {
     pub fn io_usb_hid_send(
         sndfct: unsafe extern "C" fn(*mut u8, u16),
@@ -176,53 +185,32 @@ impl Comm {
 
     /// Wait and return next button press event or APDU command.
     ///
-    /// `T` can be an integer (usually automatically infered), which matches the
-    /// Instruction byte of the APDU. In a more complex form, `T` can be any
-    /// type which implements `TryFrom<u8>`. In particular, it is recommended to
-    /// use an enumeration to enforce the compiler checking all possible
-    /// commands are handled. Also, this method will automatically respond with
-    /// an error status word if the Instruction byte is invalid (i.e. `try_from`
-    /// failed).
-    ///
-    /// # Examples
-    ///
-    /// Simple use case with `T` infered as an `i32`:
+    /// `T` can be any type built from a [`ApduHeader`] using the [`TryFrom<ApduHeader>`] trait.
+    /// The conversion can embed complex parsing logic, including checks on CLA, INS, P1 and P2
+    /// bytes, and may return an error with a status word for invalid APDUs.
     ///
-    /// ```
-    /// loop {
-    ///     match comm.next_event() {
-    ///         Event::Button(button) => { ... }
-    ///         Event::Command(0xa4) => { ... }
-    ///         Event::Command(0xb0) => { ... }
-    ///         _ => { comm.reply(StatusWords::BadCLA) }
-    ///     }
-    /// }
-    /// ```
+    /// In particular, it is recommended to use an enumeration for the possible INS values.
     ///
-    /// More complex example with an enumeration:
+    /// # Examples
     ///
     /// ```
     /// enum Instruction {
     ///     Select,
     ///     ReadBinary
     /// }
     ///
-    /// impl TryFrom<u8> for Instruction {
-    ///     type Error = ();
+    /// impl TryFrom<ApduHeader> for Instruction {
+    ///     type Error = StatusWords;
     ///
-    ///     fn try_from(v: u8) -> Result<Self, Self::Error> {
-    ///         match v {
+    ///     fn try_from(h: ApduHeader) -> Result<Self, Self::Error> {
+    ///         match h.ins {
     ///             0xa4 => Ok(Self::Select),
     ///             0xb0 => Ok(Self::ReadBinary)
-    ///             _ => Err(())
+    ///             _ => Err(StatusWords::BadIns)
     ///         }
     ///     }
     /// }
-    /// ```
     ///
-    /// Which can be used as the following:
-    ///
-    /// ```
     /// loop {
     ///     match comm.next_event() {
     ///         Event::Button(button) => { ... }
@@ -231,10 +219,11 @@ impl Comm {
     ///     }
     /// }
     /// ```
-    ///
-    /// In this later example, invalid instruction byte error handling is
-    /// automatically performed by the `next_event` method itself.
-    pub fn next_event<T: TryFrom<ApduHeader>>(&mut self) -> Event<T> {
+    pub fn next_event<T>(&mut self) -> Event<T>
+    where
+        T: TryFrom<ApduHeader>,
+        Reply: From<<T as TryFrom<ApduHeader>>::Error>,
+    {
         let mut spi_buffer = [0u8; 128];
 
         unsafe {
@@ -259,10 +248,11 @@ impl Comm {
         }
     }
 
-    pub fn decode_event<T: TryFrom<ApduHeader>>(
-        &mut self,
-        spi_buffer: &mut [u8; 128],
-    ) -> Option<Event<T>> {
+    pub fn decode_event<T>(&mut self, spi_buffer: &mut [u8; 128]) -> Option<Event<T>>
+    where
+        T: TryFrom<ApduHeader>,
+        Reply: From<<T as TryFrom<ApduHeader>>::Error>,
+    {
         // message = [ tag, len_hi, len_lo, ... ]
         let tag = spi_buffer[0];
         let len = u16::from_be_bytes([spi_buffer[1], spi_buffer[2]]);
@@ -308,56 +298,72 @@ impl Comm {
 
         if unsafe { G_io_app.apdu_state } != APDU_IDLE && unsafe { G_io_app.apdu_length } > 0 {
             self.rx = unsafe { G_io_app.apdu_length as usize };
+
+            // Reject incomplete APDUs
+            if self.rx < 4 {
+                self.reply(StatusWords::BadLen);
+                return None;
+            }
+
+            // Check for data length by using `get_data`
+            if let Err(sw) = self.get_data() {
+                self.reply(sw);
+                return None;
+            }
+
             let res = T::try_from(*self.get_apdu_metadata());
             match res {
                 Ok(ins) => {
                     return Some(Event::Command(ins));
                 }
-                Err(_) => {
+                Err(sw) => {
                     // Invalid Ins code. Send automatically an error, mask
                     // the bad instruction to the application and just
                     // discard this event.
-                    self.reply(StatusWords::BadIns);
+                    self.reply(sw);
                 }
             }
         }
         None
     }
 
-    /// Wait for the next Command event. Returns the APDU Instruction byte value
-    /// for easy instruction matching. Discards received button events.
+    /// Wait for the next Command event. Discards received button events.
     ///
-    /// Like `next_event`, `T` can be an integer, an enumeration, or any type
-    /// which implements `TryFrom<u8>`.
+    /// Like `next_event`, `T` can be any type, an enumeration, or any type
+    /// which implements `TryFrom<ApduHeader>`.
     ///
     /// # Examples
     ///
-    /// Simple use case with `T` infered as an `i32`:
-    ///
     /// ```
-    /// loop {
-    ///     match comm.next_command() {
-    ///         0xa4 => { ... }
-    ///         0xb0 => { ... }
-    ///         _ => { ... }
-    ///     }
+    /// enum Instruction {
+    ///     Select,
+    ///     ReadBinary
     /// }
-    /// ```
     ///
-    /// Other example with an enumeration:
+    /// impl TryFrom<ApduHeader> for Instruction {
+    ///     type Error = StatusWords;
+    ///
+    ///     fn try_from(h: ApduHeader) -> Result<Self, Self::Error> {
+    ///         match h.ins {
+    ///             0xa4 => Ok(Self::Select),
+    ///             0xb0 => Ok(Self::ReadBinary)
+    ///             _ => Err(StatusWords::BadIns)
+    ///         }
+    ///     }
+    /// }
     ///
-    /// ```
     /// loop {
     ///     match comm.next_command() {
     ///         Instruction::Select => { ... }
     ///         Instruction::ReadBinary => { ... }
     ///     }
     /// }
     /// ```
-    ///
-    /// In this later example, invalid instruction byte error handling is
-    /// automatically performed by the `next_command` method itself.
-    pub fn next_command<T: TryFrom<ApduHeader>>(&mut self) -> T {
+    pub fn next_command<T>(&mut self) -> T
+    where
+        T: TryFrom<ApduHeader>,
+        Reply: From<<T as TryFrom<ApduHeader>>::Error>,
+    {
         loop {
             if let Event::Command(ins) = self.next_event() {
                 return ins;

ledger_device_sdk/src/ui/gadgets.rs

@@ -2,7 +2,10 @@
 
 use core::str::from_utf8;
 
-use crate::{buttons::ButtonEvent::*, io};
+use crate::{
+    buttons::ButtonEvent::*,
+    io::{self, ApduHeader, Reply},
+};
 use ledger_secure_sdk_sys::{
     buttons::{get_button_event, ButtonEvent, ButtonsState},
     seph,
@@ -491,8 +494,8 @@ impl<'a> Page<'a> {
     }
 }
 
-pub enum EventOrPageIndex {
-    Event(io::Event<io::ApduHeader>),
+pub enum EventOrPageIndex<T> {
+    Event(io::Event<T>),
     Index(usize),
 }
 
@@ -506,7 +509,10 @@ impl<'a> MultiPageMenu<'a> {
         MultiPageMenu { comm, pages }
     }
 
-    pub fn show(&mut self) -> EventOrPageIndex {
+    pub fn show<T: TryFrom<ApduHeader>>(&mut self) -> EventOrPageIndex<T>
+    where
+        Reply: From<<T as TryFrom<ApduHeader>>::Error>,
+    {
         clear_screen();
 
         self.pages[0].place();

ledger_device_sdk/src/uxapp.rs

@@ -1,6 +1,7 @@
 use ledger_secure_sdk_sys::seph as sys_seph;
 use ledger_secure_sdk_sys::*;
 
+use crate::io::Reply;
 use crate::io::{ApduHeader, Comm, Event};
 
 pub use ledger_secure_sdk_sys::BOLOS_UX_CANCEL;
@@ -66,7 +67,11 @@ impl UxEvent {
         ret
     }
 
-    pub fn block_and_get_event<T: TryFrom<ApduHeader>>(comm: &mut Comm) -> (u32, Option<Event<T>>) {
+    pub fn block_and_get_event<T>(comm: &mut Comm) -> (u32, Option<Event<T>>)
+    where
+        T: TryFrom<ApduHeader>,
+        Reply: From<<T as TryFrom<ApduHeader>>::Error>,
+    {
         let mut ret = unsafe { os_sched_last_status(TASK_BOLOS_UX as u32) } as u32;
         let mut event = None;
         while ret == BOLOS_UX_IGNORE || ret == BOLOS_UX_CONTINUE {