P2P: add maxtimeadjustment command line option

mruddy commented at 3:48 pm on February 21, 2016: contributor

Add trustsystemclock command line option to adjust whether peer input can affect the local perspective on time.

I know that there are multiple issues open dealing with local node time management (e.g.- #4521). I didn’t see a pull request though. So, here’s one.

This is the same patch set that gained acceptance as being useful for https://github.com/bitcoinxt/bitcoinxt/pull/35/files.

It’s a really simple change that gives the user the option of letting other nodes influence it’s perspective on time vs. not (a new default).

People have been using this for months without a problem that I’m aware of. At least it has actual usage behind it. If you think making the default “false”, to make the current functionality the default, is the way to go to get this included, then I could make that change to this PR.

At least this will give users an easy option to choose how they want to handle it. On the command line, just use either -trustsystemclock=0 or -trustsystemclock=1 (the default in this initial proposed commit).

paveljanik commented at 4:01 pm on February 21, 2016: contributor

Concept ACK. This is the “one” way of “Never go to sea with two chronometers; take one or three.”.

Yes, please change the default…

No need to call AddTimeData() at all if we trust system clock. Or you can collect time samples and warn user when his system clock is wrong “a lot”…

wangchun commented at 4:46 pm on February 21, 2016: none

Concept ACK. But the default value should be set to false.

gmaxwell commented at 9:16 pm on February 21, 2016: contributor

Concept NAK; especially with the default value.

As is, there is almost never any active monitoring it’s quite common for systems even with network time configured to have totally incorrect time because the network time has simply silently stopped working. At RWC some persons from the google chrome team presented data collected from chrome users that showed very high rates of time inaccuracy on hosts (and also showed that a significant fraction of all SSL certificate errors are from clients with the wrong date).

IIRC Gnutella has the ability to query the local NTP daemon to determine if it believes that its in sync. It might be more reasonable to have an option to “use local time if and only if the local time server believes that it is in sync”.

Of course all this ignores that the normal configuration of NTP is completely insecure, easily spoofed, will believe completely implausible information (e.g. that time is a decade before the release of the software), no effort made against sybil resistance, and there are already known incidents of malicious ntp servers in the public NTP pools… but as a default off option, perhaps that is less of a concern.

mruddy commented at 10:17 pm on February 21, 2016: contributor

@paveljanik @wangchun OK, I changed the default to false. I pretty much expected that to be the first thing mentioned, thanks for confirming :) I only left it that way to keep merging easier for anyone that was already using it downstream. @gmaxwell Thanks for your perspective. That point about clocks causing cert validation errors is interesting. I would not have guessed that. With the default changed to false now, does that make this more acceptable to you? The latest commit basically leaves people in the same boat as they are now, but it gives knowledgeable users that choose to override the ability to use only their local clock.

jonasschnelli added the label P2P on Feb 22, 2016

mruddy force-pushed on Feb 22, 2016

paveljanik commented at 7:21 pm on February 23, 2016: contributor

@mruddy Can you lease add warning to the user when his clock is too different to the network time?

mruddy commented at 10:30 pm on February 23, 2016: contributor

@paveljanik The existing warning will still trigger for that scenario. Please see here https://github.com/mruddy/bitcoin/blob/a1a4d8a5de73e018485ec8fe67d3918b625ed0d0/src/timedata.cpp#L110. This is because the P2P version message still calls AddTimeData. I didn’t change any of that. This change leaves all that old time computation stuff, but ignores the offset that is calculated from it if the new command line switch is set. This is a super simple commit.

gmaxwell commented at 10:35 pm on February 23, 2016: contributor

@mruddy Have you looked into making it conditional on the local NTP thinking its in sync?

mruddy commented at 10:58 pm on February 23, 2016: contributor

@gmaxwell No, but not because I ignored the input.

The first reason is that I don’t trust myself to not mess it up and add some kind of vector where a malformed response would allow node takeover etc… The extra attack surface makes me apprehensive.

