Skip to content

Commit

Permalink
thrussh-keys panic cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@Eugeny
Eugeny committed Mar 10, 2022
1 parent 5b7806f commit ec3caf2
Show file tree
Hide file tree
Showing 12 changed files with 161 additions and 74 deletions.
11 changes: 11 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@ A full implementation of the SSH 2 protocol, both server-side and client-side.

Thrussh is completely asynchronous, and can be combined with other protocols using [Tokio](//tokio.rs).

## Panics

`deny(clippy::panic)` except for:

* When the Rust allocator fails to allocate memory during a CryptoVec being resized.

## Unsafe code

* `cryptovec` uses `unsafe` for faster copying, initialization and binding to native API.
* `russh-libsodium` uses `unsafe` for `libsodium` bindings.

## Contributing

We welcome contributions. Currently, the main areas where we need help are:
Expand Down
6 changes: 4 additions & 2 deletions cryptovec/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,9 @@ impl CryptoVec {
}

if self.p.is_null() {
panic!("Realloc failed, pointer = {:?} {:?}", self, size)
#[allow(clippy::panic)] {
panic!("Realloc failed, pointer = {:?} {:?}", self, size)
}
} else {
self.capacity = next_capacity;
self.size = size;
Expand Down Expand Up @@ -300,7 +302,7 @@ impl CryptoVec {
/// ```
pub fn push_u32_be(&mut self, s: u32) {
let s = s.to_be();
let x: [u8; 4] = unsafe { std::mem::transmute(s) };
let x: [u8; 4] = s.to_ne_bytes();
self.extend(&x)
}

Expand Down
58 changes: 31 additions & 27 deletions thrussh-config/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::indexing_slicing, clippy::panic)]
#![deny(
clippy::unwrap_used,
clippy::expect_used,
clippy::indexing_slicing,
clippy::panic
)]
use log::debug;
use std::io::Read;
use std::net::ToSocketAddrs;
Expand All @@ -12,11 +17,10 @@ pub enum Error {
HostNotFound,
#[error("No home directory")]
NoHome,
#[error("{}", source)]
Io {
#[from]
source: std::io::Error,
},
#[error("Cannot resolve the address")]
NotResolvable,
#[error("{}", 0)]
Io(#[from] std::io::Error),
}

mod proxy;
Expand Down Expand Up @@ -53,19 +57,19 @@ impl Config {
}
}

pub async fn stream(&mut self) -> Result<Stream, std::io::Error> {
pub async fn stream(&mut self) -> Result<Stream, Error> {
self.update_proxy_command();
if let Some(ref proxy_command) = self.proxy_command {
let cmd: Vec<&str> = proxy_command.split(' ').collect();
Stream::proxy_command(cmd[0], &cmd[1..]).await
Stream::proxy_command(cmd.get(0).unwrap_or(&""), cmd.get(1..).unwrap_or(&[]))
.await
.map_err(Into::into)
} else {
Stream::tcp_connect(
&(self.host_name.as_str(), self.port)
.to_socket_addrs()?
.next()
.unwrap(),
)
.await
let address = (self.host_name.as_str(), self.port)
.to_socket_addrs()?
.next()
.ok_or(Error::NotResolvable)?;
Stream::tcp_connect(&address).await.map_err(Into::into)
}
}
}
Expand Down Expand Up @@ -130,7 +134,14 @@ pub fn parse(file: &str, host: &str) -> Result<Config, Error> {
if id.starts_with("~/") {
if let Some(mut home) = dirs_next::home_dir() {
home.push(id.split_at(2).1);
config.identity_file = Some(home.to_str().unwrap().to_string());
config.identity_file = Some(
home.to_str()
.ok_or_else(|| std::io::Error::new(
std::io::ErrorKind::Other,
"Failed to convert home directory to string",
))?
.to_string(),
);
} else {
return Err(Error::NoHome);
}
Expand All @@ -149,17 +160,10 @@ pub fn parse(file: &str, host: &str) -> Result<Config, Error> {
debug!("{:?}", key);
}
}
} else {
match lower.as_str() {
"host" => {
if value.trim_start() == host {
let mut c = Config::default(host);
c.port = 22;
config = Some(c)
}
}
_ => {}
}
} else if lower.as_str() == "host" && value.trim_start() == host {
let mut c = Config::default(host);
c.port = 22;
config = Some(c)
}
}
}
Expand Down
28 changes: 22 additions & 6 deletions thrussh-config/src/proxy.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ impl Stream {
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.args(args)
.spawn()
.unwrap(),
.spawn()?
))
}
}
Expand All @@ -40,7 +39,12 @@ impl tokio::io::AsyncRead for Stream {
buf: &mut ReadBuf,
) -> Poll<Result<(), std::io::Error>> {
match *self {
Stream::Child(ref mut c) => Pin::new(c.stdout.as_mut().unwrap()).poll_read(cx, buf),
Stream::Child(ref mut c) => {
match c.stdout.as_mut() {
Some(ref mut stdout) => Pin::new(stdout).poll_read(cx, buf),
None => Poll::Ready(Ok(()))
}
},
Stream::Tcp(ref mut t) => Pin::new(t).poll_read(cx, buf),
}
}
Expand All @@ -53,14 +57,24 @@ impl tokio::io::AsyncWrite for Stream {
buf: &[u8],
) -> Poll<Result<usize, std::io::Error>> {
match *self {
Stream::Child(ref mut c) => Pin::new(c.stdin.as_mut().unwrap()).poll_write(cx, buf),
Stream::Child(ref mut c) => {
match c.stdin.as_mut() {
Some(ref mut stdin) => Pin::new(stdin).poll_write(cx, buf),
None => Poll::Ready(Ok(0))
}
},
Stream::Tcp(ref mut t) => Pin::new(t).poll_write(cx, buf),
}
}

