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

Jump to bottom

refactor: deduplicate the message sign/verify code #17577

Merged
merged 3 commits into from
Feb 25, 2020
Merged

refactor: deduplicate the message sign/verify code #17577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ce3447
Deduplicate the message verifying code
vasild Nov 19, 2019
f8f0d98
Deduplicate the message signing code
vasild Nov 22, 2019
e193a84
Refactor message hashing into a utility function
jkczyz Dec 7, 2019
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
2 changes: 2 additions & 0 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ BITCOIN_CORE_H = \
util/system.h \
util/macros.h \
util/memory.h \
util/message.h \
util/moneystr.h \
util/rbf.h \
util/settings.h \
Expand Down Expand Up @@ -517,6 +518,7 @@ libbitcoin_util_a_SOURCES = \
util/error.cpp \
util/fees.cpp \
util/system.cpp \
util/message.cpp \
util/moneystr.cpp \
util/rbf.cpp \
util/settings.cpp \
Expand Down
89 changes: 46 additions & 43 deletions src/qt/signverifymessagedialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#include <qt/walletmodel.h>

#include <key_io.h>
#include <util/validation.h> // For strMessageMagic
#include <util/message.h> // For MessageSign(), MessageVerify()
#include <wallet/wallet.h>

#include <vector>
Expand Down Expand Up @@ -141,13 +141,10 @@ void SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
return;
}

CHashWriter ss(SER_GETHASH, 0);
ss << strMessageMagic;
ss << ui->messageIn_SM->document()->toPlainText().toStdString();
const std::string& message = ui->messageIn_SM->document()->toPlainText().toStdString();
std::string signature;

std::vector<unsigned char> vchSig;
if (!key.SignCompact(ss.GetHash(), vchSig))
{
if (!MessageSign(key, message, signature)) {
ui->statusLabel_SM->setStyleSheet("QLabel { color: red; }");
ui->statusLabel_SM->setText(QString("<nobr>") + tr("Message signing failed.") + QString("</nobr>"));
return;
Expand All @@ -156,7 +153,7 @@ void SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
ui->statusLabel_SM->setStyleSheet("QLabel { color: green; }");
ui->statusLabel_SM->setText(QString("<nobr>") + tr("Message signed.") + QString("</nobr>"));

ui->signatureOut_SM->setText(QString::fromStdString(EncodeBase64(vchSig.data(), vchSig.size())));
ui->signatureOut_SM->setText(QString::fromStdString(signature));
}

void SignVerifyMessageDialog::on_copySignatureButton_SM_clicked()
Expand Down Expand Up @@ -189,51 +186,57 @@ void SignVerifyMessageDialog::on_addressBookButton_VM_clicked()

void SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()
{
CTxDestination destination = DecodeDestination(ui->addressIn_VM->text().toStdString());
if (!IsValidDestination(destination)) {
const std::string& address = ui->addressIn_VM->text().toStdString();
const std::string& signature = ui->signatureIn_VM->text().toStdString();
const std::string& message = ui->messageIn_VM->document()->toPlainText().toStdString();

const auto result = MessageVerify(address, signature, message);

if (result == MessageVerificationResult::OK) {
ui->statusLabel_VM->setStyleSheet("QLabel { color: green; }");
} else {
ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");
ui->statusLabel_VM->setText(tr("The entered address is invalid.") + QString(" ") + tr("Please check the address and try again."));
return;
}
if (!boost::get<PKHash>(&destination)) {

switch (result) {
case MessageVerificationResult::OK:
ui->statusLabel_VM->setText(
QString("<nobr>") + tr("Message verified.") + QString("</nobr>")
);
return;
case MessageVerificationResult::ERR_INVALID_ADDRESS:
ui->statusLabel_VM->setText(
tr("The entered address is invalid.") + QString(" ") +
tr("Please check the address and try again.")
);
return;
case MessageVerificationResult::ERR_ADDRESS_NO_KEY:
ui->addressIn_VM->setValid(false);
ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");
ui->statusLabel_VM->setText(tr("The entered address does not refer to a key.") + QString(" ") + tr("Please check the address and try again."));
ui->statusLabel_VM->setText(
tr("The entered address does not refer to a key.") + QString(" ") +
tr("Please check the address and try again.")
);
return;
}

bool fInvalid = false;
std::vector<unsigned char> vchSig = DecodeBase64(ui->signatureIn_VM->text().toStdString().c_str(), &fInvalid);

if (fInvalid)
{
case MessageVerificationResult::ERR_MALFORMED_SIGNATURE:
ui->signatureIn_VM->setValid(false);
ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");
ui->statusLabel_VM->setText(tr("The signature could not be decoded.") + QString(" ") + tr("Please check the signature and try again."));
ui->statusLabel_VM->setText(
tr("The signature could not be decoded.") + QString(" ") +
tr("Please check the signature and try again.")
);
return;
}

CHashWriter ss(SER_GETHASH, 0);
ss << strMessageMagic;
ss << ui->messageIn_VM->document()->toPlainText().toStdString();

CPubKey pubkey;
if (!pubkey.RecoverCompact(ss.GetHash(), vchSig))
{
case MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED:
ui->signatureIn_VM->setValid(false);
ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");
ui->statusLabel_VM->setText(tr("The signature did not match the message digest.") + QString(" ") + tr("Please check the signature and try again."));
ui->statusLabel_VM->setText(
tr("The signature did not match the message digest.") + QString(" ") +
tr("Please check the signature and try again.")
);
return;
}

if (!(CTxDestination(PKHash(pubkey)) == destination)) {
ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");
ui->statusLabel_VM->setText(QString("<nobr>") + tr("Message verification failed.") + QString("</nobr>"));
case MessageVerificationResult::ERR_NOT_SIGNED:
ui->statusLabel_VM->setText(
QString("<nobr>") + tr("Message verification failed.") + QString("</nobr>")
);
return;
}

ui->statusLabel_VM->setStyleSheet("QLabel { color: green; }");
ui->statusLabel_VM->setText(QString("<nobr>") + tr("Message verified.") + QString("</nobr>"));
}

void SignVerifyMessageDialog::on_clearButton_VM_clicked()
Expand Down
42 changes: 15 additions & 27 deletions src/rpc/misc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
#include <rpc/util.h>
#include <script/descriptor.h>
#include <util/check.h>
#include <util/message.h> // For MessageSign(), MessageVerify()
#include <util/strencodings.h>
#include <util/system.h>
#include <util/validation.h>

#include <stdint.h>
#include <tuple>
Expand Down Expand Up @@ -276,31 +276,21 @@ static UniValue verifymessage(const JSONRPCRequest& request)
std::string strSign = request.params[1].get_str();
std::string strMessage = request.params[2].get_str();

CTxDestination destination = DecodeDestination(strAddress);
if (!IsValidDestination(destination)) {
switch (MessageVerify(strAddress, strSign, strMessage)) {
case MessageVerificationResult::ERR_INVALID_ADDRESS:
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid address");
}

const PKHash *pkhash = boost::get<PKHash>(&destination);
if (!pkhash) {
case MessageVerificationResult::ERR_ADDRESS_NO_KEY:
throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
}

bool fInvalid = false;
std::vector<unsigned char> vchSig = DecodeBase64(strSign.c_str(), &fInvalid);

if (fInvalid)
case MessageVerificationResult::ERR_MALFORMED_SIGNATURE:
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Malformed base64 encoding");

CHashWriter ss(SER_GETHASH, 0);
ss << strMessageMagic;
ss << strMessage;

CPubKey pubkey;
if (!pubkey.RecoverCompact(ss.GetHash(), vchSig))
case MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED:
case MessageVerificationResult::ERR_NOT_SIGNED:
return false;
case MessageVerificationResult::OK:
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

You need another return false statement after this switch.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch. I'm not getting that -Wreturn-type warning on macOS. Only one of the Travis machines noticed it: https://travis-ci.org/bitcoin/bitcoin/jobs/649943882?utm_medium=notification&utm_source=github_status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added return false;, thanks!


return (pubkey.GetID() == *pkhash);
return false;
}

static UniValue signmessagewithprivkey(const JSONRPCRequest& request)
Expand Down Expand Up @@ -332,15 +322,13 @@ static UniValue signmessagewithprivkey(const JSONRPCRequest& request)
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key");
}

CHashWriter ss(SER_GETHASH, 0);
ss << strMessageMagic;
ss << strMessage;
std::string signature;

std::vector<unsigned char> vchSig;
if (!key.SignCompact(ss.GetHash(), vchSig))
if (!MessageSign(key, strMessage, signature)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Sign failed");
}

return EncodeBase64(vchSig.data(), vchSig.size());
return signature;
}

static UniValue setmocktime(const JSONRPCRequest& request)
Expand Down
110 changes: 110 additions & 0 deletions src/test/util_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,22 @@
#include <util/system.h>

#include <clientversion.h>
#include <hash.h> // For Hash()
#include <key.h> // For CKey
#include <optional.h>
#include <sync.h>
#include <test/util/setup_common.h>
#include <test/util/str.h>
#include <uint256.h>
#include <util/message.h> // For MessageSign(), MessageVerify(), MESSAGE_MAGIC
#include <util/moneystr.h>
#include <util/strencodings.h>
#include <util/string.h>
#include <util/time.h>
#include <util/spanparsing.h>
#include <util/vector.h>

#include <array>
#include <stdint.h>
#include <thread>
#include <univalue.h>
Expand Down Expand Up @@ -2025,4 +2030,109 @@ BOOST_AUTO_TEST_CASE(test_tracked_vector)
BOOST_CHECK_EQUAL(v8[2].copies, 0);
}

