diff --git a/crates/tor-error/src/lib.rs b/crates/tor-error/src/lib.rs
index 850eb96392d920728691ed38ea696093fecf2069..c1408a342187ba54f99e5dd947d6b5aa0e338e72 100644
--- a/crates/tor-error/src/lib.rs
+++ b/crates/tor-error/src/lib.rs
@@ -525,6 +525,14 @@ pub enum ErrorKind {
#[display(fmt = "no exit available for path")]
NoExit,
+ /// An operation failed because of _possible_ clock skew.
+ ///
+ /// The broken clock may be ours, or it may belong to another party on the
+ /// network. It's also possible that somebody else is lying about the time,
+ /// caching documents for far too long, or something like that.
+ #[display(fmt = "possible clock skew detected")]
+ ClockSkew,
+
/// Internal error (bug) in Arti.
///
/// A supposedly impossible problem has arisen. This indicates a bug in
diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs
index 111b660df7cd16f627e99e2d3be139cd97a4fd88..b68cd54dc67b5b4c053b8a4dc1c07891555797c3 100644
--- a/crates/tor-proto/src/channel/handshake.rs
+++ b/crates/tor-proto/src/channel/handshake.rs
@@ -302,10 +302,39 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> UnverifiedChannel<T> {
self,
peer: &U,
peer_cert_sha256: &[u8],
- now: Option<std::time::SystemTime>,
+ now: Option<SystemTime>,
) -> Result<VerifiedChannel<T>> {
use tor_cert::CertType;
use tor_checkable::*;
+
+ /// Helper: given a time-bound input, give a result reflecting its
+ /// validity at `now`, and the inner object.
+ ///
+ /// We use this here because we want to validate the whole handshake
+ /// regardless of whether the certs are expired, so we can determine
+ /// whether we got a plausible handshake with a skewed partner, or
+ /// whether the handshake is definitely bad.
+ fn check_timeliness<C, T>(checkable: C, now: SystemTime, skew: ClockSkew) -> (Result<()>, T)
+ where
+ C: Timebound<T, Error = TimeValidityError>,
+ {
+ let status = checkable.is_valid_at(&now).map_err(|e| match (e, skew) {
+ (TimeValidityError::Expired(expired_by), ClockSkew::Fast(skew))
+ if expired_by < skew =>
+ {
+ Error::HandshakeCertsExpired { expired_by, skew }
+ }
+ // As it so happens, we don't need to check for this case, since the certs in use
+ // here only have an expiration time in them.
+ // (TimeValidityError::NotYetValid(_), ClockSkew::Slow(_)) => todo!(),
+ (_, _) => Error::HandshakeProto("Certificate expired or not yet valid".into()),
+ });
+ let cert = checkable.dangerously_assume_timely();
+ (status, cert)
+ }
+ // Replace 'now' with the real time to use.
+ let now = now.unwrap_or_else(SystemTime::now);
+
// We need to check the following lines of authentication:
//
// First, to bind the ed identity to the channel.
@@ -342,9 +371,7 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> UnverifiedChannel<T> {
// Check the identity->signing cert
let (id_sk, id_sk_sig) = id_sk.check_key(&None)?.dangerously_split()?;
sigs.push(&id_sk_sig);
- let id_sk = id_sk
- .check_valid_at_opt(now)
- .map_err(|_| Error::HandshakeProto("Certificate expired or not yet valid".into()))?;
+ let (id_sk_timeliness, id_sk) = check_timeliness(id_sk, now, self.clock_skew);
// Take the identity key from the identity->signing cert
let identity_key = id_sk.signing_key().ok_or_else(|| {
@@ -362,9 +389,7 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> UnverifiedChannel<T> {
.check_key(&Some(*signing_key))? // TODO(nickm): this is a bad interface
.dangerously_split()?;
sigs.push(&sk_tls_sig);
- let sk_tls = sk_tls
- .check_valid_at_opt(now)
- .map_err(|_| Error::HandshakeProto("Certificate expired or not yet valid".into()))?;
+ let (sk_tls_timeliness, sk_tls) = check_timeliness(sk_tls, now, self.clock_skew);
if peer_cert_sha256 != sk_tls.subject_key().as_bytes() {
return Err(Error::HandshakeProto(
@@ -407,9 +432,8 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> UnverifiedChannel<T> {
.ok_or_else(|| Error::HandshakeProto("No RSA->Ed crosscert".into()))?;
let rsa_cert = tor_cert::rsa::RsaCrosscert::decode(rsa_cert)?
.check_signature(&pkrsa)
- .map_err(|_| Error::HandshakeProto("Bad RSA->Ed crosscert signature".into()))?
- .check_valid_at_opt(now)
- .map_err(|_| Error::HandshakeProto("RSA->Ed crosscert expired or invalid".into()))?;
+ .map_err(|_| Error::HandshakeProto("Bad RSA->Ed crosscert signature".into()))?;
+ let (rsa_cert_timeliness, rsa_cert) = check_timeliness(rsa_cert, now, self.clock_skew);
if !rsa_cert.subject_key_matches(identity_key) {
return Err(Error::HandshakeProto(
@@ -444,6 +468,12 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> UnverifiedChannel<T> {
return Err(Error::HandshakeProto("Peer RSA id not as expected".into()));
}
+ // We note expired certs last, since any other reason we might reject a
+ // handshake is probably more serious.
+ id_sk_timeliness?;
+ sk_tls_timeliness?;
+ rsa_cert_timeliness?;
+
Ok(VerifiedChannel {
link_protocol: self.link_protocol,
tls: self.tls,
diff --git a/crates/tor-proto/src/util/err.rs b/crates/tor-proto/src/util/err.rs
index 73bf56e562222ff5a06692a243ab7da8d5e1fc67..991ef5d221a28b7b7fd43857c19aa26ea3943ec8 100644
--- a/crates/tor-proto/src/util/err.rs
+++ b/crates/tor-proto/src/util/err.rs
@@ -1,5 +1,5 @@
//! Define an error type for the tor-proto crate.
-use std::sync::Arc;
+use std::{sync::Arc, time::Duration};
use thiserror::Error;
use tor_cell::relaycell::msg::EndReason;
use tor_error::{ErrorKind, HasKind};
@@ -42,6 +42,17 @@ pub enum Error {
/// Handshake protocol violation.
#[error("handshake protocol violation: {0}")]
HandshakeProto(String),
+ /// Handshake broken, maybe due to clock skew.
+ ///
+ /// (If the problem can't be due to clock skew, we return HandshakeProto
+ /// instead.)
+ #[error("handshake failed due to expired certificates (possible clock skew)")]
+ HandshakeCertsExpired {
+ /// For how long has the circuit been expired?
+ expired_by: Duration,
+ /// How fast does the relay claim that our clock is?
+ skew: Duration,
+ },
/// Protocol violation at the channel level, other than at the handshake
/// stage.
#[error("channel protocol violation: {0}")]
@@ -113,10 +124,16 @@ impl From<Error> for std::io::Error {
ChannelClosed | CircuitClosed => ErrorKind::ConnectionReset,
- BytesErr(_) | BadCellAuth | BadCircHandshake | HandshakeProto(_) | ChanProto(_)
- | CircProto(_) | CellErr(_) | ChanMismatch(_) | StreamProto(_) => {
- ErrorKind::InvalidData
- }
+ BytesErr(_)
+ | BadCellAuth
+ | BadCircHandshake
+ | HandshakeProto(_)
+ | ChanProto(_)
+ | HandshakeCertsExpired { .. }
+ | CircProto(_)
+ | CellErr(_)
+ | ChanMismatch(_)
+ | StreamProto(_) => ErrorKind::InvalidData,
Bug(ref e) if e.kind() == tor_error::ErrorKind::BadApiUsage => ErrorKind::InvalidData,
@@ -142,6 +159,7 @@ impl HasKind for Error {
E::BadCellAuth => EK::TorProtocolViolation,
E::BadCircHandshake => EK::TorProtocolViolation,
E::HandshakeProto(_) => EK::TorAccessFailed,
+ E::HandshakeCertsExpired { .. } => EK::ClockSkew,
E::ChanProto(_) => EK::TorProtocolViolation,
E::CircProto(_) => EK::TorProtocolViolation,
E::ChannelClosed | E::CircuitClosed => EK::CircuitCollapse,
diff --git a/doc/semver_status.md b/doc/semver_status.md
index 1f4f86d220732999968e900a7c867dfb098c4690..4ca94399bc13d3cadcdf305708850f36a5255725 100644
--- a/doc/semver_status.md
+++ b/doc/semver_status.md
@@ -61,6 +61,11 @@ tor-protover:
tor-proto:
api-break: OutboundClientHandshake::connect() now takes now_fn.
+ new-api: New Error::HandshakeCertsExpired.
+
+tor-error:
+ new-api: New ErrorKind::ClockSkew.
+
tor-cell:
new-api: Netinfo message now has a timestamp() accessor.