sshwire improvements

demo/common/src/config.rs

@@ -123,7 +123,7 @@ fn enc_signkey(k: &SignKey, s: &mut dyn SSHSink) -> WireResult<()> {
     // need to add a variant field if we support more key types.
     match k {
         SignKey::Ed25519(k) => k.to_bytes().enc(s),
-        _ => Err(WireError::UnknownVariant),
+        _ => Err(WireError::EncodeUnknown),
     }
 }
 

src/kex.rs

@@ -782,9 +782,10 @@ impl SharedSecret {
             }
         }?;
 
-        // TODO: error message on signature failure.
-        let h: &[u8] = kex_out.h.as_ref();
+        // OK unwrap, hash is always sha256. Pending output_size() being const.
+        let h: &[u8; 32] = kex_out.h.as_slice().try_into().unwrap();
         trace!("verify  h {h:02x?}");
+        // TODO: error message on signature failure.
         algos
             .hostsig
             .verify(&p.k_s.0, &h, &p.sig.0)
@@ -846,7 +847,9 @@ impl SharedSecret {
 
         let k_s = Blob(hostkey.pubkey());
         trace!("sign kexreply h {:02x?}", ko.h.as_slice());
-        let sig = hostkey.sign(&ko.h.as_slice())?;
+        // OK unwrap, hash is always sha256.
+        let h: &[u8; 32] = ko.h.as_slice().try_into().unwrap();
+        let sig = hostkey.sign(h)?;
         let sig: Signature = (&sig).into();
         let sig = Blob(sig);
         s.send(packets::KexDHReply { k_s, q_s, sig })

src/namelist.rs

@@ -76,7 +76,7 @@ impl SSHEncode for &LocalNames {
             + names.len().saturating_sub(1);
         (strlen as u32).enc(s)?;
         for i in 0..names.len() {
-            names[i].as_bytes().enc(s)?;
+            s.push(names[i].as_bytes())?;
             if i < names.len() - 1 {
                 b','.enc(s)?;
             }

src/packets.rs

@@ -10,8 +10,7 @@ use {
     log::{debug, error, info, log, trace, warn},
 };
 
-use core::fmt;
-use core::fmt::{Debug, Display};
+use core::fmt::{self, Debug};
 
 #[cfg(feature = "arbitrary")]
 use arbitrary::Arbitrary;
@@ -25,8 +24,7 @@ use crate::*;
 use namelist::NameList;
 use sign::{OwnedSig, SigType};
 use sshnames::*;
-use sshwire::SSHEncodeEnum;
-use sshwire::{BinString, Blob, TextString};
+use sshwire::{BinString, Blob, TextString, Unknown};
 use sshwire::{SSHDecode, SSHEncode, SSHSink, SSHSource, WireError, WireResult};
 
 #[cfg(feature = "rsa")]
@@ -952,46 +950,6 @@ pub struct DirectTcpip<'a> {
     pub origin_port: u32,
 }
 
-/// 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
-/// automatically create instances.
-#[derive(Clone, PartialEq)]
-pub struct Unknown<'a>(pub &'a [u8]);
-
-impl<'a> Unknown<'a> {
-    pub fn new(u: &'a [u8]) -> Self {
-        let u = Unknown(u);
-        trace!("saw unknown variant \"{u}\"");
-        u
-    }
-}
-
-impl Display for Unknown<'_> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        if let Ok(s) = sshwire::try_as_ascii_str(self.0) {
-            f.write_str(s)
-        } else {
-            write!(f, "non-ascii {:02x?}", self.0)
-        }
-    }
-}
-
-impl Debug for Unknown<'_> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        Display::fmt(self, f)
-    }
-}
-
-#[cfg(feature = "arbitrary")]
-impl<'arb: 'a, 'a> Arbitrary<'arb> for Unknown<'a> {
-    fn arbitrary(u: &mut arbitrary::Unstructured<'arb>) -> arbitrary::Result<Self> {
-        Ok(Self(arbitrary::Arbitrary::arbitrary(u)?))
-    }
-}
-
 /// State to be passed to decoding.
 /// Use this so the parser can select the correct enum variant to decode.
 #[derive(Default, Clone, Debug)]

src/sshwire.rs

@@ -12,7 +12,7 @@ use {
 };
 
 use core::convert::AsRef;
-use core::fmt::{Debug, Display};
+use core::fmt::{self, Debug, Display};
 use core::str::FromStr;
 
 use ascii::{AsAsciiStr, AsciiChar, AsciiStr};
