From a01b7ca62d88d4498c5bc8ebe2d85325aacc69ea Mon Sep 17 00:00:00 2001 From: Matt Johnston <matt@ucc.asn.au> Date: Wed, 12 Apr 2023 23:58:29 +0800 Subject: [PATCH] Fix and expand kex test, add no_async helper Also fix pubkey packet test and fix some doc formatting --- Cargo.toml | 4 + README.md | 10 ++- src/kex.rs | 220 ++++++++++++++++++++++++++++++++++++++----------- src/lib.rs | 2 + src/noasync.rs | 28 +++++++ src/packets.rs | 11 +-- src/runner.rs | 6 +- src/sign.rs | 7 +- src/traffic.rs | 7 +- 9 files changed, 225 insertions(+), 70 deletions(-) create mode 100644 src/noasync.rs diff --git a/Cargo.toml b/Cargo.toml index a918fc6..107c404 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,6 +62,9 @@ embedded-io = { version = "0.4", optional = true } # for debug printing pretty-hex = { version = "0.3", default-features = false } +# for non_async +futures = "0.3" + [features] std = ["snafu/std", "snafu/backtraces", "rsa"] rsa = ["dep:rsa", "ssh-key/rsa"] @@ -77,6 +80,7 @@ anyhow = { version = "1.0" } pretty-hex = "0.3" simplelog = { version = "0.12", features = ["test"] } + [patch.crates-io] # needed for Default WakerRegistration, https://github.com/embassy-rs/embassy/commit/14a2d1524080593f7795fe14950a3f0ee6e2b409 embassy-sync = { git = "https://github.com/embassy-rs/embassy", rev = "e1eac15c429f88b1176109d6ce42185e2774ac86" } diff --git a/README.md b/README.md index ab7a364..ea1c106 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ suggest something! At present the Pico W build is around 150kB binary size (plus ~200KB [cyw43](https://github.com/embassy-rs/cyw43/) wifi firmware), - using about 30kB RAM per concurrent SSH session (max stack size not confirmed). + using about 15kB RAM per concurrent SSH session (max stack size not confirmed). - [`sunset-async`](async/) adds functionality to use Sunset as a normal SSH client or server async library in normal Rust (not `no_std`). The @@ -37,16 +37,16 @@ Working: - curve25519 key exchange - chacha20-poly1305, aes256-ctr ciphers - hmac-sha256 integrity +- rsa (will be `std`-only unless someone writes a `no_std` crate) +- `~.` client escape sequences Desirable: - TCP forwarding -- rsa (will be `std`-only unless someone writes a `no_std` crate) - dh-group14 (probably `std`-only, need to investigate crates) - Perhaps aes256-gcm - Perhaps ECDSA, hardware often supports it ahead of ed25519 - SFTP -- `~.` client escape sequences ## License @@ -54,7 +54,9 @@ Currently MPL2, though may possibly move to MIT-style in future (I'm undecided) ## Rust versions -`sunset` core builds on stable Rust. There's no MSRV guarantee now since it's early. +At present `sunset` requires nightly Rust, in order to use async functions in +the `Behaviour` traits. It is intended to switch to stable Rust once that +feature stabilises. `sunset-embassy` requires a nightly Rust version, as required by Embassy. See the [embassy/rust-toolchain.toml](rust-toolchain.toml) for a known-good version. diff --git a/src/kex.rs b/src/kex.rs index 4f1fd5f..4ba22af 100644 --- a/src/kex.rs +++ b/src/kex.rs @@ -536,6 +536,7 @@ impl SharedSecret { // TODO: error message on signature failure. let h: &[u8] = kex_out.h.as_ref(); + trace!("verify h {}", h.hex_dump()); algos.hostsig.verify(&p.k_s.0, &h, &p.sig.0, None)?; debug!("Hostkey signature is valid"); if matches!(b.valid_hostkey(&p.k_s.0), Ok(true)) { @@ -572,6 +573,7 @@ impl SharedSecret { let q_s = BinString(kex_pub); let k_s = Blob(hostkey.pubkey()); + trace!("sign kexreply h {}", ko.h.as_slice().hex_dump()); let sig = hostkey.sign(&ko.h.as_slice(), None)?; let sig: Signature = (&sig).into(); let sig = Blob(sig); @@ -719,7 +721,7 @@ impl KexCurve25519 { mod tests { use pretty_hex::PrettyHex; - use crate::encrypt; + use crate::encrypt::{self, SSH_PAYLOAD_START, KeyState}; use crate::error::Error; use crate::ident::RemoteVersion; use crate::kex::*; @@ -727,6 +729,7 @@ mod tests { use crate::packets::{Packet,ParseContext}; use crate::*; use crate::sunsetlog::init_test_log; + use std::collections::VecDeque; // TODO: // - test algo negotiation @@ -776,7 +779,8 @@ mod tests { } /// Round trip a `Packet` - fn _reencode<'a>(out_buf: &'a mut [u8], p: Packet, ctx: &ParseContext) -> Packet<'a> { + fn reencode<'a>(out_buf: &'a mut [u8], p: Packet) -> Packet<'a> { + let ctx = Default::default(); let l = sshwire::write_ssh(out_buf, &p).unwrap(); sshwire::packet_from_bytes(&out_buf[..l], &ctx).unwrap() } @@ -803,77 +807,193 @@ mod tests { } } - // struct BlankTrafSend { - // buf: Vec<u8>, - // keys: encrypt::KeyState, - // } + struct TestCliBehaviour { + allow_key: bool, + } - // impl BlankTrafSend { - // fn new() -> Self { - // Self { - // buf: vec![0u8, 3000], - // keys: encrypt::KeyState::new_cleartext(), - // } - // } + impl CliBehaviour for TestCliBehaviour { + fn username(&mut self) -> BhResult<ResponseString> { + Ok("matt".try_into().unwrap()) + } - // fn sender(&mut self) -> traffic::TrafSend { - // let mut t = traffic::TrafOut::new(&mut self.buf); - // t.sender(&mut self.keys) - // } - // } + fn valid_hostkey(&mut self, key: &PubKey) -> BhResult<bool> { + Ok(self.allow_key) + } + fn authenticated(&mut self) { + } + } + + /// A debug fixture to capture output then deserialize it. + /// Leaks lots. + struct TrafCatcher { + traf_out: traffic::TrafOut<'static>, + traf_in: traffic::TrafIn<'static>, + keys: encrypt::KeyState, + rv: RemoteVersion, + + buf: VecDeque<u8>, + } + + // Round trips packets through TrafOut/TrafIn, allowing + // to capture sent packets. + // This leaks vectors rather than dealing with borrowed Packets + impl TrafCatcher { + fn new() -> Self { + let traf_in = traffic::TrafIn::new(vec![0u8; 3000].leak()); + let mut rv = RemoteVersion::new(); + rv.consume(b"SSH-2.0-thing\r\n").unwrap(); + rv.version().unwrap(); + + Self { + traf_out: traffic::TrafOut::new(vec![0u8; 3000].leak()), + traf_in, + keys: encrypt::KeyState::new_cleartext(), + rv, + buf: VecDeque::new(), + } + } + + fn sender<'f>(&'f mut self) -> traffic::TrafSend<'f, 'static> { + self.traf_out.sender(&mut self.keys) + } + + // Returns Some(packet), or None if empty + fn next(&mut self) -> Option<Packet<'static>> { + // get output + let mut b = vec![0u8; 3000]; + let l = self.traf_out.output(b.as_mut_slice()); + assert!(l < b.len(), "Not enough space"); + let b = &b[..l]; + + self.buf.extend(b.iter()); + let b = self.buf.make_contiguous(); + + self.traf_in.done_payload(false); + let l = self.traf_in.input(&mut self.keys, &mut self.rv, b).unwrap(); + self.buf.drain(..l); + + self.traf_in.payload().map(|(payload, _seq)| { + let payload = Vec::from(payload).leak(); + sshwire::packet_from_bytes(payload, &Default::default()).unwrap() + }) + } + } #[test] - fn test_agree_kex() { + fn test_agree_kex_allow_key() { + test_agree_kex(true) + } + + #[test] + #[should_panic(expectd = "Host key rejected")] + fn test_agree_kex_disallow_key() { + test_agree_kex(false) + } + + // other things to test: + // - first_follows, and kexguess2 + + fn test_agree_kex(allow_key: bool) { #![allow(unused)] init_test_log(); - // let mut bufc = [0u8; 1000]; - // let mut bufs = [0u8; 1000]; let cli_conf = kex::AlgoConfig::new(true); let serv_conf = kex::AlgoConfig::new(false); - let mut serv_version = RemoteVersion::new(); + // needs to be hardcoded because that's what we send. - serv_version.consume(b"SSH-2.0-sunset\r\n").unwrap(); - let mut cli_version = RemoteVersion::new(); - cli_version.consume(b"SSH-2.0-sunset\r\n").unwrap(); + let mut s = Vec::from(crate::ident::OUR_VERSION); + s.extend_from_slice(b"\r\n"); + let mut version = RemoteVersion::new(); + version.consume(s.as_slice()).unwrap(); + let mut keys = vec![]; + keys.push(crate::SignKey::generate(crate::KeyType::Ed25519, None).unwrap()); let mut sb = TestServBehaviour { - // TODO: put keys in - keys: vec![] + keys, }; let mut sb = Behaviour::<behaviour::UnusedCli, _>::new_server(&mut sb); - let _sb = sb.server().unwrap(); + let sb = sb.server().unwrap(); + let mut cb = TestCliBehaviour { + allow_key + }; + let mut cb = Behaviour::<_, behaviour::UnusedServ>::new_client(&mut cb); + let cb = cb.client().unwrap(); + + let mut ts = TrafCatcher::new(); + let mut tc = TrafCatcher::new(); + + let mut cli = kex::Kex::new(); + let mut serv = kex::Kex::new(); + + serv.send_kexinit(&serv_conf, &mut ts.sender()).unwrap(); + cli.send_kexinit(&cli_conf, &mut tc.sender()).unwrap(); + + let cli_init = tc.next().unwrap(); + let cli_init = if let Packet::KexInit(k) = cli_init { k } else { panic!() }; + assert!(tc.next().is_none()); + let serv_init = ts.next().unwrap(); + let serv_init = if let Packet::KexInit(k) = serv_init { k } else { panic!() }; + assert!(ts.next().is_none()); - let cli = kex::Kex::new(); - let serv = kex::Kex::new(); + serv.handle_kexinit(cli_init, false, &serv_conf, &version, &mut ts.sender()).unwrap(); + cli.handle_kexinit(serv_init, true, &cli_conf, &version, &mut tc.sender()).unwrap(); - // // reencode so we end up with NameList::String not Local - // let ctx = ParseContext::new(); - // let si = serv.make_kexinit(&serv_conf); - // let _si = reencode(&mut bufs, si, &ctx); - // let ci = cli.make_kexinit(&cli_conf); - // let _ci = reencode(&mut bufc, ci, &ctx); + let cli_dhinit = tc.next().unwrap(); + let cli_dhinit = if let Packet::KexDHInit(k) = cli_dhinit { k } else { panic!() }; + assert!(tc.next().is_none()); - // TODO fix this + assert!(ts.next().is_none()); - // let ts = BlankTrafSend::new(); - // let s = ts.sender(); - // serv.handle_kexinit(&ci, false, &serv_conf, &cli_version, &mut s).unwrap(); - // cli.handle_kexinit(&si, true, &cli_conf, &serv_version, &mut s).unwrap(); + serv.handle_kexdhinit(&cli_dhinit, &mut ts.sender(), sb).unwrap(); + let serv_dhrep = ts.next().unwrap(); + let serv_dhrep = if let Packet::KexDHReply(k) = serv_dhrep { k } else { panic!() }; + assert!(matches!(ts.next().unwrap(), Packet::NewKeys(_))); - // let ci = cli.make_kexdhinit().unwrap(); - // let ci = if let Packet::KexDHInit(k) = ci { k } else { panic!() }; - // let sout = serv.handle_kexdhinit(&ci, &None, &mut s, sb).unwrap(); + let s = &mut tc.sender(); + let f = cli.handle_kexdhreply(&serv_dhrep, s, cb); + let f = crate::non_async(f).unwrap(); + f.unwrap(); + assert!(matches!(tc.next().unwrap(), Packet::NewKeys(_))); + let (cout, calgos) = if let Kex::NewKeys { output, algos } = cli { + (output, algos) + } else { + panic!(); + }; + let (sout, salgos) = if let Kex::NewKeys { output, algos } = serv { + (output, algos) + } else { + panic!(); + }; - // let kexreply = sout.make_kexdhreply(sb); + // output hash matches + assert_eq!(cout.h, sout.h); - // let kexreply = - // if let Packet::KexDHReply(k) = kexreply { k } else { panic!() }; + // roundtrip with the derived keys + let sess_id = SessId::from_slice(&Sha256::digest(b"some sessid")).unwrap(); + + let mut skeys = crate::encrypt::KeyState::new_cleartext(); + skeys.rekey(Keys::derive(sout, &sess_id, &salgos).unwrap()); + let mut ckeys = crate::encrypt::KeyState::new_cleartext(); + ckeys.rekey(Keys::derive(cout, &sess_id, &calgos).unwrap()); + + roundtrip(b"this", &mut skeys, &mut ckeys); + roundtrip(&[13u8; 50], &mut ckeys, &mut skeys); + + } - // TODO need host signatures for it to succeed - // let cout = cli.handle_kexdhreply(&kexreply, &None).unwrap(); + fn roundtrip(payload: &[u8], enc: &mut KeyState, dec: &mut KeyState) { + let mut b = vec![]; + b.resize(SSH_PAYLOAD_START, 0); + b.extend_from_slice(payload); + b.resize(100, 0); - // assert_eq!(cout.h.as_ref(), sout.h.as_ref()); + let l = enc.encrypt(payload.len(), &mut b).unwrap(); + let l_dec = dec.decrypt_first_block(&mut b).unwrap(); + assert_eq!(l, l_dec); + b.resize(l_dec, 0u8); + let l = dec.decrypt(&mut b).unwrap(); + let dec_payload = &b[SSH_PAYLOAD_START..SSH_PAYLOAD_START+l]; + assert_eq!(payload, dec_payload); } } diff --git a/src/lib.rs b/src/lib.rs index a8b0536..6b5c09c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -41,6 +41,7 @@ pub mod behaviour; mod termmodes; mod ssh_chapoly; mod traffic; +mod noasync; pub mod packets; pub mod sshwire; @@ -60,3 +61,4 @@ pub use sshnames::ChanFail; pub use channel::{ChanData, ChanNum}; pub use runner::ChanHandle; pub use auth::AuthSigMsg; +pub use noasync::non_async; diff --git a/src/noasync.rs b/src/noasync.rs new file mode 100644 index 0000000..693fba1 --- /dev/null +++ b/src/noasync.rs @@ -0,0 +1,28 @@ +#[allow(unused_imports)] +use { + crate::error::{Error, Result, TrapBug}, + log::{debug, error, info, log, trace, warn}, +}; + +use crate::*; + +use core::task::{Context, Poll}; +use core::future::Future; + +pub struct PendingAwait; + +/// Runs an async function that is not expected to `.await`. +/// +/// Returns `Err(PendingAwait)` if the Future attempts to perform an asynchronous operation. +/// This is intended to be used by non-async applications to wrap a call to [`Runner::progress()`], +/// where all [`CliBehaviour`] or [`ServBehaviour`] callback implementations are known to be non-awaiting. +pub fn non_async<F>(f: F) -> Result<F::Output, PendingAwait> where F: Future { + futures::pin_mut!(f); + + let w = futures::task::noop_waker(); + + match f.poll(&mut Context::from_waker(&w)) { + Poll::Ready(r) => Ok(r), + Poll::Pending => Err(PendingAwait), + } +} diff --git a/src/packets.rs b/src/packets.rs index d24c615..dc31909 100644 --- a/src/packets.rs +++ b/src/packets.rs @@ -754,9 +754,9 @@ pub struct DirectTcpip<'a> { /// Placeholder for unknown method names. /// -///These are sometimes non-fatal and -/// need to be handled by the relevant code, for example newly invented pubkey types -/// This is deliberately not SSHEncode, we only receive it. sshwire-derive will +/// These are sometimes non-fatal and +/// need to be handled by the relevant code, for example newly invented pubkey types. +/// This is deliberately not `SSHEncode`, we only receive it. sshwire-derive will /// automatically create instances. #[derive(Clone, PartialEq)] pub struct Unknown<'a>(pub &'a [u8]); @@ -1018,11 +1018,12 @@ mod tests { fn roundtrip_authpubkey() { init_test_log(); // with None sig - let k = SignKey::generate(KeyType::Ed25519).unwrap(); + let k = SignKey::generate(KeyType::Ed25519, None).unwrap(); + let method = AuthMethod::PubKey(MethodPubKey::new(k.pubkey(), None).unwrap()); let p = UserauthRequest { username: "matt".into(), service: "conn".into(), - method: k.pubkey().try_into().unwrap(), + method, }.into(); test_roundtrip(&p); diff --git a/src/runner.rs b/src/runner.rs index 04c29bc..9a0a945 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -113,7 +113,9 @@ impl<'a, C: CliBehaviour, S: ServBehaviour> Runner<'a, C, S> { /// does so. Note that some computationally intensive operations may be performed /// during key exchange. /// - /// Returns Ok(true) if an input packet was handled, Ok(false) if no packet was ready + /// Non-async callers can wrap this with the [`non_async`] helper function. + /// + /// Returns `Ok(true)` if an input packet was handled, `Ok(false)` if no packet was ready /// (Can also return various errors) pub async fn progress(&mut self, behaviour: &mut Behaviour<'_, C, S>) -> Result<Progress> @@ -134,7 +136,7 @@ impl<'a, C: CliBehaviour, S: ServBehaviour> Runner<'a, C, S> { } else { // other packets have been completed trace!("handle_payload done"); - self.traf_in.done_payload(d.zeroize_payload)?; + self.traf_in.done_payload(d.zeroize_payload); } } diff --git a/src/sign.rs b/src/sign.rs index 0cf37d8..f748f0d 100644 --- a/src/sign.rs +++ b/src/sign.rs @@ -164,9 +164,10 @@ pub enum KeyType { RSA, } -/// A SSH signing key. This may hold the private part locally -/// or could potentially send the signing requests to a SSH agent -/// or other entitiy. +/// A SSH signing key. +/// +/// This may hold the private key part locally +/// or potentially send the signing requests to an SSH agent or other entity. #[derive(ZeroizeOnDrop)] pub enum SignKey { // TODO bloat: this is an expanded keypair, we should store the raw bytes diff --git a/src/traffic.rs b/src/traffic.rs index f5726d6..ba4221e 100644 --- a/src/traffic.rs +++ b/src/traffic.rs @@ -95,7 +95,6 @@ impl<'a> TrafIn<'a> { } pub fn is_input_ready(&self) -> bool { - info!("is_input_ready {:?}", self.state); match self.state { | RxState::Idle | RxState::ReadInitial { .. } @@ -114,7 +113,6 @@ impl<'a> TrafIn<'a> { buf: &[u8], ) -> Result<usize, Error> { let mut inlen = 0; - info!("assert"); debug_assert!(self.is_input_ready()); if remote_version.version().is_none() && matches!(self.state, RxState::Idle) { // Handle initial version string @@ -127,20 +125,17 @@ impl<'a> TrafIn<'a> { } /// Called when `payload()` and `payload_reborrow()` are complete. - pub(crate) fn done_payload(&mut self, zeroize: bool) -> Result<(), Error> { + pub(crate) fn done_payload(&mut self, zeroize: bool) { match self.state { RxState::InPayload { len, .. } => { if zeroize { self.buf[SSH_PAYLOAD_START..SSH_PAYLOAD_START + len].zeroize(); } - trace!("channel_input idle was {:?} done_payload", self.state); self.state = RxState::Idle; - Ok(()) } _ => { // Just ignore it // warn!("done_payload called without payload, st {:?}", self.state); - Ok(()) } } } -- GitLab