Skip to content

Commit fc6e3ab

Browse files
mjcEugeny
mjc
authored and
Eugeny
committed
transport: reject trailing encrypted message payloads
1 parent b0961ad commit fc6e3ab

2 files changed

Lines changed: 280 additions & 29 deletions

File tree

‎russh/src/client/encrypted.rs‎

Lines changed: 53 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ use crate::cert::PublicKeyOrCertificate;
2727
use crate::client::{Handler, Msg, Prompt, Reply, Session};
2828
use crate::helpers::{AlgorithmExt, EncodedExt, NameList, sign_with_hash_alg};
2929
use crate::keys::key::parse_public_key;
30-
use crate::parsing::{ChannelOpenConfirmation, ChannelType, OpenChannelMessage};
30+
use crate::parsing::{ChannelOpenConfirmation, ChannelType, OpenChannelMessage, ensure_end};
3131
use crate::session::{Encrypted, EncryptedState, GlobalRequestResponse};
3232
use crate::{
3333
Channel, ChannelId, ChannelMsg, ChannelOpenFailure, ChannelParams, Error, MethodSet, Sig, auth,
@@ -72,6 +72,7 @@ impl Session {
7272
match buf.split_first() {
7373
Some((&msg::SERVICE_ACCEPT, mut r)) => {
7474
if map_err!(Bytes::decode(&mut r))?.as_ref() == b"ssh-userauth" {
75+
map_err!(ensure_end(&r))?;
7576
*accepted = true;
7677
if let Some(ref meth) = self.common.auth_method {
7778
let len = enc.write.len();
@@ -98,7 +99,8 @@ impl Session {
9899
EncryptedState::WaitingAuthRequest(ref mut auth_request) => {
99100
trace!("waiting auth request, {:?}", buf.first(),);
100101
match buf.split_first() {
101-
Some((&msg::USERAUTH_SUCCESS, _)) => {
102+
Some((&msg::USERAUTH_SUCCESS, r)) => {
103+
map_err!(ensure_end(&r))?;
102104
debug!("userauth_success");
103105
self.sender
104106
.send(Reply::AuthSuccess)
@@ -111,6 +113,8 @@ impl Session {
111113
}
112114
Some((&msg::USERAUTH_BANNER, mut r)) => {
113115
let banner = map_err!(String::decode(&mut r))?;
116+
let _language_tag = map_err!(String::decode(&mut r))?;
117+
map_err!(ensure_end(&r))?;
114118
client.auth_banner(&banner, self).await?;
115119
return Ok(());
116120
}
@@ -120,6 +124,7 @@ impl Session {
120124
let remaining_methods: MethodSet =
121125
(&map_err!(NameList::decode(&mut r))?).into();
122126
let partial_success = map_err!(u8::decode(&mut r))? != 0;
127+
map_err!(ensure_end(&r))?;
123128
debug!(
124129
"remaining methods {remaining_methods:?}, partial success {partial_success:?}"
125130
);
@@ -146,6 +151,9 @@ impl Session {
146151
}) = auth_request.current
147152
{
148153
debug!("userauth_pk_ok");
154+
let _algo = map_err!(String::decode(&mut r))?;
155+
let _key = map_err!(Bytes::decode(&mut r))?;
156+
map_err!(ensure_end(&r))?;
149157
*sent_pk_ok = true;
150158
} else if let Some(auth::CurrentRequest::KeyboardInteractive {
151159
..
@@ -173,6 +181,7 @@ impl Session {
173181
echo,
174182
});
175183
}
184+
map_err!(ensure_end(&r))?;
176185

177186
// send challenges to caller
178187
self.sender
@@ -324,6 +333,7 @@ impl Session {
324333
}
325334
}
326335
}
336+
ensure_end(r)?;
327337
Ok(())
328338
}
329339

@@ -351,6 +361,7 @@ impl Session {
351361
Some((&msg::CHANNEL_OPEN_CONFIRMATION, mut reader)) => {
352362
debug!("channel_open_confirmation");
353363
let msg = map_err!(ChannelOpenConfirmation::decode(&mut reader))?;
364+
map_err!(ensure_end(&reader))?;
354365
let local_id = ChannelId(msg.recipient_channel);
355366

356367
if let Some(ref mut enc) = self.common.encrypted {
@@ -389,6 +400,7 @@ impl Session {
389400
Some((&msg::CHANNEL_CLOSE, mut r)) => {
390401
debug!("channel_close");
391402
let channel_num = map_err!(ChannelId::decode(&mut r))?;
403+
map_err!(ensure_end(&r))?;
392404
if let Some(ref mut enc) = self.common.encrypted {
393405
// The CHANNEL_CLOSE message must be sent to the server at this point or the session
394406
// will not be released.
@@ -406,6 +418,7 @@ impl Session {
406418
Some((&msg::CHANNEL_EOF, mut r)) => {
407419
debug!("channel_eof");
408420
let channel_num = map_err!(ChannelId::decode(&mut r))?;
421+
map_err!(ensure_end(&r))?;
409422
if let Some(chan) = self.channels.get(&channel_num) {
410423
let _ = chan.send(ChannelMsg::Eof).await;
411424
}
@@ -418,6 +431,7 @@ impl Session {
418431
.unwrap_or(ChannelOpenFailure::Unknown);
419432
let descr = map_err!(String::decode(&mut r))?;
420433
let language = map_err!(String::decode(&mut r))?;
434+
map_err!(ensure_end(&r))?;
421435
if let Some(ref mut enc) = self.common.encrypted {
422436
enc.channels.remove(&channel_num);
423437
}
@@ -436,6 +450,7 @@ impl Session {
436450
trace!("channel_data");
437451
let channel_num = map_err!(ChannelId::decode(&mut r))?;
438452
let data = map_err!(Bytes::decode(&mut r))?;
453+
map_err!(ensure_end(&r))?;
439454
let target = self.common.config.window_size;
440455
if let Some(ref mut enc) = self.common.encrypted {
441456
if enc.adjust_window_size(channel_num, &data, target)? {
@@ -458,6 +473,7 @@ impl Session {
458473
let channel_num = map_err!(ChannelId::decode(&mut r))?;
459474
let extended_code = map_err!(u32::decode(&mut r))?;
460475
let data = map_err!(Bytes::decode(&mut r))?;
476+
map_err!(ensure_end(&r))?;
461477
let target = self.common.config.window_size;
462478
if let Some(ref mut enc) = self.common.encrypted {
463479
if enc.adjust_window_size(channel_num, &data, target)? {
@@ -490,6 +506,7 @@ impl Session {
490506
"xon-xoff" => {
491507
map_err!(u8::decode(&mut r))?; // should be 0.
492508
let client_can_do = map_err!(u8::decode(&mut r))? != 0;
509+
map_err!(ensure_end(&r))?;
493510
if let Some(chan) = self.channels.get(&channel_num) {
494511
let _ = chan.send(ChannelMsg::XonXoff { client_can_do }).await;
495512
}
@@ -498,6 +515,7 @@ impl Session {
498515
"exit-status" => {
499516
map_err!(u8::decode(&mut r))?; // should be 0.
500517
let exit_status = map_err!(u32::decode(&mut r))?;
518+
map_err!(ensure_end(&r))?;
501519
if let Some(chan) = self.channels.get(&channel_num) {
502520
let _ = chan.send(ChannelMsg::ExitStatus { exit_status }).await;
503521
}
@@ -510,6 +528,7 @@ impl Session {
510528
let core_dumped = map_err!(u8::decode(&mut r))? != 0;
511529
let error_message = map_err!(String::decode(&mut r))?;
512530
let lang_tag = map_err!(String::decode(&mut r))?;
531+
map_err!(ensure_end(&r))?;
513532
if let Some(chan) = self.channels.get(&channel_num) {
514533
let _ = chan
515534
.send(ChannelMsg::ExitSignal {
@@ -533,6 +552,7 @@ impl Session {
533552
}
534553
"keepalive@openssh.com" => {
535554
let wants_reply = map_err!(u8::decode(&mut r))?;
555+
map_err!(ensure_end(&r))?;
536556
if wants_reply == 1 {
537557
if let Some(ref mut enc) = self.common.encrypted {
538558
trace!("Received channel keep alive message: {req:?}",);
@@ -570,6 +590,7 @@ impl Session {
570590
Some((&msg::CHANNEL_WINDOW_ADJUST, mut r)) => {
571591
let channel_num = map_err!(ChannelId::decode(&mut r))?;
572592
let amount = map_err!(u32::decode(&mut r))?;
593+
map_err!(ensure_end(&r))?;
573594
let mut new_size = 0;
574595
debug!("channel_window_adjust amount: {amount:?}");
575596
if let Some(ref mut enc) = self.common.encrypted {
@@ -601,6 +622,7 @@ impl Session {
601622
let wants_reply = map_err!(u8::decode(&mut r))?;
602623
if let Some(ref mut enc) = self.common.encrypted {
603624
if req.starts_with("keepalive") {
625+
map_err!(ensure_end(&r))?;
604626
if wants_reply == 1 {
605627
trace!("Received keep alive message: {req:?}",);
606628
self.common.wants_reply = false;
@@ -610,25 +632,18 @@ impl Session {
610632
}
611633
} else if req == "hostkeys-00@openssh.com" {
612634
let mut keys = vec![];
613-
loop {
614-
match Bytes::decode(&mut r) {
615-
Ok(key) => {
616-
let key = map_err!(parse_public_key(&key));
617-
match key {
618-
Ok(key) => keys.push(key),
619-
Err(ref err) => {
620-
debug!(
621-
"failed to parse announced host key {key:?}: {err:?}",
622-
)
623-
}
624-
}
625-
}
626-
Err(ssh_encoding::Error::Length) => break,
627-
x => {
628-
map_err!(x)?;
635+
while !r.is_empty() {
636+
let key_blob = map_err!(Bytes::decode(&mut r))?;
637+
match parse_public_key(&key_blob) {
638+
Ok(key) => keys.push(key),
639+
Err(ref err) => {
640+
debug!(
641+
"failed to parse announced host key {key_blob:?}: {err:?}",
642+
)
629643
}
630644
}
631645
}
646+
map_err!(ensure_end(&r))?;
632647
return client.openssh_ext_host_keys_announced(keys, self).await;
633648
} else {
634649
warn!("Unhandled global request: {req:?} {wants_reply:?}",);
@@ -641,13 +656,15 @@ impl Session {
641656
}
642657
Some((&msg::CHANNEL_SUCCESS, mut r)) => {
643658
let channel_num = map_err!(ChannelId::decode(&mut r))?;
659+
map_err!(ensure_end(&r))?;
644660
if let Some(chan) = self.channels.get(&channel_num) {
645661
let _ = chan.send(ChannelMsg::Success).await;
646662
}
647663
client.channel_success(channel_num, self).await
648664
}
649665
Some((&msg::CHANNEL_FAILURE, mut r)) => {
650666
let channel_num = map_err!(ChannelId::decode(&mut r))?;
667+
map_err!(ensure_end(&r))?;
651668
if let Some(chan) = self.channels.get(&channel_num) {
652669
let _ = chan.send(ChannelMsg::Failure).await;
653670
}
@@ -782,12 +799,15 @@ impl Session {
782799
trace!("Global Request Success");
783800
match self.open_global_requests.pop_front() {
784801
Some(GlobalRequestResponse::Keepalive) => {
802+
map_err!(ensure_end(&r))?;
785803
// ignore keepalives
786804
}
787805
Some(GlobalRequestResponse::Ping(return_channel)) => {
806+
map_err!(ensure_end(&r))?;
788807
let _ = return_channel.send(());
789808
}
790809
Some(GlobalRequestResponse::NoMoreSessions) => {
810+
map_err!(ensure_end(&r))?;
791811
debug!("no-more-sessions@openssh.com requests success");
792812
}
793813
Some(GlobalRequestResponse::TcpIpForward(return_channel)) => {
@@ -796,7 +816,16 @@ impl Session {
796816
Some(0)
797817
} else {
798818
match u32::decode(&mut r) {
799-
Ok(port) => Some(port),
819+
Ok(port) => {
820+
if let Err(e) = ensure_end(&r) {
821+
error!(
822+
"Error parsing port for TcpIpForward request: {e:?}"
823+
);
824+
None
825+
} else {
826+
Some(port)
827+
}
828+
}
800829
Err(e) => {
801830
error!("Error parsing port for TcpIpForward request: {e:?}");
802831
None
@@ -806,12 +835,15 @@ impl Session {
806835
let _ = return_channel.send(result);
807836
}
808837
Some(GlobalRequestResponse::CancelTcpIpForward(return_channel)) => {
838+
map_err!(ensure_end(&r))?;
809839
let _ = return_channel.send(true);
810840
}
811841
Some(GlobalRequestResponse::StreamLocalForward(return_channel)) => {
842+
map_err!(ensure_end(&r))?;
812843
let _ = return_channel.send(true);
813844
}
814845
Some(GlobalRequestResponse::CancelStreamLocalForward(return_channel)) => {
846+
map_err!(ensure_end(&r))?;
815847
let _ = return_channel.send(true);
816848
}
817849
None => {
@@ -820,8 +852,9 @@ impl Session {
820852
}
821853
Ok(())
822854
}
823-
Some((&msg::REQUEST_FAILURE, _)) => {
855+
Some((&msg::REQUEST_FAILURE, r)) => {
824856
trace!("global request failure");
857+
map_err!(ensure_end(&r))?;
825858
match self.open_global_requests.pop_front() {
826859
Some(GlobalRequestResponse::Keepalive) => {
827860
// ignore keepalives

0 commit comments

Comments
 (0)