Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Stax/Flex: incomplete APDU is returned #35

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Fix Stax/Flex: incomplete APDU is returned #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5387c6f
Use custom NbglReviewDisplay
yogh333 Oct 7, 2024
979099e
Use buffer to store data to be sent back through comm object
yogh333 Oct 7, 2024
d87f01b
Remove comments
yogh333 Oct 7, 2024
a9961f5
Update Rust SDK
yogh333 Oct 8, 2024
de6f214
Remove traces as it makes NanoX crashes
yogh333 Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions starknet/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
[package]
name = "starknet"
version = "2.1.0"
version = "2.1.1"
edition = "2021"
authors = ["Ledger"]

[dependencies]
ledger_device_sdk = "1.17.2"
ledger_device_sdk = "1.17.3"
ledger_secure_sdk_sys = { version = "1.5.0", features = ["heap"]}
include_gif = "1.2.0"
hex = { version = "0.4", default-features = false, features = ["alloc"]}
Expand Down
6 changes: 0 additions & 6 deletions starknet/src/crypto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,7 @@ impl From<CryptoError> for Reply {

/// Helper function that signs with ECDSA in deterministic nonce
pub fn sign_hash(ctx: &mut Ctx) -> Result<(), CryptoError> {
// ledger_device_sdk::testing::debug_print("Before shift: ");
// ledger_device_sdk::testing::debug_print(&ctx.hash.m_hash.to_hex_string());
// ledger_device_sdk::testing::debug_print("\n");
poseidon::poseidon_shift(&mut ctx.hash.m_hash);
// ledger_device_sdk::testing::debug_print("After shift: ");
// ledger_device_sdk::testing::debug_print(&ctx.hash.m_hash.to_hex_string());
// ledger_device_sdk::testing::debug_print("\n");

match Stark256::derive_from_path(ctx.bip32_path.as_ref())
.deterministic_sign(ctx.hash.m_hash.value.as_ref())
Expand Down
18 changes: 2 additions & 16 deletions starknet/src/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ use crate::{
erc20::{ERC20_TOKENS, TRANSFER},
types::FieldElement,
};
use alloc::format;
use include_gif::include_gif;
use ledger_device_sdk::{io::Comm, testing};
use ledger_device_sdk::io::Comm;

use crate::context::{Ctx, Transaction};

Expand All @@ -26,7 +25,6 @@ use ledger_device_sdk::nbgl::{
pub fn show_tx(ctx: &mut Ctx) -> Option<bool> {
match support_clear_sign(&ctx.tx) {
Some(t) => {
testing::debug_print("Clear sign supported !!! \n");
let tx = &ctx.tx;
let call = &tx.calls[0];

Expand All @@ -35,8 +33,6 @@ pub fn show_tx(ctx: &mut Ctx) -> Option<bool> {
let to = call.calldata[0].to_hex_string();
let amount = call.calldata[1].to_dec_string(Some(ERC20_TOKENS[t].decimals));

testing::debug_print("Compute fees \n");

let max_fees_str = match tx.version {
FieldElement::ONE => {
let mut max_fees_str = tx.max_fee.to_dec_string(None);
Expand All @@ -58,8 +54,6 @@ pub fn show_tx(ctx: &mut Ctx) -> Option<bool> {
}
};

testing::debug_print("Compute fees OK \n");

let my_fields = [
Field {
name: "From",
Expand All @@ -83,11 +77,6 @@ pub fn show_tx(ctx: &mut Ctx) -> Option<bool> {
},
];

testing::debug_print(&format!(
"Token: {}\nTo: {}\nAmount: {}\n",
token, to, amount
));

#[cfg(not(any(target_os = "stax", target_os = "flex")))]
{
let my_review = MultiFieldReview::new(
Expand Down Expand Up @@ -115,10 +104,7 @@ pub fn show_tx(ctx: &mut Ctx) -> Option<bool> {
Some(review.show(&my_fields))
}
}
None => {
testing::debug_print("Clear sign not supported !!! \n");
None
}
None => None,
}
}

Expand Down
69 changes: 39 additions & 30 deletions starknet/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ extern "C" fn sample_main() {
// or an APDU command
if let io::Event::Command(ins) = display::main_ui(&mut comm) {
match handle_apdu(&mut comm, &ins, &mut ctx) {
Ok(()) => comm.reply_ok(),
Ok(data) => {
comm.append(data.as_slice());
comm.reply_ok()
}
Err(sw) => comm.reply(sw),
}
}
Expand All @@ -54,7 +57,10 @@ extern "C" fn sample_main() {
// Wait for an APDU command
let ins: Ins = comm.next_command();
match handle_apdu(&mut comm, &ins, &mut ctx) {
Ok(()) => comm.reply_ok(),
Ok(data) => {
comm.append(data.as_slice());
comm.reply_ok()
}
Err(sw) => comm.reply(sw),
}
}
Expand Down Expand Up @@ -101,19 +107,22 @@ use ledger_device_sdk::io::Reply;

const SIG_LENGTH: u8 = 0x41;

fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Reply> {
fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<Vec<u8>, Reply> {
if comm.rx == 0 {
return Err(io::StatusWords::NothingReceived.into());
}

let apdu_header = comm.get_apdu_metadata();

let mut rdata: Vec<u8> = Vec::new();

match ins {
Ins::GetVersion => {
let version_major = env!("CARGO_PKG_VERSION_MAJOR").parse::<u8>().unwrap();
let version_minor = env!("CARGO_PKG_VERSION_MINOR").parse::<u8>().unwrap();
let version_patch = env!("CARGO_PKG_VERSION_PATCH").parse::<u8>().unwrap();
comm.append([version_major, version_minor, version_patch].as_slice());

rdata.extend_from_slice([version_major, version_minor, version_patch].as_slice());
}
Ins::GetPubkey { display } => {
ctx.reset();
Expand All @@ -138,7 +147,7 @@ fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Repl
true => display::pkey_ui(key.as_ref(), ctx),
};
if ret {
comm.append(key.as_ref());
rdata.extend_from_slice(key.as_ref());
} else {
return Err(io::StatusWords::UserCancelled.into());
}
Expand All @@ -164,10 +173,10 @@ fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Repl
match display::show_hash(ctx, false) {
true => {
crypto::sign_hash(ctx).unwrap();
comm.append([0x41].as_slice());
comm.append(ctx.hash.r.as_ref());
comm.append(ctx.hash.s.as_ref());
comm.append([ctx.hash.v].as_slice());
rdata.extend_from_slice([0x41].as_slice());
rdata.extend_from_slice(ctx.hash.r.as_ref());
rdata.extend_from_slice(ctx.hash.s.as_ref());
rdata.extend_from_slice([ctx.hash.v].as_slice());
}
false => {
return Err(io::StatusWords::UserCancelled.into());
Expand Down Expand Up @@ -213,13 +222,13 @@ fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Repl
true => {
display::show_pending();
ctx.hash.m_hash = crypto::tx_hash(&ctx.tx);
comm.append(ctx.hash.m_hash.value.as_ref());
rdata.extend_from_slice(ctx.hash.m_hash.value.as_ref());
crypto::sign_hash(ctx).unwrap();
rdata.extend_from_slice([SIG_LENGTH].as_slice());
rdata.extend_from_slice(ctx.hash.r.as_ref());
rdata.extend_from_slice(ctx.hash.s.as_ref());
rdata.extend_from_slice([ctx.hash.v].as_slice());
display::show_status(true, ctx);
comm.append([SIG_LENGTH].as_slice());
comm.append(ctx.hash.r.as_ref());
comm.append(ctx.hash.s.as_ref());
comm.append([ctx.hash.v].as_slice());
}
false => {
display::show_status(false, ctx);
Expand All @@ -231,13 +240,13 @@ fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Repl
ctx.hash.m_hash = crypto::tx_hash(&ctx.tx);
match display::show_hash(ctx, true) {
true => {
comm.append(ctx.hash.m_hash.value.as_ref());
rdata.extend_from_slice(ctx.hash.m_hash.value.as_ref());
crypto::sign_hash(ctx).unwrap();
rdata.extend_from_slice([SIG_LENGTH].as_slice());
rdata.extend_from_slice(ctx.hash.r.as_ref());
rdata.extend_from_slice(ctx.hash.s.as_ref());
rdata.extend_from_slice([ctx.hash.v].as_slice());
display::show_status(true, ctx);
comm.append([SIG_LENGTH].as_slice());
comm.append(ctx.hash.r.as_ref());
comm.append(ctx.hash.s.as_ref());
comm.append([ctx.hash.v].as_slice());
}
false => {
display::show_status(false, ctx);
Expand Down Expand Up @@ -284,13 +293,13 @@ fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Repl
true => {
display::show_pending();
ctx.hash.m_hash = crypto::tx_hash(&ctx.tx);
comm.append(ctx.hash.m_hash.value.as_ref());
rdata.extend_from_slice(ctx.hash.m_hash.value.as_ref());
crypto::sign_hash(ctx).unwrap();
rdata.extend_from_slice([0x41].as_slice());
rdata.extend_from_slice(ctx.hash.r.as_ref());
rdata.extend_from_slice(ctx.hash.s.as_ref());
rdata.extend_from_slice([ctx.hash.v].as_slice());
display::show_status(true, ctx);
comm.append([0x41].as_slice());
comm.append(ctx.hash.r.as_ref());
comm.append(ctx.hash.s.as_ref());
comm.append([ctx.hash.v].as_slice());
}
false => {
display::show_status(false, ctx);
Expand All @@ -302,13 +311,13 @@ fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Repl
ctx.hash.m_hash = crypto::tx_hash(&ctx.tx);
match display::show_hash(ctx, true) {
true => {
comm.append(ctx.hash.m_hash.value.as_ref());
rdata.extend_from_slice(ctx.hash.m_hash.value.as_ref());
crypto::sign_hash(ctx).unwrap();
rdata.extend_from_slice([0x41].as_slice());
rdata.extend_from_slice(ctx.hash.r.as_ref());
rdata.extend_from_slice(ctx.hash.s.as_ref());
rdata.extend_from_slice([ctx.hash.v].as_slice());
display::show_status(true, ctx);
comm.append([0x41].as_slice());
comm.append(ctx.hash.r.as_ref());
comm.append(ctx.hash.s.as_ref());
comm.append([ctx.hash.v].as_slice());
}
false => {
display::show_status(false, ctx);
Expand Down Expand Up @@ -375,5 +384,5 @@ fn handle_apdu(comm: &mut io::Comm, ins: &Ins, ctx: &mut Ctx) -> Result<(), Repl
}
}
}
Ok(())
Ok(rdata)
}
Binary file modified tests/snapshots/flex/test_app_mainmenu/00001.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests/snapshots/nanosp/test_app_mainmenu/00001.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests/snapshots/nanox/test_app_mainmenu/00001.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests/snapshots/stax/test_app_mainmenu/00001.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading