net: Avoid UBSan warning in ProcessMessage(...) #21043

practicalswift commented at 9:06 PM on January 31, 2021: contributor

Avoid UBSan warning in ProcessMessage(...).

Context: #20380 (comment) (thanks Crypt-iQ!)

DrahtBot added the label P2P on Jan 31, 2021

MarcoFalke approved

MarcoFalke commented at 6:45 AM on February 1, 2021: member

review ACK 2a241c31169e849eecb06c3bb5794fdd6ab081bb

This assumes that time is negatable, which holds for system time, but not for mocktime?

I think we should disallow negative mocktime (either here or in a follow up).

MarcoFalke commented at 7:09 AM on February 1, 2021: member

<details><summary>patch</summary>

commit eea2bf75949fd3b391711b9092e01a1843245db2
Author: MarcoFalke <falke.marco@gmail.com>
Date:   Mon Feb 1 08:03:24 2021 +0100

    util: Disallow negative mocktime

diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index b75a7b8d26..38a0bddddb 100644
--- a/src/rpc/misc.cpp
+++ b/src/rpc/misc.cpp
@@ -365,13 +365,13 @@ static RPCHelpMan signmessagewithprivkey()
 static RPCHelpMan setmocktime()
 {
     return RPCHelpMan{"setmocktime",
-                "\nSet the local time to given timestamp (-regtest only)\n",
-                {
-                    {"timestamp", RPCArg::Type::NUM, RPCArg::Optional::NO, UNIX_EPOCH_TIME + "\n"
-            "   Pass 0 to go back to using the system time."},
-                },
-                RPCResult{RPCResult::Type::NONE, "", ""},
-                RPCExamples{""},
+        "\nSet the local time to given timestamp (-regtest only)\n",
+        {
+            {"timestamp", RPCArg::Type::NUM, RPCArg::Optional::NO, UNIX_EPOCH_TIME + "\n"
+             "Pass 0 to go back to using the system time."},
+        },
+        RPCResult{RPCResult::Type::NONE, "", ""},
+        RPCExamples{""},
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
     if (!Params().IsMockableChain()) {
@@ -386,7 +386,10 @@ static RPCHelpMan setmocktime()
     LOCK(cs_main);
 
     RPCTypeCheck(request.params, {UniValue::VNUM});
-    int64_t time = request.params[0].get_int64();
+    const int64_t time{request.params[0].get_int64()};
+    if (time < 0) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime can not be negative: %s.", time));
+    }
     SetMockTime(time);
     if (request.context.Has<NodeContext>()) {
         for (const auto& chain_client : request.context.Get<NodeContext>().chain_clients) {
diff --git a/src/util/time.cpp b/src/util/time.cpp
index e96972fe12..d130e4e4d4 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -9,6 +9,8 @@
 
 #include <util/time.h>
 
+#include <util/check.h>
+
 #include <atomic>
 #include <boost/date_time/posix_time/posix_time.hpp>
 #include <ctime>
@@ -18,7 +20,7 @@
 
 void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
 
-static std::atomic<int64_t> nMockTime(0); //!< For unit testing
+static std::atomic<int64_t> nMockTime(0); //!< For testing
 
 int64_t GetTime()
 {
@@ -46,6 +48,7 @@ template std::chrono::microseconds GetTime();
 
 void SetMockTime(int64_t nMockTimeIn)
 {
+    Assert(nMockTimeIn >= 0);
     nMockTime.store(nMockTimeIn, std::memory_order_relaxed);
 }
 
diff --git a/test/functional/rpc_uptime.py b/test/functional/rpc_uptime.py
index e86f91b1d0..6177970872 100755
--- a/test/functional/rpc_uptime.py
+++ b/test/functional/rpc_uptime.py
@@ -10,6 +10,7 @@ Test corresponds to code in rpc/server.cpp.
 import time
 
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_raises_rpc_error
 
 
 class UptimeTest(BitcoinTestFramework):
@@ -18,8 +19,12 @@ class UptimeTest(BitcoinTestFramework):
         self.setup_clean_chain = True
 
     def run_test(self):
+        self._test_negative_time()
         self._test_uptime()
 
+    def _test_negative_time(self):
+        assert_raises_rpc_error(-8, "Mocktime can not be negative: -1.", self.nodes[0].setmocktime, -1)
+
     def _test_uptime(self):
         wait_time = 10
         self.nodes[0].setmocktime(int(time.time() + wait_time))

</details>

MarcoFalke added the label Needs backport (0.21) on Feb 1, 2021

MarcoFalke added this to the milestone 0.21.1 on Feb 1, 2021

practicalswift commented at 3:00 PM on February 1, 2021: contributor

This assumes that time is negatable, which holds for system time, but not for mocktime?

I think we should disallow negative mocktime (either here or in a follow up).

Good point! I've now added your patch as a second commit. Thanks!

net: Avoid UBSan warning in ProcessMessage(...) f5f2f97168

util: Disallow negative mocktime

Signed-off-by: practicalswift <practicalswift@users.noreply.github.com>

3ddbf22ed1

practicalswift force-pushed on Feb 2, 2021

MarcoFalke commented at 12:41 PM on February 2, 2021: member

re-ACK 3ddbf22ed179a2db733af4b521bec5d2b13ebf4b only change is adding patch written by me

ajtowns commented at 5:59 AM on February 4, 2021: member

ACK 3ddbf22ed179a2db733af4b521bec5d2b13ebf4b -- code review only

in src/net_processing.cpp:2494 in 3ddbf22ed1

2488 | @@ -2489,6 +2489,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2489 |          bool fRelay = true;
2490 |  
2491 |          vRecv >> nVersion >> nServiceInt >> nTime >> addrMe;
2492 | +        if (nTime < 0) {
2493 | +            nTime = 0;
2494 | +        }

vasild commented at 2:07 PM on February 5, 2021:

This will fix the overflow warning indeed.

However 0 is arbitrary choice here because -10000, 5 and 200 would also fix it. A negative nTime from the peer means he pretends that his time is pre-1970 and we just skew that to Jan 1 1970, replacing one garbage with another garbage. Then we feed that garbage to AddTimeData() which only consumes the first 200 samples and ignores all calls afterwards :raised_eyebrow:.

So, it makes sense to avoid calling AddTimeData() with input we know for sure is garbage. What about something like the following?

        if (nTime > 1609459200 /* Fri Jan  1 00:00:00 UTC 2021 */) {
            int64_t nTimeOffset = nTime - GetTime();
            pfrom.nTimeOffset = nTimeOffset;
            AddTimeData(pfrom.addr, nTimeOffset);
        } else {
            // Ignore garbage time.
            pfrom.nTimeOffset = 0;
        }

MarcoFalke commented at 3:39 PM on February 5, 2021:

1609459200 seems arbitrary here and certainly more confusing than 0. AddTimeData already deals with garbage values correctly, so I'd rather not add special case-code that doesn't protect against anything.

vasild commented at 4:05 PM on February 5, 2021:

1609459200 draws a line between "for sure garbage" and "may be garbage or may be not", but yes, it is arbitrary in a sense that 1609459205 would do the same job.

AddTimeData already deals with garbage values correctly

How? The garbage will consume one of its 200 slots.

MarcoFalke commented at 4:09 PM on February 5, 2021:

How? The garbage will consume one of its 200 slots.

If you'd like to change that, it might be better to change it inside of the timedata module in a separate pull?

vasild commented at 4:28 PM on February 5, 2021:

Yes, inside AddTimeData() would be better.

AddTimeData() would accept absolute time as received by the peer (not offset), would check for possible garbage and return the offset (or 0 if garbage) and also would avoid occupying one of the 200 slots if garbage.

-        int64_t nTimeOffset = nTime - GetTime();
-        pfrom.nTimeOffset = nTimeOffset;
-        AddTimeData(pfrom.addr, nTimeOffset);
+        pfrom.nTimeOffset = AddTimeData(pfrom.addr, nTime);

This would make the change in this PR which checks for negative nTime unnecessary, so maybe makes sense to do it in this PR, rather than in a followup one, which reverts this one?

MarcoFalke commented at 5:30 PM on February 5, 2021:

would check for possible garbage

Again, if you think this is possible to solve, please suggest an actual patch. Be aware that honest nodes likely won't send you garbage unless by accident. Dishonest nodes (let's say with the goal of filling the vector with "garbage") can still do so with your proposed solution: "1609459200 draws a line between "for sure garbage" and "may be garbage or may be not"".

practicalswift commented at 7:47 PM on February 6, 2021:

@vasild You're correct that 0 is one of many possible choices. 0 was picked since it seemed like the least surprising and/or confusing choice. It also seemed like a reasonable number from a nothing-up-my-sleeve perspective.

vasild commented at 3:41 PM on February 8, 2021:

Yes, this will protect against an accidental error. Malicious peers can still send garbage by pretending it is year 2050.

Feel free to ignore this, if you see no merit (on top of this PR):

<details> <summary>[patch] avoid integer underflow and protect time data samples from definite garbage</summary>

diff --git i/src/timedata.h w/src/timedata.h
index b165ecde2..c39c0a90d 100644
--- i/src/timedata.h
+++ w/src/timedata.h
@@ -70,9 +70,16 @@ public:
     }
 };
 
 /** Functions to keep track of adjusted P2P time */
 int64_t GetTimeOffset();
 int64_t GetAdjustedTime();
-void AddTimeData(const CNetAddr& ip, int64_t nTime);
+
+/**
+ * Consume a time sample from a peer.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] ip The address of the peer, repeated samples from the same peer are ignored.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] nTime The time of the peer (number of seconds size 1970).
+ * [@return](/bitcoin-bitcoin/contributor/return/) the difference between the peer's time and our time in seconds or 0 if nTime is nonsensical.
+ */
+int64_t AddTimeData(const CNetAddr& ip, int64_t nTime);
 
 #endif // BITCOIN_TIMEDATA_H
diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index 8936b8109..fda3b5abd 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -2486,15 +2486,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         int nVersion;
         std::string cleanSubVer;
         int starting_height = -1;
         bool fRelay = true;
 
         vRecv >> nVersion >> nServiceInt >> nTime >> addrMe;
-        if (nTime < 0) {
-            nTime = 0;
-        }
         nServices = ServiceFlags(nServiceInt);
         if (!pfrom.IsInboundConn())
         {
             m_connman.SetServices(pfrom.addr, nServices);
         }
         if (pfrom.ExpectServicesFromConn() && !HasAllDesirableServiceFlags(nServices))
@@ -2648,15 +2645,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         LogPrint(BCLog::NET, "receive version message: %s: version %d, blocks=%d, us=%s, txrelay=%d, peer=%d%s\n",
                   cleanSubVer, pfrom.nVersion,
                   peer->m_starting_height, addrMe.ToString(), fRelay, pfrom.GetId(),
                   remoteAddr);
 
-        int64_t nTimeOffset = nTime - GetTime();
-        pfrom.nTimeOffset = nTimeOffset;
-        AddTimeData(pfrom.addr, nTimeOffset);
+        pfrom.nTimeOffset = AddTimeData(pfrom.addr, nTime);
 
         // If the peer is old enough to have the old alert system, send it the final alert.
         if (greatest_common_version <= 70012) {
             CDataStream finalAlert(ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50"), SER_NETWORK, PROTOCOL_VERSION);
             m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", finalAlert));
         }
diff --git i/src/timedata.cpp w/src/timedata.cpp
index 354092752..3195baafc 100644
--- i/src/timedata.cpp
+++ w/src/timedata.cpp
@@ -35,21 +35,28 @@ int64_t GetAdjustedTime()
 {
     return GetTime() + GetTimeOffset();
 }
 
 #define BITCOIN_TIMEDATA_MAX_SAMPLES 200
 
-void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
+int64_t AddTimeData(const CNetAddr& ip, int64_t nTime)
 {
+    if (nTime < 1609459200 /* Fri Jan  1 00:00:00 UTC 2021 */) {
+        // Do not consume nonsensical times as we have a limited number of slots.
+        return 0;
+    }
+
+    const int64_t nOffsetSample = nTime - GetTime();
+
     LOCK(g_timeoffset_mutex);
     // Ignore duplicates
     static std::set<CNetAddr> setKnown;
     if (setKnown.size() == BITCOIN_TIMEDATA_MAX_SAMPLES)
-        return;
+        return nOffsetSample;
     if (!setKnown.insert(ip).second)
-        return;
+        return nOffsetSample;
 
     // Add data
     static CMedianFilter<int64_t> vTimeOffsets(BITCOIN_TIMEDATA_MAX_SAMPLES, 0);
     vTimeOffsets.input(nOffsetSample);
     LogPrint(BCLog::NET, "added time data, samples %d, offset %+d (%+d minutes)\n", vTimeOffsets.size(), nOffsetSample, nOffsetSample / 60);
 
@@ -102,7 +109,9 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
                 LogPrint(BCLog::NET, "%+d  ", n); /* Continued */
             }
             LogPrint(BCLog::NET, "|  "); /* Continued */
             LogPrint(BCLog::NET, "nTimeOffset = %+d  (%+d minutes)\n", nTimeOffset, nTimeOffset / 60);
         }
     }
+
+    return nOffsetSample;
 }
