novasamatech / parity-signer

Air-gapped crypto wallet.
https://vault.novasama.io
GNU General Public License v3.0
559 stars 168 forks source link

Parsers improvements #1155

Open montekki opened 2 years ago

montekki commented 2 years ago

Code that handles user inputs (metadata, txs, qrs) should be improved.

It should not

  1. AND, XOR or OR things with random undocumented numeric values such as 0x1000000.
  2. Panic on bad user inputs.
  3. match against undocumented values that no one understands what their values are or how stable these values are from one chain to another.

It should

  1. At least be fuzzed.

https://github.com/paritytech/parity-signer/blob/c54813197fd9e627d0a3d46b6a65ab233bf4803b/rust/transaction_parsing/src/lib.rs#L58-L69

https://github.com/paritytech/parity-signer/blob/224d3dc034993106c8eafd333cabb4057725f38b/rust/qr_reader_phone/src/process_payload.rs#L45-L57

https://github.com/paritytech/parity-signer/blob/224d3dc034993106c8eafd333cabb4057725f38b/rust/qr_reader_phone/src/process_payload.rs#L101-L111

Slesarew commented 2 years ago

AND, XOR or OR things with random undocumented numeric values such as 0x1000000

This is well documented: https://paritytech.github.io/parity-signer/development/UOS.html#raptorq-multipart-payload

Panic on bad user inputs.

Where does it panic? Please point me or propose bad user input

match against undocumented values that no one understands what their values are or how stable these values are from one chain to another.

It is well-documented here. https://paritytech.github.io/parity-signer/development/UOS.html

Please advise

montekki commented 2 years ago

a bad user input is easy to construct by hand

diff --git a/rust/signer/src/lib.rs b/rust/signer/src/lib.rs
index 801482c2..b1b0c503 100644
--- a/rust/signer/src/lib.rs
+++ b/rust/signer/src/lib.rs
@@ -219,5 +219,10 @@ ffi_support::define_string_destructor!(signer_destroy_string);

 #[cfg(test)]
 mod tests {
-    //use super::*;
+    use crate::qrparser_try_decode_qr_sequence;
+
+    #[test]
+    fn try_decode_qr_test() {
+        qrparser_try_decode_qr_sequence("[\"aa\"]", true).unwrap();
+    }
 }

reliably gets you a panic

running 1 test
thread 'tests::try_decode_qr_test' panicked at 'range end index 4 out of range for slice of length 1', library/core/src/slice/index.rs:73:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::try_decode_qr_test ... FAILED
prybalko commented 2 years ago

I'd propose to use nom as a parser lib