Second, when I think of how this would be used, the first use-case that comes to mind is of a node operator that notices some other node(s) trying to jack up its perspective on time and (s)he wants to stop that. I envision that admin to be actively monitoring that his/her system time is within reason. The second use case is that of an operator that simply wants to trust the local clock as a standard operating procedure. Again, I envision that user as having made a conscious effort to commit to maintaining the node time through normal process and procedure.

Lastly, in the case where NTP has been spoofed/tampered, or is just not reliable (silent failure etc…), as you mentioned before, then the extra complexity does not seem to help by my way of thinking.

gmaxwell commented at 11:44 pm on February 23, 2016: contributor

Indeed, detection would not help when someone is exploiting NTP’s normal lack of security to change the system time; I didn’t intend to suggest otherwise. Sorry.

I don’t agree with your attack surface concern, after all– it’s all local– portability may turn out to be more of an issue. But code to harden up this option to throw an error or refuse to operate when set to trust the local clock but the local clock is detectably undisciplined could be done as a separate PR.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

mruddy commented at 0:43 am on February 24, 2016: contributor

Yep, you’re absolutely right that portability might be the devil in the details for the NTP checking. Even being all local, it’s still conditional logic that could be triggered under a remote attacker’s influence. So, at that point it depends on your threat model whether an attacker being able to influence that exchange is in scope. If that’s in scope, then possibly a concern that doesn’t depend on a buffer overflow etc… would be where that could be used as a trigger for a previously installed trojan that listens where the NTP listener would have been and then does something bad. That’s getting a little far fetched, but it’s what came to mind.

I’ll have to double check on the performance question and get back to you. I know when I first worked on this about six months ago or so, I did wonder that. We went with it, but I forget if that was just because it was the typical pattern and the code was simple, or if it just did not matter performance-wise.

off topic: Speaking of performance, I went to prune a current datadir today that had txindex on. Of course that caused me to have to reindex. I ran 0.12 to do that and was done soooo much faster thanks to sipa and your excellent work on libsecp256k1. Thank-you!

laanwj commented at 7:48 am on February 24, 2016: member

No problem with adding this, although I don’t think it will be used a lot in practice. It should definitely not be enabled by default.

But I understand the use case where you have an expensive trusted time source, and you don’t want bitcoind messing with it, even if it means well.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

Yes - the preferred way to do this would be to set a global flag in AppInit2, then use that in the time function. Calling the argument parser every time someone asks for the time is not very elegant, at least.

Or possibly even better: have the option disable the time-offset logic, set the time offset to 0? That’d remove even need to change the GetAdjustedTime() function itself.

Nits:

Add a DEFAULT_TRUST_SYSTEM_TIME constant as is done for other options instead of hardcode the default in the help message and GetBoolArg line.

paveljanik commented at 8:24 am on February 24, 2016: contributor

Idea. This can even be extended. If I trust my time source, I can ban nodes with a huge time difference from my exact time to prevent network partition attacks on me (bad nodes trying to attack me on my node startup with a huge time difference bringing me off the main network). Not that I have seen such attack myself yet, but…

in src/init.cpp: in a1a4d8a5de outdated

348@@ -349,6 +349,7 @@ std::string HelpMessage(HelpMessageMode mode)
349 #ifndef WIN32
350     strUsage += HelpMessageOpt("-sysperms", _("Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)"));
351 #endif
352+    strUsage += HelpMessageOpt("-trustsystemclock", _("Trust, and depend solely on, the local system clock. Do not use peer time offset adjustments. (default: 0)"));

MarcoFalke commented at 2:17 pm on February 24, 2016:

Please factor out the default from the translation string. (see @laanwj’s comment.)

mruddy commented at 3:35 pm on February 25, 2016: contributor

Thanks, I just pushed the updates mentioned by your feedback. @laanwj I chose not to disable the time-offset logic entirely because I’ve gotten feedback about people liking to get the warning message “Please check that your computer’s date and time are correct!” if none of the peers are within 5 minutes just in-case. @paveljanik I also did not add any logic to ban nodes with largely differing times because I did not want to add side-effects that could increase P2P network partitioning. Right now, if the median offset is more than 70 minutes, the offset is set to zero and a warning is logged if none of the nodes are within five minutes. So, there is some protection against an attack that attempts to change your node’s perspective on time a lot. If your node disagrees substantially with every other node, then you get a warning. But you’ll continue to exchange transactions and blocks etc… If banning them turns out to be desirable, then a separate PR might be the way to go.

paveljanik commented at 3:42 pm on February 25, 2016: contributor

Please squash into one commit for easier review.

mruddy force-pushed on Feb 25, 2016

mruddy commented at 6:52 pm on February 25, 2016: contributor

Sure thing, done.

laanwj commented at 8:57 am on February 29, 2016: member

utACK 1079b2c This code change is nicely self-contained and localized. Let’s not make any changes to the P2P code in this pull, also any proposal regarding automatic banning (for any reason) has to be very carefully evaluated for advantages and dangers, for the reasons already mentioned - partitioning risk.

laanwj added the label Feature on Feb 29, 2016

sipa commented at 5:46 am on March 5, 2016: member

@gmaxwell I am not convinced that the behaviour of not correcting at all is less harmful than correcting your clock based on what random peers tell you when chosen by someone who has confidence in their own clock configuration.

mruddy commented at 11:52 am on March 5, 2016: contributor

Perhaps don’t think of it as someone “having confidence in their own clock configuration”, but rather “wanting to have a consistent or specific perspective on time”. For example, maybe a company has nodes in more than one geo location and wants to have a closely consistent perspective on time across all of its nodes. Or, maybe that company wants to have a cluster of nodes running on internally controlled time and a cluster running on network adaptive time. Perhaps there are attacker nodes that feed 69 minute time skews to peers based on user agent and those affected user agents don’t want to have their ability to follow consensus affected. From a decentralized validation point of view, I don’t see why it would be necessarily bad for a node to take a firm position, or have confidence in its own configuration, on what time it thought it was.

laanwj commented at 7:24 am on March 11, 2016: member

I’ve thought it over a bit and I still think GetTimeOffset should return 0 when -trustsystemclock is given. This will make the RPCs (getinfo, getnetworkinfo) give the correct effective time offset.

mruddy force-pushed on Mar 13, 2016

mruddy commented at 11:12 pm on March 13, 2016: contributor

Sure, that makes sense too. Update made and rebased/squashed.

mruddy force-pushed on Mar 24, 2016

mruddy force-pushed on Mar 29, 2016

mruddy commented at 1:53 pm on March 29, 2016: contributor

This seemed to be lingering, so, here’s a commit with a little different approach. Now, instead of adding -trustsystemclock (that effectively limited the max offset to zero), I added -maxtimeadjustment, that lets the user choose how much of an adjustment (in seconds) is acceptable. Thus, -trustsystemclock and -maxtimeadjustment=0 give the same basic effect of not letting peers influence the local node’s perspective of time. But, this new way gives a superset of possible choices with less code and probably less runtime overhead because it does not add a condition to GetTimeOffset. @laanwj @sipa Do you find this any more acceptable? If not, I’ll close this and forget about it. No big deal to me either way. Thanks!

laanwj commented at 2:19 pm on March 29, 2016: member

utACK, I like this better, you’ve un-magified a magic number (and you can still achieve the same as before, by setting it to 0).

P2P: add maxtimeadjustment command line option e1523cee58

in src/util.h: in d84966c337 outdated

30@@ -31,6 +31,7 @@
31 static const bool DEFAULT_LOGTIMEMICROS = false;
32 static const bool DEFAULT_LOGIPS        = false;
33 static const bool DEFAULT_LOGTIMESTAMPS = true;
34+static const int64_t DEFAULT_MAX_TIME_ADJUSTMENT = 70 * 60;

laanwj commented at 2:20 pm on March 29, 2016:

This should be in timedata.h (as that’s where the option is used)

mruddy commented at 2:36 pm on March 29, 2016:

Yep, sure thing, I’ll update that. I had thought of that and just figured I’d leave it with some other defaults since then I didn’t have to add the include in init.cpp.

MarcoFalke commented at 2:41 pm on March 29, 2016:

Also, the github pull subject line needs update. (It’s basically a different pull)

mruddy commented at 2:48 pm on March 29, 2016:

@MarcoFalke done, thanks

laanwj commented at 3:13 pm on March 29, 2016:

then I didn’t have to add the include in init.cpp

Yes - that would be slightly better, but everything considered I think it’s better to be explicit about dependencies. It’s just a symptom of the current centralization of option handling in init.cpp.

mruddy commented at 3:28 pm on March 29, 2016:

makes sense. update is made and passed travis. should be good to go once some others ack.

mruddy force-pushed on Mar 29, 2016

mruddy renamed this:
~~P2P: add trustsystemclock command line option~~
P2P: add maxtimeadjustment command line option
on Mar 29, 2016

gmaxwell commented at 3:58 pm on March 29, 2016: contributor

I like this approach much better. But the implementation has an odd non-monotonicity. E.g. if you have 5 peers claiming ten seconds off, then six more connect claiming 80 minutes. You’ll apply a correction of 0 instead of 10 seconds. Perhaps better to just fix in another PR, since it was an existing misbehavior (though more likely to be triggered when the time is cut down to small numbers).

mruddy commented at 5:33 pm on March 29, 2016: contributor

@gmaxwell Interesting. True, and in that scenario a warning message also gets output when the median goes beyond the acceptable limit.

Maybe you were working towards the, perhaps more alarming, scenario where there are five nodes at 10 seconds off and then six more claim the well-known default acceptable limit of 4200 seconds. That would cause a large effective change in offset with no other warning. Maybe that could be the basis for another warning message (in a separate PR)? Basically, the offset is within tolerance, but there might be something going on. We’d need to add an option to set the threshold of change to alert that there might be “something going on”. This is different than simply seeing a large range between min and max offset seen because it actually changes our local offset.

Now that this value no longer a hard-coded constant, a possible mitigation would be to unpredictably pick a value between 0 and 4200 so that attacker nodes would run a higher risk of sending back offsets that are out of bounds.

Also, in your scenario, I’m not sure which set of nodes are attacker nodes (or maybe they’re two sets of nodes working at cross-purposes).

For example, this is what’s really on my mind:

I’ve been working on a CLTV-based unidirectional payment channel vulnerability assessment and two actual related threats that come to mind from that are:

miners that want to get fees from time locked transactions by putting them in blocks “early” (will be mitigated by BIP113)
senders that want to claim the channel’s reserve value by getting a refund transaction mined before the receiver can spend a transaction allocating value to her

Both threats gain advantage by having more nodes accept into their mempool and relay time-locked transactions early. Even with BIP113, if a sender can get a non-opt-in-RBF sequence value containing transaction propagated early, that inhibits the receiver’s ability to even propagate an honest allocation transaction later.

So maybe the bad actors set up a bunch of passive nodes that, when connected to, return valid high time offsets.

laanwj commented at 7:28 am on March 30, 2016: member

Agreed, let’s fix that to another pull.

There is no obligation for @mruddy to fix the peer time synchronization system, which is a herculean task, especially as it seems he doesn’t want to use it at all. See #4521 for a collection of peer time related issues, will link there to this one too.

laanwj merged this on Mar 30, 2016

laanwj closed this on Mar 30, 2016

laanwj referenced this in commit 352fd57729 on Mar 30, 2016

mruddy deleted the branch on Mar 30, 2016

codablock referenced this in commit 4d9ebfa728 on Sep 16, 2017

codablock referenced this in commit b33d2bd661 on Sep 19, 2017

codablock referenced this in commit dbd6532874 on Dec 9, 2017

codablock referenced this in commit 1c84417780 on Dec 19, 2017

MarcoFalke locked this on Sep 8, 2021

P2P: add maxtimeadjustment command line option #7573