From 0b2cf533ee5771fdb16255652837af69c316642e Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Mon, 14 Mar 2022 14:33:11 -0400
Subject: [PATCH] tor-proto: better errors when handshake fails due to untimely
certs
We now check the handshake certificates unconditionally, and only
report them as _expired_ as a last resort.
(Rationale: if somebody is presenting the wrong identity from a year
ago, it is more interesting that they are presenting the wrong ID
than it is that they are doing so with an expired cert.
We also now report a different error if the certificate is expired,
but its expiration is within the range of reported clock skew.
(Rationale: it's helpful to distinguish this case, so that we can
blame the failure on possible clock skew rather than definitely
attributing it to a misbehaving relay.)
Part of #405.
---
crates/tor-error/src/lib.rs | 8 ++++
crates/tor-proto/src/channel/handshake.rs | 50 ++++++++++++++++++-----
crates/tor-proto/src/util/err.rs | 28 ++++++++++---
doc/semver_status.md | 5 +++
4 files changed, 76 insertions(+), 15 deletions(-)
diff --git a/crates/tor-error/src/lib.rs b/crates/tor-error/src/lib.rs
index 850eb96392..c1408a3421 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 111b660df7..b68cd54dc6 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 73bf56e562..991ef5d221 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 1f4f86d220..4ca94399bc 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.
--
GitLab