@@ -76,7 +76,7 @@ pub trait SSHDecodeEnum<'de>: Sized {
 ///
 /// Compiled code size is very sensitive to the size of this
 /// enum so we avoid unused elements.
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq)]
 pub enum WireError {
     NoRoom,
 
@@ -86,7 +86,7 @@ pub enum WireError {
 
     BadName,
 
-    UnknownVariant,
+    EncodeUnknown,
 
     PacketWrong,
 
@@ -96,6 +96,10 @@ pub enum WireError {
 
     BadNumber,
 
+    UnknownVariant,
+
+    SSHProtoUnsupported,
+
     UnknownPacket { number: u8 },
 }
 
@@ -110,16 +114,56 @@ impl From<WireError> for Error {
             WireError::PacketWrong => error::PacketWrong.build(),
             WireError::BadKey => Error::BadKey,
             WireError::BadNumber => Error::BadNumber,
-            WireError::UnknownVariant => {
-                Error::build_bug_msg("Can't encode Unknown")
-            }
+            WireError::EncodeUnknown => Error::build_bug_msg("Can't encode Unknown"),
+            WireError::UnknownVariant => Error::UnknownMethod { kind: "" },
+            WireError::SSHProtoUnsupported => Error::SSHProtoUnsupported,
             WireError::UnknownPacket { number } => Error::UnknownPacket { number },
         }
     }
 }
 
 pub type WireResult<T> = core::result::Result<T, WireError>;
 
+/// Placeholder for unknown variants.
+///
+/// Unknown SSH methods 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]);
+
+impl<'a> Unknown<'a> {
+    pub fn new(u: &'a [u8]) -> Self {
+        let u = Unknown(u);
+        trace!("saw unknown variant \"{u}\"");
+        u
+    }
+}
+
+impl Display for Unknown<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        if let Ok(s) = sshwire::try_as_ascii_str(self.0) {
+            f.write_str(s)
+        } else {
+            write!(f, "non-ascii {:02x?}", self.0)
+        }
+    }
+}
+
+impl Debug for Unknown<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        Display::fmt(self, f)
+    }
+}
+
+#[cfg(feature = "arbitrary")]
+impl<'arb: 'a, 'a> Arbitrary<'arb> for Unknown<'a> {
+    fn arbitrary(u: &mut arbitrary::Unstructured<'arb>) -> arbitrary::Result<Self> {
+        Ok(Self(arbitrary::Arbitrary::arbitrary(u)?))
+    }
+}
+
 ///////////////////////////////////////////////
 
 /// Parses a [`Packet`] from a borrowed `&[u8]` byte buffer.
@@ -161,8 +205,12 @@ pub fn write_ssh(target: &mut [u8], value: &dyn SSHEncode) -> Result<usize> {
 pub fn ssh_push_vec(target: &mut Vec<u8>, value: &dyn SSHEncode) -> Result<()> {
     let orig = target.len();
     let l = length_enc(value)? as usize;
-    target.resize(orig + l, 0);
-    write_ssh(&mut target[orig..], value)?;
+    let l = orig.checked_add(l).ok_or(error::NoRoom.build())?;
+    target.resize(l, 0);
+    let wl = write_ssh(&mut target[orig..], value)
+        // revert length on failure
+        .inspect_err(|_| target.truncate(orig))?;
+    debug_assert_eq!(wl, l);
     Ok(())
 }
 
@@ -290,7 +338,7 @@ impl Debug for BinString<'_> {
 impl SSHEncode for BinString<'_> {
     fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> {
         (self.0.len() as u32).enc(s)?;
-        self.0.enc(s)
+        s.push(self.0)
     }
 }
 
@@ -379,8 +427,7 @@ impl Display for TextString<'_> {
 
 impl SSHEncode for TextString<'_> {
     fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> {
-        (self.0.len() as u32).enc(s)?;
-        self.0.enc(s)
+        BinString(self.0).enc(s)
     }
 }
 
@@ -519,7 +566,7 @@ impl SSHEncode for Mpint<'_> {
         if pad {
             0u8.enc(s)?;
         }
-        self.0.enc(s)
+        s.push(self.0)
     }
 }
 
@@ -564,14 +611,6 @@ impl SSHEncode for u64 {
     }
 }
 
-// no length prefix
-impl SSHEncode for &[u8] {
-    fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> {
-        // data
-        s.push(self)
-    }
-}
-
 // no length prefix
 impl<const N: usize> SSHEncode for [u8; N] {
     fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> {
@@ -707,6 +746,23 @@ impl<'de, const N: usize> SSHDecode<'de> for heapless::String<N> {
     }
 }
 
+#[cfg(feature = "alloc")]
+impl SSHEncode for alloc::string::String {
+    fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> {
+        self.as_str().enc(s)
+    }
+}
+
+#[cfg(feature = "alloc")]
+impl<'de> SSHDecode<'de> for alloc::string::String {
+    fn dec<S>(s: &mut S) -> WireResult<Self>
+    where
+        S: SSHSource<'de>,
+    {
+        Ok(<&str>::dec(s)?.into())
+    }
+}
+
 /// Like `digest::DynDigest` but simpler.
 ///
 /// Doesn't have any optional methods that depend on `alloc`.
@@ -754,7 +810,7 @@ impl<T: SSHWireDigestUpdate> From<T> for SSHWireDigestTrace<T> {
 impl SSHEncode for rsa::BoxedUint {
     fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> {
         let b = self.to_be_bytes();
-        Mpint(&b).enc(s)
+        Mpint::new(&b).enc(s)
     }
 }
 

sshwire-derive/Cargo.toml

@@ -11,3 +11,6 @@ proc-macro = true
 
 [dependencies]
 virtue = "=0.0.17" # Last released version
+
+[dev-dependencies]
+sunset = { workspace = true, features = ["std"]}