diff --git a/compose.yaml b/compose.yaml index e0d448ed..cbae3128 100644 --- a/compose.yaml +++ b/compose.yaml @@ -5,7 +5,7 @@ services: redis: - image: bitnami/redis:7.2 + image: bitnamilegacy/redis:7.2 environment: - ALLOW_EMPTY_PASSWORD=yes ports: diff --git a/crates/sshx-core/src/lib.rs b/crates/sshx-core/src/lib.rs index 5ffbba5c..5762bca7 100644 --- a/crates/sshx-core/src/lib.rs +++ b/crates/sshx-core/src/lib.rs @@ -8,9 +8,13 @@ use std::sync::atomic::{AtomicU32, Ordering}; use serde::{Deserialize, Serialize}; +pub mod name; + +pub use name::{InvalidName, SafeName}; + /// Protocol buffer and gRPC definitions, automatically generated by Tonic. #[allow(missing_docs, non_snake_case)] -#[allow(clippy::derive_partial_eq_without_eq)] +#[allow(clippy::derive_partial_eq_without_eq, clippy::result_large_err)] pub mod proto { tonic::include_proto!("sshx"); diff --git a/crates/sshx-core/src/name.rs b/crates/sshx-core/src/name.rs new file mode 100644 index 00000000..ebacad17 --- /dev/null +++ b/crates/sshx-core/src/name.rs @@ -0,0 +1,190 @@ +//! Validated identifiers that are safe to embed in a URL path. + +use std::fmt; +use std::ops::Deref; +use std::str::FromStr; + +use serde::{de::Visitor, Deserialize, Deserializer, Serialize}; + +/// A name that is safe to interpolate into a URL path without escaping. +/// +/// Session names are generated with [`rand_alphanumeric`], so this is a +/// whitelist rather than a blacklist: it rejects anything that could change +/// the meaning of a URL path, such as `/`, `.`, or `%`. Interpolating a +/// `SafeName` into a path is therefore always correct, so callers never need +/// to escape it. +/// +/// [`rand_alphanumeric`]: crate::rand_alphanumeric +#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub struct SafeName(String); + +impl SafeName { + /// Maximum accepted length, in bytes. + /// + /// This is far above the length of any generated name, and exists only to + /// bound the work done on attacker-controlled input. + pub const MAX_LEN: usize = 64; + + /// Returns the name as a string slice. + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl fmt::Display for SafeName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +impl Deref for SafeName { + type Target = str; + + fn deref(&self) -> &str { + &self.0 + } +} + +impl FromStr for SafeName { + type Err = InvalidName; + + fn from_str(s: &str) -> Result { + if s.is_empty() { + return Err(InvalidName::Empty); + } + if s.len() > Self::MAX_LEN { + return Err(InvalidName::TooLong); + } + if !s.bytes().all(|b| b.is_ascii_alphanumeric()) { + return Err(InvalidName::NotAlphanumeric); + } + Ok(SafeName(s.to_string())) + } +} + +impl Serialize for SafeName { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(&self.0) + } +} + +/// Deserializes by way of [`SafeName::from_str`], so the whitelist is enforced +/// by every deserializer and not just by explicit `parse` calls. +/// +/// This is what lets `axum::extract::Path` reject an invalid name on +/// its own: axum deserializes path parameters, so a bad name fails extraction +/// with `400 Bad Request` before the handler (and any peer proxying) runs. +impl<'de> Deserialize<'de> for SafeName { + fn deserialize>(deserializer: D) -> Result { + struct SafeNameVisitor; + + impl Visitor<'_> for SafeNameVisitor { + type Value = SafeName; + + fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("a non-empty alphanumeric name") + } + + fn visit_str(self, v: &str) -> Result { + v.parse().map_err(E::custom) + } + } + + deserializer.deserialize_str(SafeNameVisitor) + } +} + +/// The reason a string was rejected as a [`SafeName`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InvalidName { + /// The name was the empty string. + Empty, + + /// The name exceeded [`SafeName::MAX_LEN`]. + TooLong, + + /// The name contained a byte outside `A-Z`, `a-z`, and `0-9`. + NotAlphanumeric, +} + +impl fmt::Display for InvalidName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let msg = match self { + Self::Empty => "name is empty", + Self::TooLong => "name is too long", + Self::NotAlphanumeric => "name must be purely alphanumeric", + }; + f.write_str(msg) + } +} + +impl std::error::Error for InvalidName {} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn accepts_alphanumeric() { + for name in ["aB3xY9zQ1mK", "0123456789", "a", "Z"] { + assert_eq!(name.parse::().unwrap().as_str(), name); + } + } + + #[test] + fn rejects_path_metacharacters() { + // Each of these would change the meaning of an interpolated URL path. + for name in [ + "", + "..", + "a/b", + "a%2Fb", + "../../admin", + "a b", + "a\tb", + "a\nb", + "a?b", + "a#b", + "a\\b", + "ünïcode", + "a\0b", + ] { + assert!( + name.parse::().is_err(), + "expected {name:?} to be rejected" + ); + } + } + + #[test] + fn rejects_overlong() { + assert!("a".repeat(SafeName::MAX_LEN).parse::().is_ok()); + assert!("a" + .repeat(SafeName::MAX_LEN + 1) + .parse::() + .is_err()); + } + + #[test] + fn roundtrips_through_display() { + let name: SafeName = "abcDEF1234".parse().unwrap(); + assert_eq!(name.to_string(), "abcDEF1234"); + assert_eq!(name.as_str(), "abcDEF1234"); + } + + #[test] + fn generated_names_are_always_safe() { + // Guards the invariant that makes interpolating a session name into a + // URL path safe. If the generator's alphabet ever widens past + // alphanumeric, this fails rather than silently opening a hole. + for len in [1, 10, 14, 22] { + for _ in 0..64 { + let name = crate::rand_alphanumeric(len); + assert!( + name.parse::().is_ok(), + "generated name {name:?} (len {len}) is not a SafeName" + ); + } + } + } +} diff --git a/crates/sshx-server/src/web/socket.rs b/crates/sshx-server/src/web/socket.rs index 85a43d62..48843fc3 100644 --- a/crates/sshx-server/src/web/socket.rs +++ b/crates/sshx-server/src/web/socket.rs @@ -10,7 +10,7 @@ use axum::response::IntoResponse; use bytes::Bytes; use futures_util::SinkExt; use sshx_core::proto::{server_update::ServerMessage, NewShell, TerminalInput, TerminalSize}; -use sshx_core::Sid; +use sshx_core::{SafeName, Sid}; use subtle::ConstantTimeEq; use tokio::sync::mpsc; use tokio_stream::StreamExt; @@ -21,7 +21,7 @@ use crate::web::protocol::{WsClient, WsServer}; use crate::ServerState; pub async fn get_session_ws( - Path(name): Path, + Path(name): Path, ws: WebSocketUpgrade, State(state): State>, ) -> impl IntoResponse { @@ -93,7 +93,6 @@ async fn handle_socket(socket: &mut WebSocket, session: Arc) -> Result< let metadata = session.metadata(); let user_id = session.counter().next_uid(); - session.sync_now(); send(socket, WsServer::Hello(user_id, metadata.name.clone())).await?; let can_write = match recv(socket).await? { @@ -128,6 +127,10 @@ async fn handle_socket(socket: &mut WebSocket, session: Arc) -> Result< }; let _user_guard = session.user_scope(user_id, can_write)?; + // Persist the allocated ID counter, but only once the client is actually + // authenticated, so that unauthenticated connections cannot force the + // session to be snapshotted and written to storage. + session.sync_now(); let update_tx = session.update_tx(); // start listening for updates before any state reads let mut broadcast_stream = session.subscribe_broadcast(); diff --git a/crates/sshx-server/tests/simple.rs b/crates/sshx-server/tests/simple.rs index 3549030d..7822f41d 100644 --- a/crates/sshx-server/tests/simple.rs +++ b/crates/sshx-server/tests/simple.rs @@ -32,3 +32,53 @@ async fn test_web_get() -> Result<()> { Ok(()) } + +/// A session name is interpolated into an upstream URL when proxying between +/// mesh peers, so the `{name}` path parameter must be restricted to characters +/// that cannot alter the meaning of that URL. +/// +/// Each case performs a real WebSocket handshake, because that is the only +/// unambiguous discriminator: with validation in place the upgrade is refused +/// with `400`, whereas without it the upgrade succeeds and the handler runs. +/// A plain `GET` cannot distinguish the two, since `WebSocketUpgrade` also +/// rejects a non-upgrade request with `400`. +#[tokio::test] +async fn test_session_name_rejects_path_traversal() -> Result<()> { + let server = TestServer::new().await; + + // Control: a legitimate name must still be allowed to connect. + tokio_tungstenite::connect_async(server.ws_endpoint("aB3xY9zQ1mK")).await?; + + // These stay percent-encoded on the wire, since the `http` crate does not + // normalize paths. The server decodes them into `/` and `.` before + // validating, which is exactly what has to be rejected. + let rejected = [ + "a%2Fb", // decodes to "a/b" + "a%2F..%2F..%2Fevil", // decodes to "a/../../evil" + "%2E%2E%2Fadmin", // decodes to "../admin" + "%2E%2E", // decodes to ".." + "a%20b", // decodes to "a b" + "a+b", + ]; + + for name in rejected { + let uri = format!("ws://{}/api/s/{}", server.local_addr(), name); + let err = tokio_tungstenite::connect_async(&uri) + .await + .err() + .unwrap_or_else(|| panic!("expected {name:?} to be refused")); + + match err { + tokio_tungstenite::tungstenite::Error::Http(resp) => { + assert_eq!( + resp.status(), + http::StatusCode::BAD_REQUEST, + "expected {name:?} to be refused with 400" + ); + } + other => panic!("expected an HTTP rejection for {name:?}, got {other}"), + } + } + + Ok(()) +} diff --git a/package-lock.json b/package-lock.json index 3a6fb3eb..1b93b2c5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29,6 +29,7 @@ "@sveltejs/adapter-static": "^2.0.3", "@sveltejs/kit": "^1.24.1", "@types/lodash-es": "^4.17.9", + "@types/node": "^20.6.0", "@typescript-eslint/eslint-plugin": "^6.7.0", "@typescript-eslint/parser": "^6.7.0", "autoprefixer": "^10.4.15", @@ -821,9 +822,7 @@ "version": "20.6.0", "resolved": "https://registry.npmjs.org/@types/node/-/node-20.6.0.tgz", "integrity": "sha512-najjVq5KN2vsH2U/xyh2opaSEz6cZMR2SetLIlxlj08nOcmPOemJmUK2o4kUzfLqfrWE0PIrNeE16XhYDd3nqg==", - "dev": true, - "optional": true, - "peer": true + "dev": true }, "node_modules/@types/pug": { "version": "2.0.6", diff --git a/package.json b/package.json index a37520a9..169dd931 100644 --- a/package.json +++ b/package.json @@ -35,6 +35,7 @@ "@sveltejs/adapter-static": "^2.0.3", "@sveltejs/kit": "^1.24.1", "@types/lodash-es": "^4.17.9", + "@types/node": "^20.6.0", "@typescript-eslint/eslint-plugin": "^6.7.0", "@typescript-eslint/parser": "^6.7.0", "autoprefixer": "^10.4.15", diff --git a/rustfmt.toml b/rustfmt.toml index 768ef829..35b6afdd 100644 --- a/rustfmt.toml +++ b/rustfmt.toml @@ -3,4 +3,5 @@ group_imports = "StdExternalCrate" wrap_comments = true format_strings = true normalize_comments = true +comment_width = 100 reorder_impl_items = true