device/trezor: device/trezor: correct device initialization, status check

- checks if the device is in the correct usable state
- implements check for the v2.0.9 firmware which does not support payment IDs
- simple transacttion check, payment id fmt consistency
- minor fixes, refactoring, webusb session counting fix

6 files changed, 140 insertions, 23 deletions

diff --git a/cmake/CheckTrezor.cmake b/cmake/CheckTrezor.cmake
index ea21237fd..bcd3cfc2c 100644
--- a/cmake/CheckTrezor.cmake
+++ b/cmake/CheckTrezor.cmake
@@ -56,7 +56,7 @@ if(Protobuf_FOUND AND USE_DEVICE_TREZOR AND TREZOR_PYTHON)
         endif()
 
         if(USE_DEVICE_TREZOR_UDP_RELEASE)
-            add_definitions(-DWITH_DEVICE_TREZOR_UDP_RELEASE=1)
+            add_definitions(-DUSE_DEVICE_TREZOR_UDP_RELEASE=1)
         endif()
 
         if (Protobuf_INCLUDE_DIR)
diff --git a/src/device_trezor/device_trezor.cpp b/src/device_trezor/device_trezor.cpp
index 5096fcea8..8868fb995 100644
--- a/src/device_trezor/device_trezor.cpp
+++ b/src/device_trezor/device_trezor.cpp
@@ -121,7 +121,8 @@ namespace trezor {
         const boost::optional<cryptonote::network_type> & network_type){
       AUTO_LOCK_CMD();
       require_connected();
-      test_ping();
+      device_state_reset_unsafe();
+      require_initialized();
 
       auto req = std::make_shared<messages::monero::MoneroGetAddress>();
       this->set_msg_addr<messages::monero::MoneroGetAddress>(req.get(), path, network_type);
@@ -136,7 +137,8 @@ namespace trezor {
         const boost::optional<cryptonote::network_type> & network_type){
       AUTO_LOCK_CMD();
       require_connected();
-      test_ping();
+      device_state_reset_unsafe();
+      require_initialized();
 
       auto req = std::make_shared<messages::monero::MoneroGetWatchKey>();
       this->set_msg_addr<messages::monero::MoneroGetWatchKey>(req.get(), path, network_type);
@@ -152,7 +154,8 @@ namespace trezor {
     {
       AUTO_LOCK_CMD();
       require_connected();
-      test_ping();
+      device_state_reset_unsafe();
+      require_initialized();
 
       std::shared_ptr<messages::monero::MoneroKeyImageExportInitRequest> req;
 
@@ -238,12 +241,11 @@ namespace trezor {
         cpend.construction_data = cdata.tx_data;
 
         // Transaction check
-        cryptonote::blobdata tx_blob;
-        cryptonote::transaction tx_deserialized;
-        bool r = cryptonote::t_serializable_object_to_blob(cpend.tx, tx_blob);
-        CHECK_AND_ASSERT_THROW_MES(r, "Transaction serialization failed");
-        r = cryptonote::parse_and_validate_tx_from_blob(tx_blob, tx_deserialized);
-        CHECK_AND_ASSERT_THROW_MES(r, "Transaction deserialization failed");
+        try {
+          transaction_check(cdata, aux_data);
+        } catch(const std::exception &e){
+          throw exc::ProtocolException(std::string("Transaction verification failed: ") + e.what());
+        }
 
         std::string key_images;
         bool all_are_txin_to_key = std::all_of(cdata.tx.vin.begin(), cdata.tx.vin.end(), [&](const cryptonote::txin_v& s_e) -> bool
@@ -283,7 +285,8 @@ namespace trezor {
     {
       AUTO_LOCK_CMD();
       require_connected();
-      test_ping();
+      device_state_reset_unsafe();
+      require_initialized();
 
       CHECK_AND_ASSERT_THROW_MES(idx < unsigned_tx.txes.size(), "Invalid transaction index");
       signer = std::make_shared<protocol::tx::Signer>(wallet, &unsigned_tx, idx, &aux_data);
@@ -294,6 +297,7 @@ namespace trezor {
       // Step: Init
       auto init_msg = signer->step_init();
       this->set_msg_addr(init_msg.get());
+      transaction_pre_check(init_msg);
 
       auto response = this->client_exchange<messages::monero::MoneroTransactionInitAck>(init_msg);
       signer->step_init_ack(response);
@@ -351,6 +355,59 @@ namespace trezor {
       signer->step_final_ack(ack_final);
     }
 
+    void device_trezor::transaction_pre_check(std::shared_ptr<messages::monero::MoneroTransactionInitRequest> init_msg)
+    {
+      CHECK_AND_ASSERT_THROW_MES(init_msg, "TransactionInitRequest is empty");
+      CHECK_AND_ASSERT_THROW_MES(init_msg->has_tsx_data(), "TransactionInitRequest has no transaction data");
+      CHECK_AND_ASSERT_THROW_MES(m_features, "Device state not initialized");  // make sure the caller did not reset features
+      const bool nonce_required = init_msg->tsx_data().has_payment_id() && init_msg->tsx_data().payment_id().size() > 0;
+
+      if (nonce_required){
+        // Versions 2.0.9 and lower do not support payment ID
+        CHECK_AND_ASSERT_THROW_MES(m_features->has_major_version() && m_features->has_minor_version() && m_features->has_patch_version(), "Invalid Trezor firmware version information");
+        const uint32_t vma = m_features->major_version();
+        const uint32_t vmi = m_features->minor_version();
+        const uint32_t vpa = m_features->patch_version();
+        if (vma < 2 || (vma == 2 && vmi == 0 && vpa <= 9)) {
+          throw exc::TrezorException("Trezor firmware 2.0.9 and lower does not support transactions with short payment IDs or integrated addresses. Please update.");
+        }
+      }
+    }
+
+    void device_trezor::transaction_check(const protocol::tx::TData & tdata, const hw::tx_aux_data & aux_data)
+    {
+      // Simple serialization check
+      cryptonote::blobdata tx_blob;
+      cryptonote::transaction tx_deserialized;
+      bool r = cryptonote::t_serializable_object_to_blob(tdata.tx, tx_blob);
+      CHECK_AND_ASSERT_THROW_MES(r, "Transaction serialization failed");
+      r = cryptonote::parse_and_validate_tx_from_blob(tx_blob, tx_deserialized);
+      CHECK_AND_ASSERT_THROW_MES(r, "Transaction deserialization failed");
+
+      // Extras check
+      std::vector<cryptonote::tx_extra_field> tx_extra_fields;
+      cryptonote::tx_extra_nonce nonce;
+
+      r = cryptonote::parse_tx_extra(tdata.tx.extra, tx_extra_fields);
+      CHECK_AND_ASSERT_THROW_MES(r, "tx.extra parsing failed");
+
+      const bool nonce_required = tdata.tsx_data.has_payment_id() && tdata.tsx_data.payment_id().size() > 0;
+      const bool has_nonce = cryptonote::find_tx_extra_field_by_type(tx_extra_fields, nonce);
+      CHECK_AND_ASSERT_THROW_MES(has_nonce == nonce_required, "Transaction nonce presence inconsistent");
+
+      if (nonce_required){
+        const std::string & payment_id = tdata.tsx_data.payment_id();
+        if (payment_id.size() == 32){
+          crypto::hash payment_id_long{};
+          CHECK_AND_ASSERT_THROW_MES(cryptonote::get_payment_id_from_tx_extra_nonce(nonce.nonce, payment_id_long), "Long payment ID not present");
+
+        } else if (payment_id.size() == 8){
+          crypto::hash8 payment_id_short{};
+          CHECK_AND_ASSERT_THROW_MES(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce(nonce.nonce, payment_id_short), "Short payment ID not present");
+        }
+      }
+    }
+
 #else //WITH_DEVICE_TREZOR
 
     void register_all(std::map<std::string, std::unique_ptr<device>> &registry) {
diff --git a/src/device_trezor/device_trezor.hpp b/src/device_trezor/device_trezor.hpp
index a2134574c..1f08be887 100644
--- a/src/device_trezor/device_trezor.hpp
+++ b/src/device_trezor/device_trezor.hpp
@@ -57,9 +57,8 @@ namespace trezor {
    */
   class device_trezor : public hw::trezor::device_trezor_base, public hw::device_cold {
     protected:
-      // To speed up blockchain parsing the view key maybe handle here.
-      crypto::secret_key viewkey;
-      bool has_view_key;
+      void transaction_pre_check(std::shared_ptr<messages::monero::MoneroTransactionInitRequest> init_msg);
+      void transaction_check(const protocol::tx::TData & tdata, const hw::tx_aux_data & aux_data);
 
     public:
       device_trezor();
diff --git a/src/device_trezor/device_trezor_base.cpp b/src/device_trezor/device_trezor_base.cpp
index fddc6082c..5071932ee 100644
--- a/src/device_trezor/device_trezor_base.cpp
+++ b/src/device_trezor/device_trezor_base.cpp
@@ -65,7 +65,7 @@ namespace trezor {
     }
 
     bool device_trezor_base::set_name(const std::string & name) {
-      this->full_name = name;
+      this->m_full_name = name;
       this->name = "";
 
       auto delim = name.find(':');
@@ -77,10 +77,10 @@ namespace trezor {
     }
 
     const std::string device_trezor_base::get_name() const {
-      if (this->full_name.empty()) {
+      if (this->m_full_name.empty()) {
         return std::string("<disconnected:").append(this->name).append(">");
       }
-      return this->full_name;
+      return this->m_full_name;
     }
 
     bool device_trezor_base::init() {
@@ -139,6 +139,9 @@ namespace trezor {
     }
 
     bool device_trezor_base::disconnect() {
+      m_device_state.clear();
+      m_features.reset();
+
       if (m_transport){
         try {
           m_transport->close();
@@ -193,6 +196,25 @@ namespace trezor {
       }
     }
 
+    void device_trezor_base::require_initialized(){
+      if (!m_features){
+        throw exc::TrezorException("Device state not initialized");
+      }
+
+      if (m_features->has_bootloader_mode() && m_features->bootloader_mode()){
+        throw exc::TrezorException("Device is in the bootloader mode");
+      }
+
+      if (m_features->has_firmware_present() && !m_features->firmware_present()){
+        throw exc::TrezorException("Device has no firmware loaded");
+      }
+
+      // Hard requirement on initialized field, has to be there.
+      if (!m_features->has_initialized() || !m_features->initialized()){
+        throw exc::TrezorException("Device is not initialized");
+      }
+    }
+
     void device_trezor_base::call_ping_unsafe(){
       auto pingMsg = std::make_shared<messages::management::Ping>();
       pingMsg->set_message("PING");
@@ -217,7 +239,7 @@ namespace trezor {
     void device_trezor_base::write_raw(const google::protobuf::Message * msg){
       require_connected();
       CHECK_AND_ASSERT_THROW_MES(msg, "Empty message");
-      this->getTransport()->write(*msg);
+      this->get_transport()->write(*msg);
     }
 
     GenericMessage device_trezor_base::read_raw(){
@@ -225,7 +247,7 @@ namespace trezor {
       std::shared_ptr<google::protobuf::Message> msg_resp;
       hw::trezor::messages::MessageType msg_resp_type;
 
-      this->getTransport()->read(msg_resp, &msg_resp_type);
+      this->get_transport()->read(msg_resp, &msg_resp_type);
       return GenericMessage(msg_resp_type, msg_resp);
     }
 
@@ -314,6 +336,25 @@ namespace trezor {
       return false;
     }
 
+    void device_trezor_base::device_state_reset_unsafe()
+    {
+      require_connected();
+      auto initMsg = std::make_shared<messages::management::Initialize>();
+
+      if(!m_device_state.empty()) {
+        initMsg->set_allocated_state(&m_device_state);
+      }
+
+      m_features = this->client_exchange<messages::management::Features>(initMsg);
+      initMsg->release_state();
+    }
+
+    void device_trezor_base::device_state_reset()
+    {
+      AUTO_LOCK_CMD();
+      device_state_reset_unsafe();
+    }
+
     void device_trezor_base::on_button_request(GenericMessage & resp, const messages::common::ButtonRequest * msg)
     {
       CHECK_AND_ASSERT_THROW_MES(msg, "Empty message");
@@ -361,7 +402,13 @@ namespace trezor {
         // TODO: remove passphrase from memory
         m.set_passphrase(passphrase.data(), passphrase.size());
       }
+
+      if (!m_device_state.empty()){
+        m.set_allocated_state(&m_device_state);
+      }
+
       resp = call_raw(&m);
+      m.release_state();
     }
 
     void device_trezor_base::on_passphrase_state_request(GenericMessage & resp, const messages::common::PassphraseStateRequest * msg)
@@ -369,6 +416,7 @@ namespace trezor {
       MDEBUG("on_passhprase_state_request");
       CHECK_AND_ASSERT_THROW_MES(msg, "Empty message");
 
+      m_device_state = msg->state();
       messages::common::PassphraseStateAck m;
       resp = call_raw(&m);
     }
diff --git a/src/device_trezor/device_trezor_base.hpp b/src/device_trezor/device_trezor_base.hpp
index 4d205ad7a..3c35e8aca 100644
--- a/src/device_trezor/device_trezor_base.hpp
+++ b/src/device_trezor/device_trezor_base.hpp
@@ -70,8 +70,10 @@ namespace trezor {
       std::shared_ptr<Transport> m_transport;
       i_device_callback * m_callback;
 
-      std::string full_name;
+      std::string m_full_name;
       std::vector<unsigned int> m_wallet_deriv_path;
+      std::string m_device_state;  // returned after passphrase entry, session
+      std::shared_ptr<messages::management::Features> m_features;  // features from the last device reset
 
       cryptonote::network_type network_type;
 
@@ -80,8 +82,10 @@ namespace trezor {
       //
 
       void require_connected();
+      void require_initialized();
       void call_ping_unsafe();
       void test_ping();
+      void device_state_reset_unsafe();
       void ensure_derivation_path() noexcept;
 
       // Communication methods
@@ -130,7 +134,7 @@ namespace trezor {
         // Scoped session closer
         BOOST_SCOPE_EXIT_ALL(&, this) {
           if (open_session){
-            this->getTransport()->close();
+            this->get_transport()->close();
           }
         };
 
@@ -209,7 +213,7 @@ namespace trezor {
     // Default derivation path for Monero
     static const uint32_t DEFAULT_BIP44_PATH[2];
 
-    std::shared_ptr<Transport> getTransport(){
+    std::shared_ptr<Transport> get_transport(){
       return m_transport;
     }
 
@@ -221,6 +225,10 @@ namespace trezor {
       return m_callback;
     }
 
+    std::shared_ptr<messages::management::Features> & get_features() {
+      return m_features;
+    }
+
     void set_derivation_path(const std::string &deriv_path) override;
 
     /* ======================================================================= */
@@ -250,6 +258,11 @@ namespace trezor {
      */
     bool ping();
 
+    /**
+     * Performs Initialize call to the Trezor, resets to known state.
+     */
+    void device_state_reset();
+
     // Protocol callbacks
     void on_button_request(GenericMessage & resp, const messages::common::ButtonRequest * msg);
     void on_pin_request(GenericMessage & resp, const messages::common::PinMatrixRequest * msg);
diff --git a/src/device_trezor/trezor/transport.cpp b/src/device_trezor/trezor/transport.cpp
index 814537eb6..cd66e59e8 100644
--- a/src/device_trezor/trezor/transport.cpp
+++ b/src/device_trezor/trezor/transport.cpp
@@ -840,7 +840,7 @@ namespace trezor{
       throw exc::DeviceAcquireException("Unable to claim libusb device");
     }
 
-    m_conn_count += 1;
+    m_conn_count = 1;
     m_proto->session_begin(*this);
     
 #undef TREZOR_DESTROY_SESSION