Eliminate built-in SNTP support (fixes #4207): #4628
Conversation
|
How does this compare with HowardHinnant@6c239bd ? |
I feel that it is more thorough, restructuring and simplifying the code, instead of simply eliminating the SNTP code. |
src/ripple/core/TimeKeeper.h
Outdated
|
|
||
| return (offset * 3) / 4; | ||
| }()); | ||
| assert(success); |
There was a problem hiding this comment.
I don't really object, but doesn't asserting here mean you can unconditionally assign? If the assert fails, that means another thread changed the value between the time we read it and changed it. If we're asserting that never fails do we need the compare and exchange strong? Do we need the atomic?
Honestly, I think I'd keep the atomics - it makes the code thread safe, but I might remove the assert. What breaks if that assert fires?
There was a problem hiding this comment.
You're right; we could just unconditionally assign and nothing really goes wrong. My intention here was to implement a "weak" form of the precondition that calling this function is supposed to be externally serialized, but it's extremely weak and subject to timing, instead of being a robust check.
I can just change this to be:
closeOffset_ = [by, offset]() {
// Ignore small offsets and push the close time
// towards our wall time.
if (by > 1s)
return offset + ((by + 3s) / 4);
if (by < -1s)
return offset + ((by - 3s) / 4);
return (offset * 3) / 4;
}();
|
@intelliot, I've addressed @seelabs's comment and rebased the code against the latest develop branch. I will let Scott do a final pass and give a thumbs up, but I think this is ready to go. |
Awaiting final pass by @seelabs . Also, there are (new) conflicts that must be resolved. |
| adjust(std::chrono::system_clock::time_point when) | ||
| { | ||
| return time_point(std::chrono::duration_cast<duration>( | ||
| when.time_since_epoch() - days(10957))); |
There was a problem hiding this comment.
Just a nit: we usually like to separate the implementation from the interface so it's easier to see interfaces. I'm OK merging this as-is though. It's only a nit and we can clean it up latter (although I did want to flag it; for future changes we should try to stick with separate interfaces/implementations).
nbougalis
added
the
Passed
The codebase includes a simple SNTP client that is enabled by default and used to adjust an application-level clock in case the operating system's clock is not already synchronized. A recent audit of the code conducted by Bishop-Fox identified a potential attack vector, which could allow an attacker with the capability to manipulate the UDP traffic flow of a server by redirecting queries or by injecting crafted SNTP responses to manipulate the correction factor, skewing the clock. Although the impact of such an attack, if mounted, is minimal and can be completely eliminated by editing the configuration file to disable the built-in SNTP client, this commit instead entirely eliminates the "crufty" 10+ year-old code, removes a mutex and greatly simplifies the `TimeKeeper`. **Note to server operators:** It is highly recommended to use time synchronization software to ensure that the system clock is accurate. For more details see https://en.wikipedia.org/wiki/Ntpd#Implementations.
|
The code contained a simple SNTP client that could be used to synchronize the clock that the code uses even when the system clock was unsynchronized.
Not only was the SNTP code "crufty" but during a recent audit of the codebase by Bishop-Fox, a potential attack vector that could allow an attacker with the ability to shape or redirect traffic to skew the server's clock was discovered.
This commit removes the SNTP client code, removes a mutex and generally simplifies the
TimeKeeper.Note to server operators:
It is highly recommended to use time synchronization software to ensure that system clocks are accurate.
Type of Change