From 3a816398b326bc47fd8c405c491c0f6d82153fa0 Mon Sep 17 00:00:00 2001 From: anonimal Date: Fri, 6 Sep 2019 22:48:16 +0000 Subject: [PATCH 1/6] epee: connection_basic: resolve CID 203920 (UNINIT_CTOR) --- contrib/epee/src/connection_basic.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/epee/src/connection_basic.cpp b/contrib/epee/src/connection_basic.cpp index 7526dde26..2ff757f30 100644 --- a/contrib/epee/src/connection_basic.cpp +++ b/contrib/epee/src/connection_basic.cpp @@ -136,6 +136,7 @@ connection_basic::connection_basic(boost::asio::ip::tcp::socket&& sock, std::sha socket_(GET_IO_SERVICE(sock), get_context(m_state.get())), m_want_close_connection(false), m_was_shutdown(false), + m_is_multithreaded(false), m_ssl_support(ssl_support) { // add nullptr checks if removed From 1bd962d9f9037ed20fdf588d3b91ce36567f4867 Mon Sep 17 00:00:00 2001 From: anonimal Date: Fri, 6 Sep 2019 23:11:37 +0000 Subject: [PATCH 2/6] wallet2: resolve CID 203918 null pointer deference (NULL_RETURNS) --- src/wallet/wallet2.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index b99fc12e2..009a0441f 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -13143,6 +13143,12 @@ bool wallet2::save_to_file(const std::string& path_to_file, const std::string& r } FILE *fp = fopen(path_to_file.c_str(), "w+"); + if (!fp) + { + MERROR("Failed to open wallet file for writing: " << path_to_file << ": " << strerror(errno)); + return false; + } + // Save the result b/c we need to close the fp before returning success/failure. int write_result = PEM_write(fp, ASCII_OUTPUT_MAGIC.c_str(), "", (const unsigned char *) raw.c_str(), raw.length()); fclose(fp); From 2825f07d95ae32110486905232b7a64609cfa6ea Mon Sep 17 00:00:00 2001 From: anonimal Date: Fri, 6 Sep 2019 23:18:00 +0000 Subject: [PATCH 3/6] epee: connection_basic: resolve CID 203916 (UNINIT_CTOR) --- contrib/epee/src/connection_basic.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/epee/src/connection_basic.cpp b/contrib/epee/src/connection_basic.cpp index 2ff757f30..3ce7a1057 100644 --- a/contrib/epee/src/connection_basic.cpp +++ b/contrib/epee/src/connection_basic.cpp @@ -161,6 +161,7 @@ connection_basic::connection_basic(boost::asio::io_service &io_service, std::sha socket_(io_service, get_context(m_state.get())), m_want_close_connection(false), m_was_shutdown(false), + m_is_multithreaded(false), m_ssl_support(ssl_support) { // add nullptr checks if removed From d099658522c6f17055a3b9977f74c6e91c6c8199 Mon Sep 17 00:00:00 2001 From: anonimal Date: Sat, 7 Sep 2019 00:29:09 +0000 Subject: [PATCH 4/6] bootstrap_daemon: resolve CID 203915 (UNCAUGHT_EXCEPT) The issue is triggered by the captured `this` in RPC server, which passes reference to throwable `core_rpc_server`: `core_rpc_server.cpp:164: m_bootstrap_daemon.reset(new bootstrap_daemon([this]{ return get_random_public_node(); }));` The solution is to simply remove noexcept from the remaining `bootstrap_daemon` constructors because noexcept is false in this context. >"An exception of type "boost::exception_detail::clone_impl>" is thrown but the throw list "noexcept" doesn't allow it to be thrown. This will cause a call to unexpected() which usually calls terminate()." --- src/rpc/bootstrap_daemon.cpp | 2 +- src/rpc/bootstrap_daemon.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/bootstrap_daemon.cpp b/src/rpc/bootstrap_daemon.cpp index 6f8426ee7..c97b2c95a 100644 --- a/src/rpc/bootstrap_daemon.cpp +++ b/src/rpc/bootstrap_daemon.cpp @@ -12,7 +12,7 @@ namespace cryptonote { - bootstrap_daemon::bootstrap_daemon(std::function()> get_next_public_node) noexcept + bootstrap_daemon::bootstrap_daemon(std::function()> get_next_public_node) : m_get_next_public_node(get_next_public_node) { } diff --git a/src/rpc/bootstrap_daemon.h b/src/rpc/bootstrap_daemon.h index 130a6458d..6276b1b21 100644 --- a/src/rpc/bootstrap_daemon.h +++ b/src/rpc/bootstrap_daemon.h @@ -15,7 +15,7 @@ namespace cryptonote class bootstrap_daemon { public: - bootstrap_daemon(std::function()> get_next_public_node) noexcept; + bootstrap_daemon(std::function()> get_next_public_node); bootstrap_daemon(const std::string &address, const boost::optional &credentials); std::string address() const noexcept; From d46f7015151ccec7536c94a0dc0eb6e4342ec1c3 Mon Sep 17 00:00:00 2001 From: anonimal Date: Sat, 7 Sep 2019 00:38:49 +0000 Subject: [PATCH 5/6] tests: rct_mlsag: resolve CID 203914 (UNINIT_CTOR) --- tests/performance_tests/rct_mlsag.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/performance_tests/rct_mlsag.h b/tests/performance_tests/rct_mlsag.h index 59eae074e..da0c064fd 100644 --- a/tests/performance_tests/rct_mlsag.h +++ b/tests/performance_tests/rct_mlsag.h @@ -82,6 +82,6 @@ public: private: rct::keyV sk; rct::keyM P; - size_t ind; + size_t ind{}; rct::mgSig IIccss; }; From cd57a10c90134bbe67d35cbe71037d9ca42427ad Mon Sep 17 00:00:00 2001 From: anonimal Date: Sat, 7 Sep 2019 01:35:47 +0000 Subject: [PATCH 6/6] epee: abstract_tcp_server2: resolve CID 203919 (DC.WEAK_CRYPTO) The problem actually exists in two parts: 1. When sending chunks over a connection, if the queue size is greater than N, the seed is predictable across every monero node. >"If rand() is used before any calls to srand(), rand() behaves as if it was seeded with srand(1). Each time rand() is seeded with the same seed, it must produce the same sequence of values." 2. The CID speaks for itself: "'rand' should not be used for security-related applications, because linear congruential algorithms are too easy to break." *But* this is an area of contention. One could argue that a CSPRNG is warranted in order to fully mitigate any potential timing attacks based on crafting chunk responses. Others could argue that the existing LCG, or even an MTG, would suffice (if properly seeded). As a compromise, I've used an MTG with a full bit space. This should give a healthy balance of security and speed without relying on the existing crypto library (which I'm told might break on some systems since epee is not (shouldn't be) dependent upon the existing crypto library). --- contrib/epee/include/net/abstract_tcp_server2.inl | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/contrib/epee/include/net/abstract_tcp_server2.inl b/contrib/epee/include/net/abstract_tcp_server2.inl index 12a87071a..bc1898e0e 100644 --- a/contrib/epee/include/net/abstract_tcp_server2.inl +++ b/contrib/epee/include/net/abstract_tcp_server2.inl @@ -50,6 +50,8 @@ #include #include #include +#include +#include #undef MONERO_DEFAULT_LOG_CATEGORY #define MONERO_DEFAULT_LOG_CATEGORY "net" @@ -628,7 +630,17 @@ PRAGMA_WARNING_DISABLE_VS(4355) return false; // aborted }*/ - long int ms = 250 + (rand()%50); + using engine = std::mt19937; + + engine rng; + std::random_device dev; + std::seed_seq::result_type rand[engine::state_size]{}; // Use complete bit space + + std::generate_n(rand, engine::state_size, std::ref(dev)); + std::seed_seq seed(rand, rand + engine::state_size); + rng.seed(seed); + + long int ms = 250 + (rng() % 50); MDEBUG("Sleeping because QUEUE is FULL, in " << __FUNCTION__ << " for " << ms << " ms before packet_size="<