Skip to content

shadowsocks: Align SS2022 timestamp check with SIP022 ±30s window#148

Open
zytakeshi wants to merge 1 commit into
cfal:masterfrom
zytakeshi:fix/ss2022-clock-skew-sip022
Open

shadowsocks: Align SS2022 timestamp check with SIP022 ±30s window#148
zytakeshi wants to merge 1 commit into
cfal:masterfrom
zytakeshi:fix/ss2022-clock-skew-sip022

Conversation

@zytakeshi
Copy link
Copy Markdown

Problem

The AEAD-2022 (Shadowsocks 2022) request/response header timestamp validation in shadowsocks_stream.rs uses an asymmetric clock-skew window that does not match the official spec:

if current_time_secs >= timestamp_secs {
    if current_time_secs - timestamp_secs > 30 {        // 30s tolerance into the PAST
        return Err(... "timestamp is greater than 30 seconds");
    }
} else {
    // Make sure times aren't too far in the future.
    if timestamp_secs - current_time_secs > 2 {         // only 2s tolerance into the FUTURE
        return Err(... "timestamp is {} seconds in the future");
    }
}

A peer whose clock is behind is tolerated up to 30 seconds, but a peer whose clock is ahead is rejected after just 2 seconds.

Why this is wrong (the official standard)

SIP022 — Shadowsocks 2022 Edition specifies a single, symmetric tolerance, not a directional one:

The replay protection is based on the timestamp. The timestamp is the current Unix Epoch timestamp. The server should reject the connection if the time difference is greater than 30 seconds.

The "time difference" is an absolute value — 30 seconds in either direction. This also lines up with the implementation's own 60-second salt replay cache (TimedSaltChecker::new(60)), which is sized for a ±30s window (30s back + 30s forward = 60s total span). The 2-second future cap is inconsistent with both the spec and the replay cache it pairs with.

Real-world impact

The 2-second forward cap rejects perfectly valid peers whose clock simply runs a little fast:

  • Server side: a client with mild NTP drift, or on a virtualized/unsynced host, gets rejected with timestamp is N seconds in the future even though it is well inside the spec's 30s window.
  • Client side: if the server's clock is >2s ahead, the response header is rejected and the connection fails.

Clock drift of a few seconds is common on phones and VMs; 2 seconds is far too tight to be standard-compliant.

Fix

Replace both the request-header and response-header checks with a single symmetric abs_diff > 30 test, exactly matching SIP022:

// SIP022 (Shadowsocks 2022 Edition) requires rejecting any header
// whose timestamp is more than 30 seconds away from the current
// time, in either direction.
let time_diff_secs = current_time_secs.abs_diff(timestamp_secs);
if time_diff_secs > 30 {
    return Err(std::io::Error::other(format!(
        "timestamp is {time_diff_secs} seconds away from the current time"
    )));
}

This keeps the 30s past tolerance unchanged and widens the future tolerance from 2s to the standard 30s. abs_diff is stable and available on the crate's edition = "2024" toolchain.

Testing

  • cargo build --lib passes (only pre-existing unrelated warnings in socket_util.rs).

🤖 Generated with Claude Code

The AEAD-2022 header timestamp validation used an asymmetric window:
up to 30 seconds in the past, but only 2 seconds in the future. SIP022
specifies a single tolerance ("not more than 30 seconds away"), applied
in both directions, and the 60-second salt replay cache is sized for a
±30s window.

The 2-second future cap rejected peers whose clock ran only slightly
ahead (NTP drift, unsynced/virtualized hosts), failing the handshake
with "timestamp is N seconds in the future" even though the spec would
accept anything within 30 seconds. Replace both request- and
response-header checks with a symmetric abs_diff > 30 check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant