Merge pull request #769

b4eada9 wallet: make load_keys check types when loading JSON (moneromooo-monero)
3e55725 wallet: make the JSON reading type safe (moneromooo-monero)
f8d05f3 common: new json_util.h (moneromooo-monero)

4 files changed, 122 insertions, 64 deletions

diff --git a/src/common/json_util.h b/src/common/json_util.h
new file mode 100644
index 000000000..6f8b4c18f
--- /dev/null
+++ b/src/common/json_util.h
@@ -0,0 +1,53 @@
+// Copyright (c) 2016, The Monero Project
+// 
+// All rights reserved.
+// 
+// Redistribution and use in source and binary forms, with or without modification, are
+// permitted provided that the following conditions are met:
+// 
+// 1. Redistributions of source code must retain the above copyright notice, this list of
+//    conditions and the following disclaimer.
+// 
+// 2. Redistributions in binary form must reproduce the above copyright notice, this list
+//    of conditions and the following disclaimer in the documentation and/or other
+//    materials provided with the distribution.
+// 
+// 3. Neither the name of the copyright holder nor the names of its contributors may be
+//    used to endorse or promote products derived from this software without specific
+//    prior written permission.
+// 
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
+// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#pragma once 
+
+#define GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, name, type, jtype, mandatory) \
+  type field_##name; \
+  bool field_##name##_found = false; \
+  (void)field_##name##_found; \
+  do if (json.HasMember(#name)) \
+  { \
+    if (json[#name].Is##jtype()) \
+    { \
+      field_##name = json[#name].Get##jtype(); \
+      field_##name##_found = true; \
+    } \
+    else \
+    { \
+      LOG_ERROR("Field " << #name << " found in JSON, but not " << #jtype); \
+      return false; \
+    } \
+  } \
+  else if (mandatory) \
+  { \
+    LOG_ERROR("Field " << #name << " not found in JSON"); \
+    return false; \
+  } while(0)
+
diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp
index d05b5ea4c..5b7be9029 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -58,6 +58,7 @@
 #include "crypto/crypto.h"  // for crypto::secret_key definition
 #include "mnemonics/electrum-words.h"
 #include "rapidjson/document.h"
+#include "common/json_util.h"
 #include <stdexcept>
 
 #if defined(WIN32)
@@ -833,40 +834,27 @@ bool simple_wallet::generate_from_json(const boost::program_options::variables_m
     return false;
   }
 
-  if (!json.HasMember("version")) {
-    fail_msg_writer() << tr("Version not found in JSON");
-    return false;
-  }
-  unsigned int version = json["version"].GetUint();
+  GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, version, unsigned, Uint, true);
   const int current_version = 1;
-  if (version > current_version) {
-    fail_msg_writer() << boost::format(tr("Version %u too new, we can only grok up to %u")) % version % current_version;
-    return false;
-  }
-  if (!json.HasMember("filename")) {
-    fail_msg_writer() << tr("Filename not found in JSON");
+  if (field_version > current_version) {
+    fail_msg_writer() << boost::format(tr("Version %u too new, we can only grok up to %u")) % field_version % current_version;
     return false;
   }
-  std::string filename = json["filename"].GetString();
 
-  bool recover = false;
-  uint64_t scan_from_height = 0;
-  if (json.HasMember("scan_from_height")) {
-    scan_from_height = json["scan_from_height"].GetUint64();
-    recover = true;
-  }
+  GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, filename, std::string, String, true);
 
-  password = "";
-  if (json.HasMember("password")) {
-    password = json["password"].GetString();
-  }
+  GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, scan_from_height, uint64_t, Uint64, false);
+  bool recover = field_scan_from_height_found;
 
-  std::string viewkey_string("");
+  GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, password, std::string, String, false);
+  password = field_password;
+
+  GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, viewkey, std::string, String, false);
   crypto::secret_key viewkey;
-  if (json.HasMember("viewkey")) {
-    viewkey_string = json["viewkey"].GetString();
+  if (field_viewkey_found)
+  {
     cryptonote::blobdata viewkey_data;
-    if(!epee::string_tools::parse_hexstr_to_binbuff(viewkey_string, viewkey_data))
+    if(!epee::string_tools::parse_hexstr_to_binbuff(field_viewkey, viewkey_data))
     {
       fail_msg_writer() << tr("failed to parse view key secret key");
       return false;
@@ -874,12 +862,12 @@ bool simple_wallet::generate_from_json(const boost::program_options::variables_m
     viewkey = *reinterpret_cast<const crypto::secret_key*>(viewkey_data.data());
   }
 
-  std::string spendkey_string("");
+  GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, spendkey, std::string, String, false);
   crypto::secret_key spendkey;
-  if (json.HasMember("spendkey")) {
-    spendkey_string = json["spendkey"].GetString();
+  if (field_spendkey_found)
+  {
     cryptonote::blobdata spendkey_data;
-    if(!epee::string_tools::parse_hexstr_to_binbuff(spendkey_string, spendkey_data))
+    if(!epee::string_tools::parse_hexstr_to_binbuff(field_spendkey, spendkey_data))
     {
       fail_msg_writer() << tr("failed to parse spend key secret key");
       return false;
@@ -887,30 +875,32 @@ bool simple_wallet::generate_from_json(const boost::program_options::variables_m
     spendkey = *reinterpret_cast<const crypto::secret_key*>(spendkey_data.data());
   }
 
-  std::string seed("");
+  GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, seed, std::string, String, false);
   std::string old_language;
-  if (json.HasMember("seed")) {
-    seed = json["seed"].GetString();
-    if (!crypto::ElectrumWords::words_to_bytes(seed, m_recovery_key, old_language))
+  if (field_seed_found)
+  {
+    if (!crypto::ElectrumWords::words_to_bytes(field_seed, m_recovery_key, old_language))
     {
       fail_msg_writer() << tr("Electrum-style word list failed verification");
       return false;
     }
-    m_electrum_seed = seed;
+    m_electrum_seed = field_seed;
     m_restore_deterministic_wallet = true;
   }
 
   // compatibility checks
-  if (seed.empty() && viewkey_string.empty()) {
+  if (!field_seed_found && !field_viewkey_found)
+  {
     fail_msg_writer() << tr("At least one of Electrum-style word list and private view key must be specified");
     return false;
   }
-  if (!seed.empty() && (!viewkey_string.empty() || !spendkey_string.empty())) {
+  if (field_seed_found && (field_viewkey_found || field_spendkey_found))
+  {
     fail_msg_writer() << tr("Both Electrum-style word list and private key(s) specified");
     return false;
   }
 
-  m_wallet_file = filename;
+  m_wallet_file = field_filename;
 
   bool was_deprecated_wallet = m_restore_deterministic_wallet && ((old_language == crypto::ElectrumWords::old_language_name) ||
     crypto::ElectrumWords::get_is_old_style_seed(m_electrum_seed));
@@ -925,7 +915,7 @@ bool simple_wallet::generate_from_json(const boost::program_options::variables_m
 
   try
   {
-    if (!seed.empty())
+    if (!field_seed.empty())
     {
       m_wallet->generate(m_wallet_file, password, m_recovery_key, recover, false);
     }
@@ -941,7 +931,7 @@ bool simple_wallet::generate_from_json(const boost::program_options::variables_m
         return false;
       }
 
-      if (spendkey_string.empty())
+      if (field_spendkey.empty())
       {
         m_wallet->generate(m_wallet_file, password, address, viewkey);
       }
@@ -957,7 +947,7 @@ bool simple_wallet::generate_from_json(const boost::program_options::variables_m
     return false;
   }
 
-  m_wallet->set_refresh_from_block_height(scan_from_height);
+  m_wallet->set_refresh_from_block_height(field_scan_from_height);
 
   wallet_file = m_wallet_file;
 
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 95a160434..a9a65535f 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -52,6 +52,7 @@ using namespace epee;
 #include "rapidjson/document.h"
 #include "rapidjson/writer.h"
 #include "rapidjson/stringbuffer.h"
+#include "common/json_util.h"
 
 extern "C"
 {
@@ -1004,7 +1005,7 @@ namespace
  * \param keys_file_name Name of wallet file
  * \param password       Password of wallet file
  */
-void wallet2::load_keys(const std::string& keys_file_name, const std::string& password)
+bool wallet2::load_keys(const std::string& keys_file_name, const std::string& password)
 {
   wallet2::keys_file_data keys_file_data;
   std::string buf;
@@ -1033,34 +1034,44 @@ void wallet2::load_keys(const std::string& keys_file_name, const std::string& pa
   }
   else
   {
-    account_data = std::string(json["key_data"].GetString(), json["key_data"].GetString() +
-      json["key_data"].GetStringLength());
-    if (json.HasMember("seed_language"))
+    if (!json.HasMember("key_data"))
     {
-      set_seed_language(std::string(json["seed_language"].GetString(), json["seed_language"].GetString() +
-        json["seed_language"].GetStringLength()));
+      LOG_ERROR("Field key_data not found in JSON");
+      return false;
     }
-    if (json.HasMember("watch_only"))
+    if (!json["key_data"].IsString())
     {
-      m_watch_only = json["watch_only"].GetInt() != 0;
+      LOG_ERROR("Field key_data found in JSON, but not String");
+      return false;
     }
-    else
+    const char *field_key_data = json["key_data"].GetString();
+    account_data = std::string(field_key_data, field_key_data + json["key_data"].GetStringLength());
+
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, seed_language, std::string, String, false);
+    if (field_seed_language_found)
     {
-      m_watch_only = false;
-    }
-    m_always_confirm_transfers = json.HasMember("always_confirm_transfers") && (json["always_confirm_transfers"].GetInt() != 0);
-    m_store_tx_info = (json.HasMember("store_tx_keys") && (json["store_tx_keys"].GetInt() != 0))
-                   || (json.HasMember("store_tx_info") && (json["store_tx_info"].GetInt() != 0));
-    m_default_mixin = json.HasMember("default_mixin") ? json["default_mixin"].GetUint() : 0;
-    m_auto_refresh = !json.HasMember("auto_refresh") || (json["auto_refresh"].GetInt() != 0);
+      set_seed_language(field_seed_language);
+    }
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, watch_only, int, Int, false);
+    m_watch_only = field_watch_only_found && field_watch_only;
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, always_confirm_transfers, int, Int, false);
+    m_always_confirm_transfers = field_always_confirm_transfers_found && field_always_confirm_transfers;
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, store_tx_keys, int, Int, false);
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, store_tx_info, int, Int, false);
+    m_store_tx_info = (field_store_tx_keys_found && (field_store_tx_keys != 0))
+                   || (field_store_tx_info_found && (field_store_tx_info != 0));
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, default_mixin, unsigned int, Uint, false);
+    m_default_mixin = field_default_mixin_found ? field_default_mixin : 0;
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, auto_refresh, int, Int, false);
+    m_auto_refresh = !field_auto_refresh_found || (field_auto_refresh != 0);
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, refresh_type, int, Int, false);
     m_refresh_type = RefreshType::RefreshDefault;
-    if (json.HasMember("refresh_type"))
+    if (field_refresh_type_found)
     {
-      int type = json["refresh_type"].GetInt();
-      if (type == RefreshFull || type == RefreshOptimizeCoinbase || type == RefreshNoCoinbase)
-        m_refresh_type = (RefreshType)type;
+      if (field_refresh_type == RefreshFull || field_refresh_type == RefreshOptimizeCoinbase || field_refresh_type == RefreshNoCoinbase)
+        m_refresh_type = (RefreshType)field_refresh_type;
       else
-        LOG_PRINT_L0("Unknown refresh-type value (" << type << "), using default");
+        LOG_PRINT_L0("Unknown refresh-type value (" << field_refresh_type << "), using default");
     }
   }
 
@@ -1070,6 +1081,7 @@ void wallet2::load_keys(const std::string& keys_file_name, const std::string& pa
   if(!m_watch_only)
     r = r && verify_keys(keys.m_spend_secret_key, keys.m_account_address.m_spend_public_key);
   THROW_WALLET_EXCEPTION_IF(!r, error::invalid_password);
+  return true;
 }
 
 /*!
@@ -1358,7 +1370,10 @@ void wallet2::load(const std::string& wallet_, const std::string& password)
   bool exists = boost::filesystem::exists(m_keys_file, e);
   THROW_WALLET_EXCEPTION_IF(e || !exists, error::file_not_found, m_keys_file);
 
-  load_keys(m_keys_file, password);
+  if (!load_keys(m_keys_file, password))
+  {
+    THROW_WALLET_EXCEPTION_IF(true, error::file_read_error, m_keys_file);
+  }
   LOG_PRINT_L0("Loaded wallet keys file, with public address: " << m_account.get_public_address_str(m_testnet));
 
   //keys loaded ok!
diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h
index fc700a3de..b6466d3f6 100644
--- a/src/wallet/wallet2.h
+++ b/src/wallet/wallet2.h
@@ -378,7 +378,7 @@ namespace tools
      * \param keys_file_name Name of wallet file
      * \param password       Password of wallet file
      */
-    void load_keys(const std::string& keys_file_name, const std::string& password);
+    bool load_keys(const std::string& keys_file_name, const std::string& password);
     void process_new_transaction(const cryptonote::transaction& tx, uint64_t height, bool miner_tx);
     void process_new_blockchain_entry(const cryptonote::block& b, const cryptonote::block_complete_entry& bche, const crypto::hash& bl_id, uint64_t height);
     void detach_blockchain(uint64_t height);