From d752df294ef8b1be65a25388b46e2c24c2854aca Mon Sep 17 00:00:00 2001 From: Sergiy Yevtushenko Date: Mon, 25 Mar 2024 14:41:01 +0100 Subject: [PATCH] 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.lock | 6 +++--- Cargo.toml | 2 +- ragger_tests/conftest.py | 19 ------------------- ragger_tests/test_get_app_version.py | 2 +- sbor/src/print/primitives.rs | 7 +++++-- src/app_error.rs | 1 + src/crypto/secp256k1.rs | 14 ++++++-------- src/io.rs | 13 +++++++++++++ src/main.rs | 22 +--------------------- test/test-get-application-version.py | 2 +- 10 files changed, 32 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 85a58b2e..81f07c5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19,7 +19,7 @@ checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa" [[package]] name = "babylon-ledger-app" -version = "0.7.27" +version = "0.7.28" dependencies = [ "blake2", "include_gif", @@ -218,9 +218,9 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55" [[package]] name = "ledger_device_sdk" -version = "1.5.1" +version = "1.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e5b4725e4e236f13bb5a55dadee88cff36faacea738707cc44aff4ff6f1ab96e" +checksum = "8c119b704c1240c3e21fcaff94ea8cdc110ac949eb118c5e6e5dbc9c7a61ebe2" dependencies = [ "include_gif", "ledger_secure_sdk_sys", diff --git a/Cargo.toml b/Cargo.toml index c249face..3fa2923d 100644 --- a/Cargo.toml +++ b/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" diff --git a/ragger_tests/conftest.py b/ragger_tests/conftest.py index bcd2b4e4..51cdbe22 100644 --- a/ragger_tests/conftest.py +++ b/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) diff --git a/ragger_tests/test_get_app_version.py b/ragger_tests/test_get_app_version.py index 9e981f85..a1f80940 100644 --- a/ragger_tests/test_get_app_version.py +++ b/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" diff --git a/sbor/src/print/primitives.rs b/sbor/src/print/primitives.rs index c077fe93..f4ffd388 100644 --- a/sbor/src/print/primitives.rs +++ b/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; } } diff --git a/src/app_error.rs b/src/app_error.rs index bcaa8bbc..c4b4297f 100644 --- a/src/app_error.rs +++ b/src/app_error.rs @@ -17,6 +17,7 @@ pub enum AppError { BadP1P2 = 0x6e02, BadLen = 0x6e03, UserCancelled = 0x6e04, + BadSDKResponse = 0x6e05, BadBip32PathLen = 0x6e10, BadBip32PathDataLen = 0x6e11, diff --git a/src/crypto/secp256k1.rs b/src/crypto/secp256k1.rs index f64624de..ca07b5c3 100644 --- a/src/crypto/secp256k1.rs +++ b/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); diff --git a/src/io.rs b/src/io.rs index bb047c9e..a1115884 100644 --- a/src/io.rs +++ b/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) => { diff --git a/src/main.rs b/src/main.rs index c33dc5be..d6532e23 100644 --- a/src/main.rs +++ b/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::() { - Event::Button(ButtonEvent::BothButtonsRelease) => { - break; - } - Event::Command(_) => { - comm.reply(AppError::CxErrorNotUnlocked); - } - _ => {} - } - } -} diff --git a/test/test-get-application-version.py b/test/test-get-application-version.py index a181a8ac..f5c67142 100755 --- a/test/test-get-application-version.py +++ b/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")