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

@@ -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

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,

crates/tor-proto/src/util/err.rs

 //! 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,

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.