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

trezor: try empty passphrase first, make trezor usage easier for users #7793

Merged
Show file tree
Hide file tree
Changes from all 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
20 changes: 20 additions & 0 deletions src/device/device_cold.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,26 @@ namespace hw {
* Live refresh process termination
*/
virtual void live_refresh_finish() =0;

/**
* Requests public address, uses empty passphrase if asked for.
*/
virtual bool get_public_address_with_no_passphrase(cryptonote::account_public_address &pubkey) =0;

/**
* Reset session ID, restart with a new session.
*/
virtual void reset_session() =0;

/**
* Returns true if device already asked for passphrase entry before (i.e., obviously supports passphrase entry)
*/
virtual bool seen_passphrase_entry_prompt() =0;

/**
* Uses empty passphrase for all passphrase queries.
*/
virtual void set_use_empty_passphrase(bool always_use_empty_passphrase) =0;
};
}

Expand Down
21 changes: 21 additions & 0 deletions src/device_trezor/device_trezor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,15 @@ namespace trezor {
}
}

bool device_trezor::get_public_address_with_no_passphrase(cryptonote::account_public_address &pubkey) {
m_reply_with_empty_passphrase = true;
const auto empty_passphrase_reverter = epee::misc_utils::create_scope_leave_handler([&]() {
m_reply_with_empty_passphrase = false;
});

return get_public_address(pubkey);
}