BOOST_AUTO_TEST_CASE(message_sign)
{
const std::array<unsigned char, 32> privkey_bytes = {
// just some random data
// derived address from this private key: 15CRxFdyRpGZLW9w8HnHvVduizdL5jKNbs
0xD9, 0x7F, 0x51, 0x08, 0xF1, 0x1C, 0xDA, 0x6E,
0xEE, 0xBA, 0xAA, 0x42, 0x0F, 0xEF, 0x07, 0x26,
0xB1, 0xF8, 0x98, 0x06, 0x0B, 0x98, 0x48, 0x9F,
0xA3, 0x09, 0x84, 0x63, 0xC0, 0x03, 0x28, 0x66
};

const std::string message = "Trust no one";

const std::string expected_signature =
"IPojfrX2dfPnH26UegfbGQQLrdK844DlHq5157/P6h57WyuS/Qsl+h/WSVGDF4MUi4rWSswW38oimDYfNNUBUOk=";

CKey privkey;
std::string generated_signature;

BOOST_REQUIRE_MESSAGE(!privkey.IsValid(),
"Confirm the private key is invalid");

BOOST_CHECK_MESSAGE(!MessageSign(privkey, message, generated_signature),
"Sign with an invalid private key");

privkey.Set(privkey_bytes.begin(), privkey_bytes.end(), true);

BOOST_REQUIRE_MESSAGE(privkey.IsValid(),
"Confirm the private key is valid");

BOOST_CHECK_MESSAGE(MessageSign(privkey, message, generated_signature),
"Sign with a valid private key");

BOOST_CHECK_EQUAL(expected_signature, generated_signature);
}

BOOST_AUTO_TEST_CASE(message_verify)
{
BOOST_CHECK_EQUAL(
MessageVerify(
"invalid address",
"signature should be irrelevant",
"message too"),
MessageVerificationResult::ERR_INVALID_ADDRESS);

BOOST_CHECK_EQUAL(
MessageVerify(
"3B5fQsEXEaV8v6U3ejYc8XaKXAkyQj2MjV",
"signature should be irrelevant",
"message too"),
MessageVerificationResult::ERR_ADDRESS_NO_KEY);

BOOST_CHECK_EQUAL(
MessageVerify(
"1KqbBpLy5FARmTPD4VZnDDpYjkUvkr82Pm",
"invalid signature, not in base64 encoding",
"message should be irrelevant"),
MessageVerificationResult::ERR_MALFORMED_SIGNATURE);

BOOST_CHECK_EQUAL(
MessageVerify(
"1KqbBpLy5FARmTPD4VZnDDpYjkUvkr82Pm",
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=",
"message should be irrelevant"),
MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED);

BOOST_CHECK_EQUAL(
MessageVerify(
"15CRxFdyRpGZLW9w8HnHvVduizdL5jKNbs",
"IPojfrX2dfPnH26UegfbGQQLrdK844DlHq5157/P6h57WyuS/Qsl+h/WSVGDF4MUi4rWSswW38oimDYfNNUBUOk=",
"I never signed this"),
MessageVerificationResult::ERR_NOT_SIGNED);

BOOST_CHECK_EQUAL(
MessageVerify(
"15CRxFdyRpGZLW9w8HnHvVduizdL5jKNbs",
"IPojfrX2dfPnH26UegfbGQQLrdK844DlHq5157/P6h57WyuS/Qsl+h/WSVGDF4MUi4rWSswW38oimDYfNNUBUOk=",
"Trust no one"),
MessageVerificationResult::OK);

BOOST_CHECK_EQUAL(
MessageVerify(
"11canuhp9X2NocwCq7xNrQYTmUgZAnLK3",
"IIcaIENoYW5jZWxsb3Igb24gYnJpbmsgb2Ygc2Vjb25kIGJhaWxvdXQgZm9yIGJhbmtzIAaHRtbCeDZINyavx14=",
"Trust me"),
MessageVerificationResult::OK);
}

BOOST_AUTO_TEST_CASE(message_hash)
{
const std::string unsigned_tx = "...";
const std::string prefixed_message =
std::string(1, (char)MESSAGE_MAGIC.length()) +
MESSAGE_MAGIC +
std::string(1, (char)unsigned_tx.length()) +
unsigned_tx;

const uint256 signature_hash = Hash(unsigned_tx.begin(), unsigned_tx.end());
const uint256 message_hash1 = Hash(prefixed_message.begin(), prefixed_message.end());
const uint256 message_hash2 = MessageHash(unsigned_tx);

BOOST_CHECK_EQUAL(message_hash1, message_hash2);
BOOST_CHECK_NE(message_hash1, signature_hash);
}

BOOST_AUTO_TEST_SUITE_END()
Loading