diff --git i/src/test/timedata_tests.cpp w/src/test/timedata_tests.cpp
index 1dcee23bb..552d4fc63 100644
--- i/src/test/timedata_tests.cpp
+++ w/src/test/timedata_tests.cpp
@@ -6,12 +6,13 @@
 #include <netaddress.h>
 #include <noui.h>
 #include <test/util/logging.h>
 #include <test/util/setup_common.h>
 #include <timedata.h>
 #include <util/string.h>
+#include <util/time.h>
 #include <util/translation.h>
 #include <warnings.h>
 
 #include <string>
 
 #include <boost/test/unit_test.hpp>
@@ -46,13 +47,13 @@ BOOST_AUTO_TEST_CASE(util_MedianFilter)
 static void MultiAddTimeData(int n, int64_t offset)
 {
     static int cnt = 0;
     for (int i = 0; i < n; ++i) {
         CNetAddr addr;
         addr.SetInternal(ToString(++cnt));
-        AddTimeData(addr, offset);
+        AddTimeData(addr, GetTime() + offset);
     }
 }
 
 
 BOOST_AUTO_TEST_CASE(addtimedata)
 {

</details>

practicalswift commented at 3:58 PM on February 8, 2021:

@vasild Thanks for providing a patch, but I'll leave this PR unchanged to avoid expanding the scope beyond fixing the UBSan warning. I want to keep this PR minimal and laser focused in order to make review trivial :)

MarcoFalke merged this on Feb 11, 2021

MarcoFalke closed this on Feb 11, 2021

MarcoFalke removed the label Needs backport (0.21) on Feb 11, 2021

MarcoFalke referenced this in commit 95218ee95c on Feb 11, 2021

MarcoFalke referenced this in commit 08dada8456 on Feb 11, 2021

MarcoFalke commented at 11:43 AM on February 11, 2021: member

Backported in #20901

sidhujag referenced this in commit 35deaebd8a on Feb 11, 2021

practicalswift deleted the branch on Apr 10, 2021

Fabcien referenced this in commit 1084310190 on Jul 22, 2022

DrahtBot locked this on Aug 16, 2022