bool device_trezor::get_secret_keys(crypto::secret_key &viewkey , crypto::secret_key &spendkey) {
try {
MDEBUG("Loading view-only key from the Trezor. Please check the Trezor for a confirmation.");
Expand Down Expand Up @@ -206,6 +215,18 @@ namespace trezor {
get_address(index, payment_id, true);
}

void device_trezor::reset_session() {
m_device_session_id.clear();
}

bool device_trezor::seen_passphrase_entry_prompt() {
return m_seen_passphrase_entry_message;
}

void device_trezor::set_use_empty_passphrase(bool always_use_empty_passphrase) {
m_always_use_empty_passphrase = always_use_empty_passphrase;
}

/* ======================================================================= */
/* Helpers */
/* ======================================================================= */
Expand Down
20 changes: 20 additions & 0 deletions src/device_trezor/device_trezor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,26 @@ namespace trezor {
const ::tools::wallet2::unsigned_tx_set & unsigned_tx,
::tools::wallet2::signed_tx_set & signed_tx,
hw::tx_aux_data & aux_data) override;

/**
* Requests public address, uses empty passphrase if asked for.
*/
bool get_public_address_with_no_passphrase(cryptonote::account_public_address &pubkey) override;

/**
* Reset session ID, restart with a new session.
*/
virtual void reset_session() override;

/**
* Returns true if device already asked for passphrase entry before (i.e., obviously supports passphrase entry)
*/
bool seen_passphrase_entry_prompt() override;

/**
* Uses empty passphrase for all passphrase queries.
*/
void set_use_empty_passphrase(bool use_always_empty_passphrase) override;
};

#endif
Expand Down
29 changes: 19 additions & 10 deletions src/device_trezor/device_trezor_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ namespace trezor {

const uint32_t device_trezor_base::DEFAULT_BIP44_PATH[] = {0x8000002c, 0x80000080};

device_trezor_base::device_trezor_base(): m_callback(nullptr), m_last_msg_type(messages::MessageType_Success) {
device_trezor_base::device_trezor_base(): m_callback(nullptr), m_last_msg_type(messages::MessageType_Success),
m_reply_with_empty_passphrase(false),
m_always_use_empty_passphrase(false),
m_seen_passphrase_entry_message(false) {
#ifdef WITH_TREZOR_DEBUGGING
m_debug = false;
#endif
Expand Down Expand Up @@ -155,6 +158,9 @@ namespace trezor {
TREZOR_AUTO_LOCK_DEVICE();
m_device_session_id.clear();
m_features.reset();
m_seen_passphrase_entry_message = false;
m_reply_with_empty_passphrase = false;
m_always_use_empty_passphrase = false;

if (m_transport){
try {
Expand Down Expand Up @@ -476,6 +482,7 @@ namespace trezor {
return;
}

m_seen_passphrase_entry_message = true;
bool on_device = true;
if (msg->has__on_device() && !msg->_on_device()){
on_device = false; // do not enter on device, old devices.
Expand All @@ -491,19 +498,21 @@ namespace trezor {
}

boost::optional<epee::wipeable_string> passphrase;
TREZOR_CALLBACK_GET(passphrase, on_passphrase_request, on_device);
if (m_reply_with_empty_passphrase || m_always_use_empty_passphrase) {
MDEBUG("Answering passphrase prompt with an empty passphrase, always use empty: " << m_always_use_empty_passphrase);
on_device = false;
passphrase = epee::wipeable_string("");
} else if (m_passphrase){
MWARNING("Answering passphrase prompt with a stored passphrase (do not use; passphrase can be seen by a potential malware / attacker)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain "stored passphrase" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it was part of the old API, you could set m_passphrase to the device object, the passphrase will be then used instead of prompting user. I've added this warning as in general, we don't want users entering their passhprases on computers, but this may have useful use-cases when you know what are you doing. E.g., in tests, automation, etc. It was part of the API so I don't feel like removing it now.

on_device = false;
passphrase = epee::wipeable_string(m_passphrase.get());
} else {
TREZOR_CALLBACK_GET(passphrase, on_passphrase_request, on_device);
}

messages::common::PassphraseAck m;
m.set_on_device(on_device);
if (!on_device) {
if (!passphrase && m_passphrase) {
passphrase = m_passphrase;
}

if (m_passphrase) {
m_passphrase = boost::none;
}

if (passphrase) {
m.set_allocated_passphrase(new std::string(passphrase->data(), passphrase->size()));
}
Expand Down
3 changes: 3 additions & 0 deletions src/device_trezor/device_trezor_base.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ namespace trezor {
messages::MessageType m_last_msg_type;

cryptonote::network_type network_type;
bool m_reply_with_empty_passphrase;
bool m_always_use_empty_passphrase;
bool m_seen_passphrase_entry_message;

#ifdef WITH_TREZOR_DEBUGGING
std::shared_ptr<trezor_debug_callback> m_debug_callback;
Expand Down
21 changes: 20 additions & 1 deletion src/wallet/wallet2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4424,7 +4424,26 @@ bool wallet2::load_keys_buf(const std::string& keys_buf, const epee::wipeable_st
m_account.set_device(hwdev);

account_public_address device_account_public_address;
THROW_WALLET_EXCEPTION_IF(!hwdev.get_public_address(device_account_public_address), error::wallet_internal_error, "Cannot get a device address");
bool fetch_device_address = true;

::hw::device_cold* dev_cold = nullptr;
if (m_key_device_type == hw::device::device_type::TREZOR && (dev_cold = dynamic_cast<::hw::device_cold*>(&hwdev)) != nullptr) {
THROW_WALLET_EXCEPTION_IF(!dev_cold->get_public_address_with_no_passphrase(device_account_public_address), error::wallet_internal_error, "Cannot get a device address");
if (device_account_public_address == m_account.get_keys().m_account_address) {
LOG_PRINT_L0("Wallet opened with an empty passphrase");
fetch_device_address = false;
dev_cold->set_use_empty_passphrase(true);
} else {
fetch_device_address = true;
Copy link
Contributor Author

@ph4r05 ph4r05 Jul 14, 2021

Choose a reason for hiding this comment

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

I wanted to use dev_cold->seen_passphrase_entry_prompt() and to try again with passphrase only if device obviously asked for any. But to avoid potential problems we can just ask anyway. The worst case is that address is loaded twice on error.

LOG_PRINT_L0("Wallet opening with an empty passphrase failed. Retry again: " << fetch_device_address);
dev_cold->reset_session();
}
}

if (fetch_device_address) {
THROW_WALLET_EXCEPTION_IF(!hwdev.get_public_address(device_account_public_address), error::wallet_internal_error, "Cannot get a device address");
}

THROW_WALLET_EXCEPTION_IF(device_account_public_address != m_account.get_keys().m_account_address, error::wallet_internal_error, "Device wallet does not match wallet address. If the device uses the passphrase feature, please check whether the passphrase was entered correctly (it may have been misspelled - different passphrases generate different wallets, passphrase is case-sensitive). "
"Device address: " + cryptonote::get_account_address_as_str(m_nettype, false, device_account_public_address) +
", wallet address: " + m_account.get_public_address_str(m_nettype));
Expand Down