From b3994e3cb6c91ffa87943f6bda9919101e3b4898 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Mon, 14 Feb 2022 14:27:09 -0700 Subject: [PATCH] Don't open clipboard static channel when clipboard is disabled (#10348) * Don't open clipboard static channel when clipboard is disabled Prior to this change, we relied only on the frontend to check whether or not RBAC allows access to the shared clipboard feature. Add a new config parameter, which is passed all the way down do Rust. If the clipboard is not enabled, we will never open the CLIPRDR channel and no clipboard data will be exchanged with the RDP server. This is both more secure and more efficient. We also take care to ensure that clipboard actions that are attempted when the clipboard is disabled is treated as a no-op rather than an error. This ensures that a misbehaving client can't cause us to kill the connection by sending clipboard messages when the shared clipboard is disabled. * Sorry, Clippy. --- lib/srv/desktop/rdp/rdpclient/client.go | 1 + .../desktop/rdp/rdpclient/client_common.go | 4 + lib/srv/desktop/rdp/rdpclient/librdprs.h | 3 +- lib/srv/desktop/rdp/rdpclient/src/lib.rs | 88 ++++++++++++------- lib/srv/desktop/windows_server.go | 9 +- 5 files changed, 67 insertions(+), 38 deletions(-) diff --git a/lib/srv/desktop/rdp/rdpclient/client.go b/lib/srv/desktop/rdp/rdpclient/client.go index 9150d13072cbd..a74d6695b78b8 100644 --- a/lib/srv/desktop/rdp/rdpclient/client.go +++ b/lib/srv/desktop/rdp/rdpclient/client.go @@ -224,6 +224,7 @@ func (c *Client) connect(ctx context.Context) error { // screen size. C.uint16_t(c.clientWidth), C.uint16_t(c.clientHeight), + C.bool(c.cfg.AllowClipboard), ) if err := cgoError(res.err); err != nil { return trace.Wrap(err) diff --git a/lib/srv/desktop/rdp/rdpclient/client_common.go b/lib/srv/desktop/rdp/rdpclient/client_common.go index c424717bc3b6f..9ad356d094361 100644 --- a/lib/srv/desktop/rdp/rdpclient/client_common.go +++ b/lib/srv/desktop/rdp/rdpclient/client_common.go @@ -45,6 +45,10 @@ type Config struct { // Encoder is an optional override for PNG encoding. Encoder *png.Encoder + // AllowClipboard indicates whether the RDP connection should enable + // clipboard sharing. + AllowClipboard bool + // Log is the logger for status messages. Log logrus.FieldLogger } diff --git a/lib/srv/desktop/rdp/rdpclient/librdprs.h b/lib/srv/desktop/rdp/rdpclient/librdprs.h index f6460f2148607..f1a2876cdc078 100644 --- a/lib/srv/desktop/rdp/rdpclient/librdprs.h +++ b/lib/srv/desktop/rdp/rdpclient/librdprs.h @@ -102,7 +102,8 @@ struct ClientOrError connect_rdp(uintptr_t go_ref, uint32_t key_der_len, uint8_t *key_der, uint16_t screen_width, - uint16_t screen_height); + uint16_t screen_height, + bool allow_clipboard); /** * `update_clipboard` is called from Go, and caches data that was copied diff --git a/lib/srv/desktop/rdp/rdpclient/src/lib.rs b/lib/srv/desktop/rdp/rdpclient/src/lib.rs index 64a607b75bc6f..332174ee43111 100644 --- a/lib/srv/desktop/rdp/rdpclient/src/lib.rs +++ b/lib/srv/desktop/rdp/rdpclient/src/lib.rs @@ -122,6 +122,7 @@ pub unsafe extern "C" fn connect_rdp( key_der: *mut u8, screen_width: u16, screen_height: u16, + allow_clipboard: bool, ) -> ClientOrError { // Convert from C to Rust types. let addr = from_go_string(go_addr); @@ -132,11 +133,14 @@ pub unsafe extern "C" fn connect_rdp( connect_rdp_inner( go_ref, &addr, - username, - cert_der, - key_der, - screen_width, - screen_height, + ConnectParams { + username, + cert_der, + key_der, + screen_width, + screen_height, + allow_clipboard, + }, ) .into() } @@ -162,14 +166,19 @@ impl From for ConnectError { const RDP_CONNECT_TIMEOUT: time::Duration = time::Duration::from_secs(5); -fn connect_rdp_inner( - go_ref: usize, - addr: &str, +struct ConnectParams { username: String, cert_der: Vec, key_der: Vec, screen_width: u16, screen_height: u16, + allow_clipboard: bool, +} + +fn connect_rdp_inner( + go_ref: usize, + addr: &str, + params: ConnectParams, ) -> Result { // Connect and authenticate. let addr = addr @@ -186,20 +195,21 @@ fn connect_rdp_inner( let protocols = x224::Protocols::ProtocolSSL as u32 | x224::Protocols::ProtocolRDP as u32; let x224 = x224::Client::connect(tpkt::Client::new(tcp), protocols, false, None, false, false)?; let mut mcs = mcs::Client::new(x224); + + // request the static channels we'll need: + // rdpdr: derive redirection (smart cards) + // rdpsnd: sound (for some reason we need to request this) + // cliprdr: clipboard + let mut static_channels = vec![rdpdr::CHANNEL_NAME.to_string(), "rdpsnd".to_string()]; + if params.allow_clipboard { + static_channels.push(cliprdr::CHANNEL_NAME.to_string()) + } mcs.connect( "rdp-rs".to_string(), - screen_width, - screen_height, + params.screen_width, + params.screen_height, KeyboardLayout::US, - // request the static channels we'll need: - // rdpdr: derive redirection (smart cards) - // rdpsnd: sound (for some reason we need to request this) - // cliprdr: clipboard - &[ - rdpdr::CHANNEL_NAME.to_string(), - "rdpsnd".to_string(), - cliprdr::CHANNEL_NAME.to_string(), - ], + &static_channels, )?; // Generate a random 8-digit PIN for our smartcard. let mut rng = rand_chacha::ChaCha20Rng::from_entropy(); @@ -207,7 +217,7 @@ fn connect_rdp_inner( sec::connect( &mut mcs, &domain.to_string(), - &username, + ¶ms.username, &pin, true, // InfoPasswordIsScPin means that the user will not be prompted for the smartcard PIN code, @@ -218,18 +228,22 @@ fn connect_rdp_inner( let global = global::Client::new( mcs.get_user_id(), mcs.get_global_channel_id(), - screen_width, - screen_height, + params.screen_width, + params.screen_height, KeyboardLayout::US, "rdp-rs", ); // Client for the "rdpdr" channel - smartcard emulation. - let rdpdr = rdpdr::Client::new(cert_der, key_der, pin); + let rdpdr = rdpdr::Client::new(params.cert_der, params.key_der, pin); // Client for the "cliprdr" channel - clipboard sharing. - let cliprdr: cliprdr::Client = cliprdr::Client::new(Box::new(move |v| unsafe { - handle_remote_copy(go_ref, v.as_ptr() as _, v.len() as u32); - })); + let cliprdr = if params.allow_clipboard { + Some(cliprdr::Client::new(Box::new(move |v| unsafe { + handle_remote_copy(go_ref, v.as_ptr() as _, v.len() as u32); + }))) + } else { + None + }; let rdp_client = RdpClient { mcs, @@ -249,7 +263,8 @@ struct RdpClient { mcs: mcs::Client, global: global::Client, rdpdr: rdpdr::Client, - cliprdr: cliprdr::Client, + + cliprdr: Option, } impl RdpClient { @@ -263,7 +278,10 @@ impl RdpClient { match channel_name.as_str() { "global" => self.global.read(message, &mut self.mcs, callback), rdpdr::CHANNEL_NAME => self.rdpdr.read(message, &mut self.mcs), - cliprdr::CHANNEL_NAME => self.cliprdr.read(message, &mut self.mcs), + cliprdr::CHANNEL_NAME => match self.cliprdr { + Some(ref mut clip) => clip.read(message, &mut self.mcs), + None => Ok(()), + }, _ => Err(RdpError::RdpError(RdpProtocolError::new( RdpErrorKind::UnexpectedType, &format!("Invalid channel name {:?}", channel_name), @@ -383,12 +401,16 @@ pub unsafe extern "C" fn update_clipboard( }; let data = from_go_array(len, data); let mut lock = client.rdp_client.lock().unwrap(); - match lock.cliprdr.update_clipboard(data) { - Ok(message) => match lock.mcs.write(&cliprdr::CHANNEL_NAME.to_string(), message) { - Ok(()) => CGO_OK, - Err(e) => to_cgo_error(format!("failed writing cliprdr format list: {:?}", e)), + + match lock.cliprdr { + Some(ref mut clip) => match clip.update_clipboard(data) { + Ok(message) => match lock.mcs.write(&cliprdr::CHANNEL_NAME.to_string(), message) { + Ok(()) => CGO_OK, + Err(e) => to_cgo_error(format!("failed writing cliprdr format list: {:?}", e)), + }, + Err(e) => to_cgo_error(format!("failed updating clipboard: {:?}", e)), }, - Err(e) => to_cgo_error(format!("failed updating clipboard: {:?}", e)), + None => CGO_OK, } } diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index b1939d015bafd..cd96dccdf37e5 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -811,10 +811,11 @@ func (s *WindowsService) connectRDP(ctx context.Context, log logrus.FieldLogger, GenerateUserCert: func(ctx context.Context, username string, ttl time.Duration) (certDER, keyDER []byte, err error) { return s.generateCredentials(ctx, username, desktop.GetDomain(), ttl) }, - CertTTL: windowsDesktopCertTTL, - Addr: desktop.GetAddr(), - Conn: tdpConn, - AuthorizeFn: authorize, + CertTTL: windowsDesktopCertTTL, + Addr: desktop.GetAddr(), + Conn: tdpConn, + AuthorizeFn: authorize, + AllowClipboard: authCtx.Checker.DesktopClipboard(), }) if err != nil { s.onSessionStart(ctx, &identity, sessionStartTime, windowsUser, string(sessionID), desktop, err)