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.

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: sign raw bytes #1332

Merged
merged 6 commits into from
Oct 6, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions rust/Cargo.lock

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

2 changes: 1 addition & 1 deletion rust/db_handling/src/db_transactions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,7 @@ pub struct TrDbColdSign {
///
/// Messages contain SCALE-encoded text messages.
#[cfg(feature = "signer")]
#[derive(Debug, Decode, Encode)]
#[derive(Debug, Decode, Encode, Clone)]
pub enum SignContent {
/// `53xx00` or `53xx02` transaction
Transaction {
Expand Down
43 changes: 24 additions & 19 deletions rust/navigator/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,20 @@ fn signature_is_good(transaction_hex: &str, signature_hex: &str) -> bool {
sp_core::ed25519::Pair::verify(&signature, &message, &public)
}
"5301" => {
assert!(
signature_hex.starts_with("01"),
"Signature in sr25519 should start with `01`."
);
let into_signature: [u8; 64] = hex::decode(&signature_hex[2..])
.unwrap()
.try_into()
.unwrap();
let into_signature: [u8; 64] = match &transaction_hex[4..6] {
"03" => hex::decode(&signature_hex).unwrap().try_into().unwrap(), // raw message
_ => {
assert!(
signature_hex.starts_with("01"),
"Signature in sr25519 should start with `01`."
);
hex::decode(&signature_hex[2..])
.unwrap()
.try_into()
.unwrap()
}
};

let signature = sp_core::sr25519::Signature::from_raw(into_signature);
let into_public: [u8; 32] = hex::decode(&transaction_hex[6..70])
.unwrap()
Expand Down Expand Up @@ -5601,9 +5607,10 @@ fn flow_test_1() {

// let's scan a text message
do_action(Action::NavbarScan, "", "").unwrap().unwrap();
let message_hex = "5301033efeca331d646d8a2986374bb3bb8d6e9e3cfcdd7c45c2b69104fab5d61d3f34f5064c6f72656d20697073756d20646f6c6f722073697420616d65742c20636f6e73656374657475722061646970697363696e6720656c69742c2073656420646f20656975736d6f642074656d706f7220696e6369646964756e74207574206c61626f726520657420646f6c6f7265206d61676e6120616c697175612e20557420656e696d206164206d696e696d2076656e69616d2c2071756973206e6f737472756420657865726369746174696f6e20756c6c616d636f206c61626f726973206e69736920757420616c697175697020657820656120636f6d6d6f646f20636f6e7365717561742e2044756973206175746520697275726520646f6c6f7220696e20726570726568656e646572697420696e20766f6c7570746174652076656c697420657373652063696c6c756d20646f6c6f726520657520667567696174206e756c6c612070617269617475722e204578636570746575722073696e74206f6363616563617420637570696461746174206e6f6e2070726f6964656e742c2073756e7420696e2063756c706120717569206f666669636961206465736572756e74206d6f6c6c697420616e696d20696420657374206c61626f72756d2ee143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e";
let card_text = "4c6f72656d20697073756d20646f6c6f722073697420616d65742c20636f6e73656374657475722061646970697363696e6720656c69742c2073656420646f20656975736d6f642074656d706f7220696e6369646964756e74207574206c61626f726520657420646f6c6f7265206d61676e6120616c697175612e20557420656e696d206164206d696e696d2076656e69616d2c2071756973206e6f737472756420657865726369746174696f6e20756c6c616d636f206c61626f726973206e69736920757420616c697175697020657820656120636f6d6d6f646f20636f6e7365717561742e2044756973206175746520697275726520646f6c6f7220696e20726570726568656e646572697420696e20766f6c7570746174652076656c697420657373652063696c6c756d20646f6c6f726520657520667567696174206e756c6c612070617269617475722e204578636570746575722073696e74206f6363616563617420637570696461746174206e6f6e2070726f6964656e742c2073756e7420696e2063756c706120717569206f666669636961206465736572756e74206d6f6c6c697420616e696d20696420657374206c61626f72756d2e".to_string();
let action = do_action(Action::TransactionFetched, message_hex, "")
let card_text = hex::encode(b"uuid-abcd");
let sign_msg = hex::encode(b"<Bytes>uuid-abcd</Bytes>");
let message_hex = format!("5301033efeca331d646d8a2986374bb3bb8d6e9e3cfcdd7c45c2b69104fab5d61d3f34{}e143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e", sign_msg);
let action = do_action(Action::TransactionFetched, &message_hex, "")
.unwrap()
.unwrap();
let expected_action = ActionResult {
Expand Down Expand Up @@ -5679,7 +5686,7 @@ fn flow_test_1() {
*/

assert!(
signature_is_good(message_hex, &signature_hex),
signature_is_good(&message_hex, &signature_hex),
"Produced bad signature: \n{}",
signature_hex
);
Expand All @@ -5702,7 +5709,7 @@ fn flow_test_1() {
timestamp: String::new(),
events: vec![Event::MessageSigned {
sign_message_display: SignMessageDisplay {
message: String::from_utf8(hex::decode(&card_text).unwrap())
message: String::from_utf8(hex::decode(&sign_msg).unwrap())
.unwrap(),
network_name: "westend".to_string(),
signed_by: VerifierValue::Standard {
Expand Down Expand Up @@ -5748,7 +5755,7 @@ fn flow_test_1() {
events: vec![MEventMaybeDecoded {
event: Event::MessageSigned {
sign_message_display: SignMessageDisplay {
message: String::from_utf8(hex::decode(&card_text).unwrap()).unwrap(),
message: String::from_utf8(hex::decode(&sign_msg).unwrap()).unwrap(),
network_name: "westend".to_string(),
signed_by: VerifierValue::Standard {
m: MultiSigner::Sr25519(
Expand Down Expand Up @@ -6307,9 +6314,7 @@ fn flow_test_1() {
message: Some(vec![TransactionCard {
index: 0,
indent: 0,
card: Card::TextCard {
f: card_text.clone(),
},
card: Card::TextCard { f: card_text },
}]),
..Default::default()
},
Expand Down Expand Up @@ -6442,7 +6447,7 @@ fn flow_test_1() {
};
let network_genesis_hash =
H256::from_str("e143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e").unwrap();
let message = String::from_utf8(hex::decode(&card_text).unwrap()).unwrap();
let message = String::from_utf8(hex::decode(&sign_msg).unwrap()).unwrap();
let expected_action = ActionResult {
screen_label: String::new(),
back: false,
Expand Down Expand Up @@ -6685,7 +6690,7 @@ fn flow_test_1() {
timestamp: String::new(),
events: vec![Event::MessageSigned {
sign_message_display: SignMessageDisplay {
message: String::from_utf8_lossy(&hex::decode(&card_text).unwrap())
message: String::from_utf8_lossy(&hex::decode(&sign_msg).unwrap())
.to_string(),
network_name: "westend".to_string(),
signed_by: verifier_value,
Expand Down
1 change: 1 addition & 0 deletions rust/transaction_parsing/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ sled = "0.34.6"
sp-core = {git = "https://github.com/paritytech/substrate", default-features = false, features = ["full_crypto"]}
sp-runtime = {git = "https://github.com/paritytech/substrate", default-features = false}
thiserror = "1.0.37"
nom = "7.1.1"

[dev-dependencies]
pretty_assertions = "1"
Expand Down
6 changes: 6 additions & 0 deletions rust/transaction_parsing/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,12 @@ pub enum Error {
/// network supported encryption
encryption: Encryption,
},

#[error(transparent)]
NotUtf8(#[from] std::str::Utf8Error),

#[error("Parser error: {0}")]
ParserError(String),
}

fn display_parsing_errors(network_name: &str, errors: &[(u32, parser::Error)]) -> String {
Expand Down
52 changes: 45 additions & 7 deletions rust/transaction_parsing/src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@ use definitions::{
keyring::{AddressKey, NetworkSpecsKey},
navigation::TransactionCardSet,
};
use parity_scale_codec::DecodeAll;
use nom::bytes::complete::{tag, take_until};
use parser::cards::ParserCard;
use std::path::Path;
use std::str;

use nom::sequence::preceded;
use nom::IResult;

use crate::cards::{make_author_info, Card, Warning};
use crate::error::{Error, Result};
Expand All @@ -23,11 +27,8 @@ where
multisigner_msg_genesis_encryption(data_hex)?;
let network_specs_key = NetworkSpecsKey::from_parts(&genesis_hash, &encryption);

// this is a standard decoding of String, with utf8 conversion;
// processing input vec![20, 104, 101, 3, 108, 111] will not throw error at element `3`,
// it will result in output `helo` instead, length, however, is still correct, 5.
// note that some invisible symbols may thus sneak into the message;
let message = String::decode_all(&mut &message_vec[..])?;
let message = str::from_utf8(&message_vec)?.to_owned();
let display_msg = strip_bytes_tag(&message)?;

// initialize index and indent
let mut index: u32 = 0;
Expand All @@ -39,7 +40,7 @@ where
match try_get_address_details(&db_path, &address_key)? {
Some(address_details) => {
if address_details.network_id.contains(&network_specs_key) {
let message_card = Card::ParserCard(&ParserCard::Text(message.to_string()))
let message_card = Card::ParserCard(&ParserCard::Text(display_msg))
.card(&mut index, indent);
let sign = TrDbColdSign::generate(
SignContent::Message(message),
Expand Down Expand Up @@ -136,3 +137,40 @@ where
}
}
}

fn bytes_prefix(i: &str) -> IResult<&str, &str> {
tag("<Bytes>")(i)
}

fn take_until_suffix(i: &str) -> IResult<&str, &str> {
take_until("</Bytes>")(i)
}

fn strip_bytes_tag(message: &str) -> Result<String> {
let mut parser = preceded(bytes_prefix, take_until_suffix);
let (_, payload) = parser(message).map_err(|e| Error::ParserError(format!("{:?}", e)))?;
Ok(payload.to_owned())
}

#[cfg(test)]
mod tests {

use super::*;

#[test]
fn parse_bytes_msg() {
let result = strip_bytes_tag("<Bytes>uuid-1234</Bytes>");
assert!(result.is_ok());
assert_eq!(result.unwrap(), "uuid-1234");
}

#[test]
fn parse_bytes_err() {
let result = strip_bytes_tag("<Bytes>uuid-1234");
assert!(result.is_err());
assert_eq!(
result.unwrap_err().to_string(),
"Parser error: Error(Error { input: \"uuid-1234\", code: TakeUntil })"
);
}
}
15 changes: 8 additions & 7 deletions rust/transaction_parsing/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2381,9 +2381,9 @@ fn parse_transaction_8_error_on_parsing() {
fn parse_msg_1() {
let dbname = "for_tests/parse_msg_1";
populate_cold(dbname, Verifier { v: None }).unwrap();
let line = "530103d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27df5064c6f72656d20697073756d20646f6c6f722073697420616d65742c20636f6e73656374657475722061646970697363696e6720656c69742c2073656420646f20656975736d6f642074656d706f7220696e6369646964756e74207574206c61626f726520657420646f6c6f7265206d61676e6120616c697175612e20557420656e696d206164206d696e696d2076656e69616d2c2071756973206e6f737472756420657865726369746174696f6e20756c6c616d636f206c61626f726973206e69736920757420616c697175697020657820656120636f6d6d6f646f20636f6e7365717561742e2044756973206175746520697275726520646f6c6f7220696e20726570726568656e646572697420696e20766f6c7570746174652076656c697420657373652063696c6c756d20646f6c6f726520657520667567696174206e756c6c612070617269617475722e204578636570746575722073696e74206f6363616563617420637570696461746174206e6f6e2070726f6964656e742c2073756e7420696e2063756c706120717569206f666669636961206465736572756e74206d6f6c6c697420616e696d20696420657374206c61626f72756d2ee143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e";

let text = "4c6f72656d20697073756d20646f6c6f722073697420616d65742c20636f6e73656374657475722061646970697363696e6720656c69742c2073656420646f20656975736d6f642074656d706f7220696e6369646964756e74207574206c61626f726520657420646f6c6f7265206d61676e6120616c697175612e20557420656e696d206164206d696e696d2076656e69616d2c2071756973206e6f737472756420657865726369746174696f6e20756c6c616d636f206c61626f726973206e69736920757420616c697175697020657820656120636f6d6d6f646f20636f6e7365717561742e2044756973206175746520697275726520646f6c6f7220696e20726570726568656e646572697420696e20766f6c7570746174652076656c697420657373652063696c6c756d20646f6c6f726520657520667567696174206e756c6c612070617269617475722e204578636570746575722073696e74206f6363616563617420637570696461746174206e6f6e2070726f6964656e742c2073756e7420696e2063756c706120717569206f666669636961206465736572756e74206d6f6c6c697420616e696d20696420657374206c61626f72756d2e".to_string();
let sign_msg = hex::encode(b"<Bytes>uuid-abcd</Bytes>");
let text = hex::encode(b"uuid-abcd");
let line = format!("530103d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d{}e143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e", sign_msg);

let set_expected = TransactionCardSet {
message: Some(vec![TransactionCard {
Expand All @@ -2405,7 +2405,7 @@ fn parse_msg_1() {
};

let network_info_known = westend_spec();
let action = produce_output(line, dbname);
let action = produce_output(&line, dbname);

if let TransactionAction::Sign {
content: set,
Expand All @@ -2430,18 +2430,19 @@ fn parse_msg_2() {
let dbname = "for_tests/parse_msg_2";
populate_cold(dbname, Verifier { v: None }).unwrap();
// sneaking one extra byte in the text body
let line = "530103d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27df5064c6f72656d20697073756d20646f6c6f722073697420616d65742c20636f6e73656374657475722061646970697363696e6720656c69742c2073656420646f20656975736d6f642074656d706f7220696e6369646964756e74207574206c61626f726520657420646f6c6f7265206d61676e6120616c697175612e20557420656e696d206164206d696e696d2076656e69616d2c2071756973206e6f737472756420657865726369746174696f6e20756c6c616d636f206c61626f726973206e69736920757420616c697175697020657820656120636f6d6d6f646f20636f6e7365717561742e2044756973206175746520697275726520646f6c6f7220696e20726570726568656e646572697420696e20766f6c7570746174652076656c697420657373652063696c6c756d20646f6c6f726520657520667567696174206e756c6c612070617269617475722e204578636570746575722073696e74206f6363616563617420637570696461746174206e6f6e2070726f6964656e742c2073756e7420696e2063756c706120717569206f666669636961206465736572756e74206d6f6c6c697420616e696d20696420657374206c6c61626f72756d2ee143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e";
let sign_msg = hex::encode(b"<Bytes>uuid-abcd");
let line = format!("530103d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d{}e143f23803ac50e8f6f8e62695d1ce9e4e1d68aa36c1cd2cfd15340213f3423e", sign_msg);
let set_expected = TransactionCardSet {
error: Some(vec![TransactionCard {
index: 0,
indent: 0,
card: Card::ErrorCard {
f: "Bad input data. Received message could not be read. Input buffer has still data left after decoding!".to_string(),
f: "Bad input data. Parser error: Error(Error { input: \"uuid-abcd\", code: TakeUntil })".to_string(),
},
}]),
..Default::default()
};
let action = produce_output(line, dbname);
let action = produce_output(&line, dbname);
if let TransactionAction::Read { r: set } = action {
assert_eq!(set, set_expected);
} else {
Expand Down
42 changes: 27 additions & 15 deletions rust/transaction_signing/src/sign_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub(crate) fn create_signature(
user_comment: &str,
database_name: &str,
checksum: u32,
) -> Result<MultiSignature> {
) -> Result<String> {
let sign = TrDbColdSign::from_storage(database_name, checksum)?;
let pwd = {
if sign.has_pwd() {
Expand All @@ -27,11 +27,12 @@ pub(crate) fn create_signature(
None
}
};
let content_vec = match sign.content() {
let content = sign.content().to_owned();
let content_vec = match &content {
SignContent::Transaction { method, extensions } => {
[method.to_vec(), extensions.to_vec()].concat()
}
SignContent::Message(a) => a.encode(),
SignContent::Message(a) => a.as_bytes().to_vec(),
};

// For larger transactions, their hash should be signed instead; this is not implemented
Expand All @@ -44,7 +45,8 @@ pub(crate) fn create_signature(
}
};
let mut full_address = seed_phrase.to_owned() + &sign.path();
match sign_as_address_key(&content_vec, &sign.multisigner(), &full_address, pwd) {
let signature = match sign_as_address_key(&content_vec, &sign.multisigner(), &full_address, pwd)
{
Ok(s) => {
full_address.zeroize();
sign.apply(false, user_comment, database_name)?;
Expand All @@ -59,7 +61,20 @@ pub(crate) fn create_signature(
Err(e)
}
}
}
}?;

let encoded = match &content {
SignContent::Transaction {
method: _,
extensions: _,
} => hex::encode(signature.encode()),
SignContent::Message(_) => match signature {
MultiSignature::Sr25519(a) => hex::encode(a),
MultiSignature::Ed25519(a) => hex::encode(a),
MultiSignature::Ecdsa(a) => hex::encode(a),
},
Comment on lines +71 to +75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this is right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it conceptually wrong, or is there a better way to write it? I struggle to find another solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think that it is not a good pattern to turn a return type from MultiSignature to String
  • Are you sure that this should be just the bare signature that is returned, not MultiSignature? How the users are going to tell the difference between different signature schemes? In case of a transaction MultiSignature is returned

};
Ok(encoded)
}

pub fn create_signature_png(
Expand All @@ -69,16 +84,13 @@ pub fn create_signature_png(
database_name: &str,
checksum: u32,
) -> Result<Vec<u8>> {
let hex_result = hex::encode(
create_signature(
seed_phrase,
pwd_entry,
user_comment,
database_name,
checksum,
)?
.encode(),
);
let hex_result = create_signature(
seed_phrase,
pwd_entry,
user_comment,
database_name,
checksum,
)?;
let qr_data = png_qr_from_string(&hex_result, DataType::Regular)?;
Ok(qr_data)
}
Loading