Audit (#71)

* Reject incomplete APDUs (#65)
* Bump SDK version (#66)
* Prevent buffer underflow
* Fix index check.
* Validate Secp256k1 DER signature format received from SDK
* Fix checking of start indexes
* Remove "Pending review" banner.

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "babylon-ledger-app"
-version = "0.7.27"
+version = "0.7.28"
 authors = ["siy"]
 edition = "2021"
 description = "Radix Babylon"

ragger_tests/conftest.py

@@ -16,22 +16,3 @@
 
 # Pull all features from the base ragger conftest using the overridden configuration
 pytest_plugins = ("ragger.conftest.base_conftest", )
-
-# Notes :
-# 1. Remove this fixture once the pending review screen is removed from the app
-# 2. This fixture clears the pending review screen before each test
-# 3. The scope should be the same as the one configured by BACKEND_SCOPE in 
-# ragger/conftest/configuration.py
-@pytest.fixture(scope="class", autouse=True)
-def clear_pending_review(firmware, navigator):
-    print("Clearing pending review")
-    # Press a button to clear the pending review
-    if firmware.device.startswith("nano"):
-        instructions = [
-            NavInsID.BOTH_CLICK,
-        ]
-    else:
-        instructions = [
-            NavInsID.TAPPABLE_CENTER_TAP,
-        ]
-    navigator.navigate(instructions,screen_change_before_first_instruction=False)

ragger_tests/test_get_app_version.py

@@ -3,4 +3,4 @@
 
 
 def test_get_version(backend):
-    assert backend.exchange(cla=CLA, ins=INS).data.hex() == "00071b"
+    assert backend.exchange(cla=CLA, ins=INS).data.hex() == "00071c"

sbor/src/print/primitives.rs

@@ -128,14 +128,14 @@ macro_rules! ixx {
             unsafe { ptr.add(i).write((n % 10) as u8 + b'0') }
             n /= 10;
 
-            if n == 0 {
+            if n == 0 || i == 0 {
                 break;
             } else {
                 i -= 1;
             }
         }
 
-        if negative {
+        if negative && i > 0 {
             i -= 1;
             unsafe { ptr.add(i).write(b'-') }
         }
@@ -157,6 +157,9 @@ macro_rules! uxx {
             if n == 0 {
                 break;
             } else {
+                if i == 0 {
+                    break;
+                }
                 i -= 1;
             }
         }

src/app_error.rs

@@ -17,6 +17,7 @@ pub enum AppError {
     BadP1P2 = 0x6e02,
     BadLen = 0x6e03,
     UserCancelled = 0x6e04,
+    BadSDKResponse = 0x6e05,
 
     BadBip32PathLen = 0x6e10,
     BadBip32PathDataLen = 0x6e11,

src/crypto/secp256k1.rs

@@ -22,6 +22,7 @@ const PRIV_KEY_LEN: usize = 32;
 const PUB_KEY_X_COORDINATE_SIZE: usize = 32;
 const PUB_KEY_UNCOMPRESSED_LAST_BYTE: usize = 64;
 const DER_MAX_LEN: usize = 72;
+const MAX_DER_OFFSET: usize = DER_MAX_LEN - 32;
 pub const SECP256K1_SIGNATURE_LEN: usize = 65;
 pub const SECP256K1_PUBLIC_KEY_LEN: usize = PUB_KEY_COMPRESSED_LEN;
 
@@ -100,26 +101,23 @@ impl KeyPairSecp256k1 {
             let r_len = comm.work_buffer[index_r_len] as usize;
             let mut r_start = index_r_len + 1;
             let index_s_len = r_start + r_len + 1;
-            let s_len = comm.work_buffer[index_s_len] as usize;
             let s_start = index_s_len + 1;
+
             if r_len == 33 {
                 // we skip first byte of R.
                 r_start += 1;
             }
 
-            // +4 for `02`, `Lr`, `02` and `Ls`.
-            assert_eq!(
-                r_len + s_len + 4,
-                (comm.work_buffer[1] as usize),
-                "Parsed S_len + R_len should equal 'L' + 4, but it did not"
-            );
-
             let parity = if (info & CX_ECCINFO_PARITY_ODD) != 0 {
                 0x01u8
             } else {
                 0x00
             };
 
+            if r_start > MAX_DER_OFFSET || s_start > MAX_DER_OFFSET {
+                return Err(AppError::BadSDKResponse);
+            }
+
             comm.append(&[parity]);
             comm.append_work_buffer_from_to(r_start, r_start + 32);
             comm.append_work_buffer_from_to(s_start, s_start + 32);

src/io.rs

@@ -232,6 +232,19 @@ 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) => {

src/main.rs

@@ -11,12 +11,10 @@ use ledger_device_sdk::ui::bagls::{
     CERTIFICATE_ICON, COGGLE_ICON, DASHBOARD_X_ICON, PROCESSING_ICON,
 };
 use ledger_device_sdk::ui::gadgets::clear_screen;
-use ledger_secure_sdk_sys::buttons::ButtonEvent;
 
 use handler::dispatcher;
 
 use crate::app_error::AppError;
-use crate::command::Command;
 use crate::io::{Comm, Event, UxEvent};
 use crate::ledger_display_io::LedgerTTY;
 use crate::settings::Settings;
@@ -170,7 +168,7 @@ extern "C" fn sample_main() {
     let mut main_menu = Menu::new(&menu);
     let mut ticker = 0i8;
 
-    display_pending_review(&mut comm);
+    core::intrinsics::black_box(&mut comm);
 
     main_menu.display();
 
@@ -212,21 +210,3 @@ extern "C" fn sample_main() {
         }
     }
 }
-
-fn display_pending_review(comm: &mut Comm) {
-    clear_screen();
-
-    ledger_device_sdk::ui::gadgets::SingleMessage::new("Pending Review").show();
-
-    loop {
-        match comm.next_event::<Command>() {
-            Event::Button(ButtonEvent::BothButtonsRelease) => {
-                break;
-            }
-            Event::Command(_) => {
-                comm.reply(AppError::CxErrorNotUnlocked);
-            }
-            _ => {}
-        }
-    }
-}

test/test-get-application-version.py

@@ -23,5 +23,5 @@
 print("Testing", "GetAppVersion", instructionCode, end=" ")
 response = dongle.exchange(bytes.fromhex(instructionClass + instructionCode + p1 + p2 + dataLength))
 
-assert response.hex() == '00071b', "Invalid version\nReceived:" + response.hex()
+assert response.hex() == '00071c', "Invalid version\nReceived:" + response.hex()
 print("Success")