Refactor message hashing into a utility function

jkczyz · vasild · commit e193a84fb280 · 2020-02-14T10:45:41.000+01:00
And add unit test for it.

The purpose of using a preamble or "magic" text as part of signing and
verifying a message was not given when the code was repeated in a few
locations. Make a test showing how it is used to prevent inadvertently
signing a transaction.
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
@@ -5,12 +5,14 @@
 #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 <util/message.h> // For MessageSign(), MessageVerify()
+#include <uint256.h>
+#include <util/message.h> // For MessageSign(), MessageVerify(), MESSAGE_MAGIC
 #include <util/moneystr.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -2116,4 +2118,21 @@ BOOST_AUTO_TEST_CASE(message_verify)
         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()
diff --git a/src/util/message.cpp b/src/util/message.cpp
@@ -15,7 +15,11 @@
 #include <string>
 #include <vector>
 
-const std::string strMessageMagic = "Bitcoin Signed Message:\n";
+/**
+ * Text used to signify that a signed message follows and to prevent
+ * inadvertently signing a transaction.
+ */
+const std::string MESSAGE_MAGIC = "Bitcoin Signed Message:\n";
 
 MessageVerificationResult MessageVerify(
     const std::string& address,
@@ -37,12 +41,8 @@ MessageVerificationResult MessageVerify(
         return MessageVerificationResult::ERR_MALFORMED_SIGNATURE;
     }
 
-    CHashWriter ss(SER_GETHASH, 0);
-    ss << strMessageMagic;
-    ss << message;
-
     CPubKey pubkey;
-    if (!pubkey.RecoverCompact(ss.GetHash(), signature_bytes)) {
+    if (!pubkey.RecoverCompact(MessageHash(message), signature_bytes)) {
         return MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED;
     }
 
@@ -58,17 +58,21 @@ bool MessageSign(
     const std::string& message,
     std::string& signature)
 {
-    CHashWriter ss(SER_GETHASH, 0);
-    ss << strMessageMagic;
-    ss << message;
-
     std::vector<unsigned char> signature_bytes;
 
-    if (!privkey.SignCompact(ss.GetHash(), signature_bytes)) {
+    if (!privkey.SignCompact(MessageHash(message), signature_bytes)) {
         return false;
     }
 
     signature = EncodeBase64(signature_bytes.data(), signature_bytes.size());
 
     return true;
 }
+
+uint256 MessageHash(const std::string& message)
+{
+    CHashWriter hasher(SER_GETHASH, 0);
+    hasher << MESSAGE_MAGIC << message;
+
+    return hasher.GetHash();
+}
diff --git a/src/util/message.h b/src/util/message.h
@@ -7,10 +7,11 @@
 #define BITCOIN_UTIL_MESSAGE_H
 
 #include <key.h> // For CKey
+#include <uint256.h>
 
 #include <string>
 
-extern const std::string strMessageMagic;
+extern const std::string MESSAGE_MAGIC;
 
 /** The result of a signed message verification.
  * Message verification takes as an input:
@@ -58,4 +59,10 @@ bool MessageSign(
     const std::string& message,
     std::string& signature);
 
+/**
+ * Hashes a message for signing and verification in a manner that prevents
+ * inadvertently signing a transaction.
+ */
+uint256 MessageHash(const std::string& message);
+
 #endif // BITCOIN_UTIL_MESSAGE_H
