From 13ee324d36726fe3cd3dc69fe11f223491a9d34f Mon Sep 17 00:00:00 2001
From: zzz <zzz@i2pmail.org>
Date: Wed, 26 Jan 2022 07:28:43 -0500
Subject: [PATCH] NTCP2: Clock skew handling improvements
as discussed in #ls2 meeting
- Bob replies with Session Created even if skewed,
so that Alice finds out what the skew is
- Alice handles Session Created timestamp and drops if skewed,
bans Bob, and updates clock if NTP failed
- If Alice does reply with SessionConfirmed, Bob will send a
destroy with a skew error code
- Don't change skew error code if netdb store failed
- Fix skew adjustment for RTT by Bob
- Call setLastBadSkew() in the right places
- Fix ntcp.invalidInboundSkew and ntcp.invalidOutboundSkew stats
---
.../transport/ntcp/InboundEstablishState.java | 44 ++++++++++---------
.../transport/ntcp/OutboundNTCP2State.java | 19 +++++++-
2 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/router/java/src/net/i2p/router/transport/ntcp/InboundEstablishState.java b/router/java/src/net/i2p/router/transport/ntcp/InboundEstablishState.java
index 0f45d47cf1..dd0e9450a0 100644
--- a/router/java/src/net/i2p/router/transport/ntcp/InboundEstablishState.java
+++ b/router/java/src/net/i2p/router/transport/ntcp/InboundEstablishState.java
@@ -199,6 +199,10 @@ class InboundEstablishState extends EstablishBase implements NTCP2Payload.Payloa
if (_log.shouldLog(Log.DEBUG))
_log.debug(prefix() + "verification successful for " + _con);
+ // Adjust skew calculation now that we know RTT
+ // rtt from receiving #1 to receiving #3
+ long rtt = _context.clock().now() - _con.getCreated();
+ _peerSkew -= ((rtt / 2) + 500) / 1000;
long diff = 1000*Math.abs(_peerSkew);
if (!_context.clock().getUpdatedSuccessfully()) {
// Adjust the clock one time in desperation
@@ -209,16 +213,12 @@ class InboundEstablishState extends EstablishBase implements NTCP2Payload.Payloa
if (diff != 0)
_log.logAlways(Log.WARN, "NTP failure, NTCP adjusting clock by " + DataHelper.formatDuration(diff));
} else if (diff >= Router.CLOCK_FUDGE_FACTOR) {
- _context.statManager().addRateData("ntcp.invalidInboundSkew", diff);
- _transport.markReachable(aliceHash, true);
// Only banlist if we know what time it is
_context.banlist().banlistRouter(DataHelper.formatDuration(diff),
aliceHash,
_x("Excessive clock skew: {0}"));
_transport.setLastBadSkew(_peerSkew);
- if (getVersion() < 2)
- fail("Clocks too skewed (" + diff + " ms)", null, true);
- else if (_log.shouldWarn())
+ if (_log.shouldWarn())
_log.warn("Clocks too skewed (" + diff + " ms)");
_msg3p2FailReason = NTCPConnection.REASON_SKEW;
return false;
@@ -361,18 +361,21 @@ class InboundEstablishState extends EstablishBase implements NTCP2Payload.Payloa
_msg3p2len = (int) DataHelper.fromLong(options, 4, 2);
long tsA = DataHelper.fromLong(options, 8, 4);
long now = _context.clock().now();
- // In NTCP1, timestamp comes in msg 3 so we know the RTT.
- // In NTCP2, it comes in msg 1, so just guess.
- // We could defer this to msg 3 to calculate the RTT?
- long rtt = 250;
- _peerSkew = (now - (tsA * 1000) - (rtt / 2) + 500) / 1000;
- if ((_peerSkew > MAX_SKEW || _peerSkew < 0 - MAX_SKEW) &&
- !_context.clock().getUpdatedSuccessfully()) {
- // If not updated successfully, allow it.
- // This isn't very likely, outbound will do it first
- // See verifyInbound() above.
- fail("Clock Skew: " + _peerSkew, null, true);
- return;
+ // Will be adjusted for RTT in verifyInbound()
+ _peerSkew = (now - (tsA * 1000) + 500) / 1000;
+ if (_peerSkew > MAX_SKEW || _peerSkew < 0 - MAX_SKEW) {
+ long diff = 1000*Math.abs(_peerSkew);
+ _context.statManager().addRateData("ntcp.invalidInboundSkew", diff);
+ _transport.setLastBadSkew(_peerSkew);
+ if (_log.shouldWarn())
+ _log.warn("Clock Skew: " + _peerSkew + " on " + this);
+ // Do them a favor, keep going with msg 2 so they get our timestamp.
+ // They should disconnect after that.
+ // If they do send a msg 3, verifyInbound() will catch it and
+ // will send a destroy with code 7 and
+ // ban the peer for a while.
+ //fail("Clock Skew: " + _peerSkew, null, true);
+ //return;
}
if (_msg3p2len < MSG3P2_MIN || _msg3p2len > MSG3P2_MAX) {
fail("bad msg3p2 len: " + _msg3p2len);
@@ -671,9 +674,10 @@ class InboundEstablishState extends EstablishBase implements NTCP2Payload.Payloa
ok = verifyInboundNetworkID(ri);
if (!ok)
throw new DataFormatException("NTCP2 network ID mismatch");
- // hash collision?
- // expired RI?
- _msg3p2FailReason = NTCPConnection.REASON_MSG3;
+ // generally expired/future RI
+ // don't change reason if already set as clock skew
+ if (_msg3p2FailReason <= 0)
+ _msg3p2FailReason = NTCPConnection.REASON_MSG3;
throw new DataFormatException("RI store fail: " + ri, iae);
}
_con.setRemotePeer(_aliceIdent);
diff --git a/router/java/src/net/i2p/router/transport/ntcp/OutboundNTCP2State.java b/router/java/src/net/i2p/router/transport/ntcp/OutboundNTCP2State.java
index 4e53146c11..c576fa2a32 100644
--- a/router/java/src/net/i2p/router/transport/ntcp/OutboundNTCP2State.java
+++ b/router/java/src/net/i2p/router/transport/ntcp/OutboundNTCP2State.java
@@ -300,8 +300,23 @@ class OutboundNTCP2State implements EstablishState {
long rtt = now - _con.getCreated();
_peerSkew = (now - (tsB * 1000) - (rtt / 2) + 500) / 1000;
if (_peerSkew > MAX_SKEW || _peerSkew < 0 - MAX_SKEW) {
- fail("Clock Skew: " + _peerSkew, null, true);
- return;
+ long diff = 1000 * Math.abs(_peerSkew);
+ if (_context.clock().getUpdatedSuccessfully()) {
+ // usual case
+ _context.statManager().addRateData("ntcp.invalidOutboundSkew", diff);
+ fail("Clock Skew: " + _peerSkew, null, true);
+ // Only banlist if we know what time it is
+ _context.banlist().banlistRouter(DataHelper.formatDuration(diff),
+ _con.getRemotePeer().calculateHash(),
+ "Excessive clock skew: {0}"); // _x in IES
+ _transport.setLastBadSkew(_peerSkew);
+ return;
+ }
+ // Adjust the clock one time in desperation
+ // We are Alice, he is Bob, adjust to match Bob
+ _context.clock().setOffset(1000 * (0 - _peerSkew), true);
+ _peerSkew = 0;
+ _log.logAlways(Log.WARN, "NTP failure, NTCP adjusting clock by " + DataHelper.formatDuration(diff));
}
changeState(State.OB_GOT_HXY);
_received = 0;
--
GitLab