fn poll_flush(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<Result<(), std::io::Error>> {
match *self {
Stream::Child(ref mut c) => Pin::new(c.stdin.as_mut().unwrap()).poll_flush(cx),
Stream::Child(ref mut c) => {
match c.stdin.as_mut() {
Some(ref mut stdin) => Pin::new(stdin).poll_flush(cx),
None => Poll::Ready(Ok(()))
}
},
Stream::Tcp(ref mut t) => Pin::new(t).poll_flush(cx),
}
}
Expand All @@ -71,7 +85,9 @@ impl tokio::io::AsyncWrite for Stream {
) -> Poll<Result<(), std::io::Error>> {
match *self {
Stream::Child(ref mut c) => {
ready!(Pin::new(c.stdin.as_mut().unwrap()).poll_shutdown(cx))?;
if let Some(ref mut stdin) = c.stdin {
ready!(Pin::new(stdin).poll_shutdown(cx))?;
}
drop(c.stdin.take());
Poll::Ready(Ok(()))
}
Expand Down
30 changes: 19 additions & 11 deletions thrussh-keys/src/agent/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
}
}
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);
self.read_response().await?;
Ok(())
}
Expand All @@ -202,7 +202,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.push(msg::LOCK);
self.buf.extend_ssh_string(passphrase);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);
self.read_response().await?;
Ok(())
}
Expand All @@ -214,7 +214,8 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.push(msg::UNLOCK);
self.buf.extend_ssh_string(passphrase);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
#[allow(clippy::indexing_slicing)] // static length
BigEndian::write_u32(&mut self.buf[..], len as u32);
self.read_response().await?;
Ok(())
}
Expand All @@ -226,11 +227,13 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.resize(4);
self.buf.push(msg::REQUEST_IDENTITIES);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);

self.read_response().await?;
debug!("identities: {:?}", &self.buf[..]);
let mut keys = Vec::new();

#[allow(clippy::indexing_slicing)] // static length
if self.buf[0] == msg::IDENTITIES_ANSWER {
let mut r = self.buf.reader(1);
let n = r.read_u32()?;
Expand Down Expand Up @@ -286,13 +289,14 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
return (self, Err(e));
}

#[allow(clippy::indexing_slicing)] // length is checked
if !self.buf.is_empty() && self.buf[0] == msg::SIGN_RESPONSE {
let resp = self.write_signature(hash, &mut data);
if let Err(e) = resp {
return (self, Err(e));
}
(self, Ok(data))
} else if self.buf[0] == msg::FAILURE {
} else if self.buf.get(0) == Some(&msg::FAILURE) {
(self, Err(Error::AgentFailure))
} else {
debug!("self.buf = {:?}", &self.buf[..]);
Expand All @@ -319,7 +323,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
};
self.buf.push_u32_be(hash);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);
hash
}

Expand Down Expand Up @@ -350,6 +354,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
return (self, Err(e));
}

#[allow(clippy::indexing_slicing)] // length is checked
if !self.buf.is_empty() && self.buf[0] == msg::SIGN_RESPONSE {
let base64 = data_encoding::BASE64_NOPAD.encode(&self.buf[1..]);
(self, Ok(base64))
Expand All @@ -372,6 +377,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
if let Err(e) = self.read_response().await {
return (self, Err(e));
}
#[allow(clippy::indexing_slicing)] // length is checked
if !self.buf.is_empty() && self.buf[0] == msg::SIGN_RESPONSE {
let as_sig = |buf: &CryptoVec| -> Result<crate::signature::Signature, Error> {
let mut r = buf.reader(1);
Expand Down Expand Up @@ -415,7 +421,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.push(msg::REMOVE_IDENTITY);
key_blob(public, &mut self.buf);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);
self.read_response().await?;
Ok(())
}
Expand All @@ -428,7 +434,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.extend_ssh_string(id.as_bytes());
self.buf.extend_ssh_string(pin);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);
self.read_response().await?;
Ok(())
}
Expand All @@ -438,7 +444,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.clear();
self.buf.resize(4);
self.buf.push(msg::REMOVE_ALL_IDENTITIES);
BigEndian::write_u32(&mut self.buf[0..], 5);
BigEndian::write_u32(&mut self.buf[..], 5);
self.read_response().await?;
Ok(())
}
Expand All @@ -451,7 +457,7 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.extend_ssh_string(typ);
self.buf.extend_ssh_string(ext);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);
self.read_response().await?;
Ok(())
}
Expand All @@ -463,12 +469,13 @@ impl<S: AsyncRead + AsyncWrite + Unpin> AgentClient<S> {
self.buf.push(msg::EXTENSION);
self.buf.extend_ssh_string(typ);
let len = self.buf.len() - 4;
BigEndian::write_u32(&mut self.buf[0..], len as u32);
BigEndian::write_u32(&mut self.buf[..], len as u32);
self.read_response().await?;

let mut r = self.buf.reader(1);
ext.extend(r.read_string()?);

#[allow(clippy::indexing_slicing)] // length is checked
Ok(!self.buf.is_empty() && self.buf[0] == msg::SUCCESS)
}
}
Expand All @@ -492,6 +499,7 @@ fn key_blob(public: &key::PublicKey, buf: &mut CryptoVec) {
buf.extend_ssh_string(b"ssh-ed25519");
buf.extend_ssh_string(&p.key[0..]);
let len1 = buf.len();
#[allow(clippy::indexing_slicing)] // length is known
BigEndian::write_u32(&mut buf[5..], (len1 - len0) as u32);
}
}
Expand Down
Loading

0 comments on commit ec3caf2

Please sign in to comment.