From 68b32cecb095400768ab914ca1925f11e613d549 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Mon, 5 Dec 2022 22:38:16 +0800
Subject: [PATCH] Tidying and comments
Fix a few things from cargo clippy
---
src/behaviour.rs | 31 +++++++++++++++++++++++--------
src/channel.rs | 12 +++++++-----
src/conn.rs | 5 +++--
src/encrypt.rs | 11 +++++------
src/kex.rs | 2 +-
src/namelist.rs | 1 +
src/packets.rs | 5 +++--
src/servauth.rs | 2 +-
src/sign.rs | 14 ++++++++++++--
src/sshwire.rs | 24 ++++++++++++++++++------
10 files changed, 74 insertions(+), 33 deletions(-)
diff --git a/src/behaviour.rs b/src/behaviour.rs
index 62adb23..a126f2e 100644
--- a/src/behaviour.rs
+++ b/src/behaviour.rs
@@ -67,20 +67,18 @@ impl<'a> Behaviour<'a> {
/// Calls either client or server
pub(crate) fn open_tcp_forwarded(&mut self, chan: u32,
t: &ForwardedTcpip) -> channel::ChanOpened {
- if self.is_client() {
- self.client().unwrap().open_tcp_forwarded(chan, t)
- } else {
- self.server().unwrap().open_tcp_forwarded(chan, t)
+ match self {
+ Self::Client(b) => b.open_tcp_forwarded(chan, t),
+ Self::Server(b) => b.open_tcp_forwarded(chan, t),
}
}
/// Calls either client or server
pub(crate) fn open_tcp_direct(&mut self, chan: u32,
t: &DirectTcpip) -> channel::ChanOpened {
- if self.is_client() {
- self.client().unwrap().open_tcp_direct(chan, t)
- } else {
- self.server().unwrap().open_tcp_direct(chan, t)
+ match self {
+ Self::Client(b) => b.open_tcp_direct(chan, t),
+ Self::Server(b) => b.open_tcp_direct(chan, t),
}
}
@@ -178,29 +176,46 @@ pub trait ServBehaviour: Sync+Send {
#[allow(unused)]
// TODO: or return a slice of enums
+ /// Implementations should take care to avoid leaking user existence
+ /// based on timing.
fn have_auth_password(&self, username: TextString) -> bool {
false
}
#[allow(unused)]
+ /// Implementations should take care to avoid leaking user existence
+ /// based on timing.
fn have_auth_pubkey(&self, username: TextString) -> bool {
false
}
#[allow(unused)]
/// Return true to allow the user to log in with no authentication
+ ///
+ /// Implementations may need to take care to avoid leaking user existence
+ /// based on timing.
fn auth_unchallenged(&mut self, username: TextString) -> bool {
false
}
#[allow(unused)]
// TODO: change password
+ /// Implementations should perform password hash comparisons
+ /// in constant time, using
+ /// [`subtle::ConstantTimeEq`](subtle::ConstantTimeEq) or similar.
+ ///
+ /// Implementations may need to take care to avoid leaking user existence
+ /// based on timing.
fn auth_password(&mut self, username: TextString, password: TextString) -> bool {
false
}
/// Returns true if the pubkey can be used to log in.
+ ///
/// TODO: allow returning pubkey restriction options
+ ///
+ /// Implementations may need to take care to avoid leaking user existence
+ /// based on timing.
#[allow(unused)]
fn auth_pubkey(&mut self, username: TextString, pubkey: &PubKey) -> bool {
false
diff --git a/src/channel.rs b/src/channel.rs
index c6349ea..5fef02a 100644
--- a/src/channel.rs
+++ b/src/channel.rs
@@ -67,7 +67,7 @@ impl Channels {
ty: packets::ChannelOpenType<'b>,
init_req: InitReqs,
) -> Result<(&Channel, Packet<'b>)> {
- let (num, ch) = self.unused_chan()?;
+ let num = self.unused_chan()?;
let chan = Channel::new(num, (&ty).into(), init_req);
let p = packets::ChannelOpen {
@@ -77,6 +77,7 @@ impl Channels {
ty,
}
.into();
+ let ch = &mut self.ch[num as usize];
*ch = Some(chan);
Ok((ch.as_ref().unwrap(), p))
}
@@ -144,19 +145,19 @@ impl Channels {
}
/// Returns the first available channel
- fn unused_chan(&mut self) -> Result<(u32, &mut Option<Channel>)> {
+ fn unused_chan(&self) -> Result<u32> {
self.ch
- .iter_mut()
+ .iter()
.enumerate()
.find_map(
- |(i, ch)| if ch.as_mut().is_none() { Some((i as u32, ch)) } else { None },
+ |(i, ch)| if ch.as_ref().is_none() { Some(i as u32) } else { None },
)
.ok_or(Error::NoChannels)
}
/// Creates a new channel in InOpen state.
fn reserve_chan(&mut self, co: &ChannelOpen<'_>) -> Result<&mut Channel> {
- let (num, ch) = self.unused_chan()?;
+ let num = self.unused_chan()?;
let mut chan = Channel::new(num, (&co.ty).into(), Vec::new());
chan.send = Some(ChanDir {
num: co.num,
@@ -165,6 +166,7 @@ impl Channels {
});
chan.state = ChanState::InOpen;
+ let ch = &mut self.ch[num as usize];
*ch = Some(chan);
Ok(ch.as_mut().unwrap())
}
diff --git a/src/conn.rs b/src/conn.rs
index 954f771..72374cb 100644
--- a/src/conn.rs
+++ b/src/conn.rs
@@ -84,6 +84,7 @@ enum ConnState {
}
#[derive(Default)]
+/// Returned state from `handle_payload()` for `Runner` to use.
pub(crate) struct Dispatched {
pub data_in: Option<channel::DataIn>,
/// set for sensitive payloads such as password auth
@@ -251,7 +252,7 @@ impl Conn {
}
Packet::KexDHInit(p) => {
match self.state {
- ConnState::InKex { done_auth: _, ref mut output } => {
+ ConnState::InKex { ref mut output, .. } => {
if self.cliserv.is_client() {
// TODO: client/server validity checks should move somewhere more general
trace!("kexdhinit not server");
@@ -270,7 +271,7 @@ impl Conn {
}
Packet::KexDHReply(p) => {
match self.state {
- ConnState::InKex { done_auth: _, ref mut output } => {
+ ConnState::InKex { ref mut output, .. } => {
if let ClientServer::Client(_cli) = &mut self.cliserv {
if self.kex.maybe_discard_packet() {
// ok
diff --git a/src/encrypt.rs b/src/encrypt.rs
index 0a986f7..b1b882c 100644
--- a/src/encrypt.rs
+++ b/src/encrypt.rs
@@ -44,8 +44,7 @@ const MAX_KEY_LEN: usize = 64;
#[derive(Debug)]
pub(crate) struct KeyState {
keys: Keys,
- // Packet sequence numbers. These must be transferred to subsequent KeyState
- // since they don't reset with rekeying.
+ // Packet sequence numbers. These don't reset with rekeying.
seq_encrypt: Wrapping<u32>,
seq_decrypt: Wrapping<u32>,
}
@@ -157,7 +156,7 @@ impl Keys {
}
}
- pub fn new_from(
+ pub fn derive(
k: &[u8], h: &SessId, sess_id: &SessId, algos: &kex::Algos,
) -> Result<Self, Error> {
let mut key = [0u8; MAX_KEY_LEN];
@@ -816,7 +815,7 @@ mod tests {
let sharedkey = b"hello";
trace!("algos enc {algos:?}");
- let newkeys = Keys::new_from(sharedkey, &h, &sess_id, &algos).unwrap();
+ let newkeys = Keys::derive(sharedkey, &h, &sess_id, &algos).unwrap();
keys_enc.rekey(newkeys);
// client and server enc/dec keys are derived differently, we need them
@@ -825,7 +824,7 @@ mod tests {
core::mem::swap(&mut algos.cipher_enc, &mut algos.cipher_dec);
core::mem::swap(&mut algos.integ_enc, &mut algos.integ_dec);
trace!("algos dec {algos:?}");
- let newkeys_b = Keys::new_from(sharedkey, &h, &sess_id, &algos).unwrap();
+ let newkeys_b = Keys::derive(sharedkey, &h, &sess_id, &algos).unwrap();
keys_dec.rekey(newkeys_b);
} else {
@@ -852,7 +851,7 @@ mod tests {
let sess_id = SessId::from_slice(&Sha256::digest(b"some sessid")).unwrap();
let sharedkey = b"hello";
let newkeys =
- Keys::new_from(sharedkey, &h, &sess_id, &algos).unwrap();
+ Keys::derive(sharedkey, &h, &sess_id, &algos).unwrap();
keys.rekey(newkeys);
trace!("algos {algos:?}");
diff --git a/src/kex.rs b/src/kex.rs
index 1b8bc63..be26b74 100644
--- a/src/kex.rs
+++ b/src/kex.rs
@@ -505,7 +505,7 @@ impl<'a> KexOutput {
let h = kex_hash.finish(k);
let sess_id = sess_id.as_ref().unwrap_or(&h);
- let keys = Keys::new_from(k, &h, &sess_id, algos)?;
+ let keys = Keys::derive(k, &h, &sess_id, algos)?;
Ok(KexOutput { h, keys })
}
diff --git a/src/namelist.rs b/src/namelist.rs
index cbf4105..52ae7cf 100644
--- a/src/namelist.rs
+++ b/src/namelist.rs
@@ -23,6 +23,7 @@ static EMPTY_LOCALNAMES: LocalNames = LocalNames::new();
/// A comma separated string, can be decoded or encoded.
/// Used for remote name lists.
+/// Wire format is described in [RFC4251](https://tools.ietf.org/html/rfc4251) SSH Architecture "name-list"
#[derive(SSHEncode, SSHDecode, Debug)]
pub struct StringNames<'a>(pub &'a AsciiStr);
diff --git a/src/packets.rs b/src/packets.rs
index 2a46875..1bd0cb4 100644
--- a/src/packets.rs
+++ b/src/packets.rs
@@ -2,6 +2,7 @@
//!
//! A [`Packet`] can be encoded/decoded to the
//! SSH Binary Packet Protocol using [`sshwire`].
+//! SSH packet format is described in [RFC4253](https://tools.ietf.org/html/rfc5643) SSH Transport
use core::borrow::BorrowMut;
use core::cell::Cell;
@@ -148,7 +149,7 @@ impl<'de: 'a, 'a> SSHDecode<'de> for Userauth60<'a> {
Some(auth::AuthType::PubKey) => Ok(Self::PkOk(SSHDecode::dec(s)?)),
_ => {
trace!("Wrong packet state for userauth60");
- return Err(WireError::PacketWrong)
+ Err(WireError::PacketWrong)
}
}
}
@@ -603,7 +604,7 @@ impl core::fmt::Display for Unknown<'_> {
pub struct ParseContext {
pub cli_auth_type: Option<auth::AuthType>,
- // Used by sign_encode()
+ // Used by auth_sig_msg()
pub method_pubkey_force_sig_bool: bool,
// Set to true if an unknown variant is encountered.
diff --git a/src/servauth.rs b/src/servauth.rs
index b0de747..34b0a4c 100644
--- a/src/servauth.rs
+++ b/src/servauth.rs
@@ -131,7 +131,7 @@ impl ServAuth {
let msg = auth::AuthSigMsg::new(&p, sess_id);
match sig_type.verify(&m.pubkey.0, &msg, sig) {
Ok(()) => true,
- Err(e) => { trace!("sig failde {e}"); false},
+ Err(e) => { trace!("sig failed {e}"); false},
}
}
diff --git a/src/sign.rs b/src/sign.rs
index f0892d2..4674996 100644
--- a/src/sign.rs
+++ b/src/sign.rs
@@ -104,7 +104,7 @@ impl SigType {
}
pub(crate) enum OwnedSig {
- // Signature doesn't let us borrow the inner bytes,
+ // salty::Signature doesn't let us borrow the inner bytes,
// so we just store raw bytes here.
Ed25519([u8; 64]),
_RSA256, // TODO
@@ -168,7 +168,7 @@ impl SignKey {
}
pub(crate) fn sign(&self, msg: &impl SSHEncode, parse_ctx: Option<&ParseContext>) -> Result<OwnedSig> {
- match self {
+ let sig: OwnedSig = match self {
SignKey::Ed25519(k) => {
k.sign_parts(|h| {
sshwire::hash_ser(h, msg, parse_ctx).map_err(|_| salty::Error::ContextTooLong)
@@ -176,7 +176,17 @@ impl SignKey {
.trap()
.map(|s| s.into())
}
+ }?;
+
+ {
+ // Validate to avoid letting errors/bitflips expose private key.
+ // Maybe this needs to be configurable for slow platforms?
+ let vsig: Signature = (&sig).into();
+ let sig_type = vsig.sig_type().unwrap();
+ sig_type.verify(&self.pubkey(), msg, &vsig)?;
}
+
+ Ok(sig)
}
}
diff --git a/src/sshwire.rs b/src/sshwire.rs
index 0c5b7fc..e6a9a48 100644
--- a/src/sshwire.rs
+++ b/src/sshwire.rs
@@ -1,6 +1,9 @@
//! SSH wire format reading/writing.
+//!
//! Used in conjunction with [`sshwire_derive`] and the [`packet`](crate::packets) format
//! definitions.
+//!
+//! SSH wire format is described in [RFC4251](https://tools.ietf.org/html/rfc4251) SSH Architecture
#[allow(unused_imports)]
use {
@@ -36,6 +39,10 @@ pub trait SSHSource<'de> {
/// Encodes the type in SSH wire format
pub trait SSHEncode {
+ /// Encode data
+ ///
+ /// The state of the `SSHSink` is undefined after an error is returned, data may
+ /// have been partially encoded.
fn enc<S>(&self, s: &mut S) -> WireResult<()> where S: SSHSink;
}
@@ -48,6 +55,10 @@ pub trait SSHEncodeEnum {
/// Decodes `struct` and `enum`s without an externally provided enum name
pub trait SSHDecode<'de>: Sized {
+ /// Decode data
+ ///
+ /// The state of the `SSHSource` is undefined after an error is returned, data may
+ /// have been partially consumed.
fn dec<S>(s: &mut S) -> WireResult<Self> where S: SSHSource<'de>;
}
@@ -169,11 +180,12 @@ struct EncodeBytes<'a> {
impl SSHSink for EncodeBytes<'_> {
fn push(&mut self, v: &[u8]) -> WireResult<()> {
- if self.target.len() - self.pos < v.len() {
+ let end = self.pos.checked_add(v.len()).ok_or(WireError::NoRoom)?;
+ if end > self.target.len() {
return Err(WireError::NoRoom);
}
- self.target[self.pos..self.pos + v.len()].copy_from_slice(v);
- self.pos += v.len();
+ self.target[self.pos..end].copy_from_slice(v);
+ self.pos = end;
Ok(())
}
}
@@ -438,7 +450,7 @@ impl SSHEncode for &[u8] {
fn enc<S>(&self, s: &mut S) -> WireResult<()>
where S: SSHSink {
// data
- s.push(&self)
+ s.push(self)
}
}
@@ -507,7 +519,7 @@ impl<'de> SSHDecode<'de> for u32 {
/// Decodes a SSH name string. Must be ASCII
/// without control characters. RFC4251 section 6.
-pub fn try_as_ascii<'a>(t: &'a [u8]) -> WireResult<&'a AsciiStr> {
+pub fn try_as_ascii(t: &[u8]) -> WireResult<&AsciiStr> {
let n = t.as_ascii_str().map_err(|_| WireError::BadName)?;
if n.chars().any(|ch| ch.is_ascii_control() || ch == AsciiChar::DEL) {
return Err(WireError::BadName);
@@ -515,7 +527,7 @@ pub fn try_as_ascii<'a>(t: &'a [u8]) -> WireResult<&'a AsciiStr> {
Ok(n)
}
-pub fn try_as_ascii_str<'a>(t: &'a [u8]) -> WireResult<&'a str> {
+pub fn try_as_ascii_str(t: &[u8]) -> WireResult<&str> {
try_as_ascii(t).map(AsciiStr::as_str)
}
--
GitLab