p2p: Don’t use timestamps from inbound peers for Adjusted Time #23631

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202111_timedata_inbound changing 1 files +5 −1
  1. mzumsande commented at 10:05 pm on November 29, 2021: member

    GetAdjustedTime() (used e.g. in validation and addrman) returns a time with an offset that is influenced by timestamps that our peers have sent us in their version message.

    Currently, timestamps from all peers are used for this. However, I think that it would make sense to ignore the timedata samples from inbound peers, making it much harder for others to influence the Adjusted Time in a targeted way. With the extra feeler connections (every 2 minutes on average) and extra block-relay-only connections (every 5 minutes on average) there are also now plenty of opportunities to gather a meaningful number of timedata samples from outbound peers.

    There are some measures in place to prevent abuse: the -maxtimeadjustment parameter with a default of 70 minutes, warnings in cases of large deviations, only using the first 200 samples (explanation), but I think that only using samples from outbound connections in the first place would be an additional safety measure that would make sense.

    See also issue #4521 for further context and links: There have been several discussions in the past about replacing or abolishing the existing timedata system.

  2. p2p: Don't use timestamps from inbound peers 
    This makes it harder for others to tamper with
our adjusted time.
    0c85dc30e6
  3. sipa commented at 10:12 pm on November 29, 2021: member
    Concept ACK
  4. fanquake added the label P2P on Nov 29, 2021
  5. MarcoFalke commented at 1:39 pm on November 30, 2021: member
    Is this fixing #6071 ?
  6. mzumsande commented at 5:59 pm on November 30, 2021: member

    Is this fixing #6071 ?

    Yes I think so, with SPV clients being inbound peers.

  7. vasild commented at 4:03 pm on December 1, 2021: member

    Concept ACK

    I have been thinking “what could go wrong with this?” but can’t find a realistic adverse scenario. Need to stare a bit more at the surrounding code before full ACK.

    What about this - a node fails to create outgoing connections while incoming connections arrive just fine (could happen due to a firewall or some other network mishap (intentional or not) or addrman filled with junk). Many incoming connections arrive from honest nodes with correct times, which are ignored. How realistic is that? Is it worth considering an extension of this patch that starts collecting time samples from incoming connections if none or very few samples have been collected from outgoing connections for a long time after startup?

  8. mzumsande commented at 9:01 pm on December 1, 2021: member

    What about this - a node fails to create outgoing connections while incoming connections arrive just fine (could happen due to a firewall or some other network mishap (intentional or not) or addrman filled with junk). Many incoming connections arrive from honest nodes with correct times, which are ignored. How realistic is that?

    I’ve never heard of someone in this situation this before, maybe others have? It seems hard to get into this, after all outbound connections must have worked at some point in the past to broadcast our address to the network before others would connect to us. Plus, the lack of timedata samples could only cause potential problems if on top of all this, our system time is seriously off.

    Is it worth considering an extension of this patch that starts collecting time samples from incoming connections if none or very few samples have been collected from outgoing connections for a long time after startup?

    My first reaction was that this situation is so unlikely that it wouldn’t be worth the extra complexity, but I’d love to hear other opinions!

  9. darosior commented at 10:25 am on December 2, 2021: member
    tentative Concept ACK. The motivation sounds good to me although i’m not very familiar with this code.
  10. jnewbery commented at 2:08 pm on December 2, 2021: member
    Concept and code review ACK 0c85dc30e6b628f7538a67776c7eefcb84ef4f82
  11. in src/net_processing.cpp:2686 in 0c85dc30e6 
    2682@@ -2683,7 +2683,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2683 
2684         int64_t nTimeOffset = nTime - GetTime();
2685         pfrom.nTimeOffset = nTimeOffset;
2686-        AddTimeData(pfrom.addr, nTimeOffset);
2687+        if (!pfrom.IsInboundConn()) {
    vasild commented at 3:41 pm on December 2, 2021:

    nit, feel free to ignore: there is already if (!pfrom.IsInboundConn()) { just a few lines above and the nTimeOffset local variable seems unnecessary.

     0--- i/src/net_processing.cpp
 1+++ w/src/net_processing.cpp
 2@@ -2657,47 +2657,48 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 3         if (!pfrom.IsInboundConn()) {
 4             // For non-inbound connections, we update the addrman to record
 5             // connection success so that addrman will have an up-to-date
 6             // notion of which peers are online and available.
 7             //
 8             // While we strive to not leak information about block-relay-only
 9             // connections via the addrman, not moving an address to the tried
10             // table is also potentially detrimental because new-table entries
11             // are subject to eviction in the event of addrman collisions.  We
12             // mitigate the information-leak by never calling
13             // AddrMan::Connected() on block-relay-only peers; see
14             // FinalizeNode().
15             //
16             // This moves an address from New to Tried table in Addrman,
17             // resolves tried-table collisions, etc.
18             m_addrman.Good(pfrom.addr);
19+
20+            // Don't use timedata samples from inbound peers to make it
21+            // harder for others to tamper with our adjusted time.
22+            pfrom.nTimeOffset = nTime - GetTime();
23+            AddTimeData(pfrom.addr, pfrom.nTimeOffset);
24         }
25
26         std::string remoteAddr;
27         if (fLogIPs)
28             remoteAddr = ", peeraddr=" + pfrom.addr.ToString();
29
30         LogPrint(BCLog::NET, "receive version message: %s: version %d, blocks=%d, us=%s, txrelay=%d, peer=%d%s\n",
31                   cleanSubVer, pfrom.nVersion,
32                   peer->m_starting_height, addrMe.ToString(), fRelay, pfrom.GetId(),
33                   remoteAddr);
34
35-        int64_t nTimeOffset = nTime - GetTime();
36-        pfrom.nTimeOffset = nTimeOffset;
37-        AddTimeData(pfrom.addr, nTimeOffset);
38-
39         // If the peer is old enough to have the old alert system, send it the final alert.
    jnewbery commented at 4:21 pm on December 2, 2021:
    That suggested patch would leave pfrom.nTimeOffset unset for inbound peers, which in turn would mean that timeoffset is unset in the node stats.
    vasild commented at 8:13 am on December 7, 2021:
    Right!
  12. vasild approved
  13. vasild commented at 3:41 pm on December 2, 2021: member

    ACK 0c85dc30e6b628f7538a67776c7eefcb84ef4f82

    Is it worth considering an extension of this patch…

    Probably not.

  14. naumenkogs commented at 8:12 am on December 6, 2021: member

    I think the justification makes sense and the code is good. This is an improvement. ACK 0c85dc30e6b628f7538a67776c7eefcb84ef4f82

    What about this - a node fails to create outgoing connections while incoming connections arrive just fine (could happen due to a firewall or some other network mishap (intentional or not) or addrman filled with junk). Many incoming connections arrive from honest nodes with correct times, which are ignored. How realistic is that? Is it worth considering an extension of this patch that starts collecting time samples from incoming connections if none or very few samples have been collected from outgoing connections for a long time after startup?

    Yeah, it’s hard for me to imagine this, I can only see this happen if Addrman is attacked in a very severe way sometime during node uptime. And even in that case i don’t think this current approach can help much.

  15. fanquake merged this on Dec 7, 2021
  16. fanquake closed this on Dec 7, 2021
  17. sidhujag referenced this in commit 455720cb71 on Dec 7, 2021
  18. mzumsande deleted the branch on Dec 8, 2021
  19. RandyMcMillan referenced this in commit 7c967381a7 on Dec 23, 2021
  20. jonatack commented at 11:41 am on March 18, 2022: member

    Posthumous ACK 0c85dc30e6b628f7538a67776c7eefcb84ef4f82

    -maxtimeadjustment should be updated with this change; done in #24609.

  21. MarcoFalke referenced this in commit a7b3123fea on Mar 18, 2022
  22. sidhujag referenced this in commit cd8b49a9bf on Mar 18, 2022
  23. Fabcien referenced this in commit 0eda354e86 on Mar 17, 2023
  24. DrahtBot locked this on Mar 18, 2023


mzumsande sipa MarcoFalke vasild darosior jnewbery naumenkogs jonatack

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me