Skip to content

Commit

Permalink
Don't open clipboard static channel when clipboard is disabled (#10348)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
zmb3 authored Feb 14, 2022
1 parent b34ba35 commit b3994e3
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 38 deletions.
1 change: 1 addition & 0 deletions lib/srv/desktop/rdp/rdpclient/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions lib/srv/desktop/rdp/rdpclient/client_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
3 changes: 2 additions & 1 deletion lib/srv/desktop/rdp/rdpclient/librdprs.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
88 changes: 55 additions & 33 deletions lib/srv/desktop/rdp/rdpclient/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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()
}
Expand All @@ -162,14 +166,19 @@ impl From<RdpError> 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<u8>,
key_der: Vec<u8>,
screen_width: u16,
screen_height: u16,
allow_clipboard: bool,
}

fn connect_rdp_inner(
go_ref: usize,
addr: &str,
params: ConnectParams,
) -> Result<Client, ConnectError> {
// Connect and authenticate.
let addr = addr
Expand All @@ -186,28 +195,29 @@ 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();
let pin = format!("{:08}", rng.gen_range(0..99999999));
sec::connect(
&mut mcs,
&domain.to_string(),
&username,
&params.username,
&pin,
true,
// InfoPasswordIsScPin means that the user will not be prompted for the smartcard PIN code,
Expand All @@ -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,
Expand All @@ -249,7 +263,8 @@ struct RdpClient<S> {
mcs: mcs::Client<S>,
global: global::Client,
rdpdr: rdpdr::Client,
cliprdr: cliprdr::Client,

cliprdr: Option<cliprdr::Client>,
}

impl<S: Read + Write> RdpClient<S> {
Expand All @@ -263,7 +278,10 @@ impl<S: Read + Write> RdpClient<S> {
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),
Expand Down Expand Up @@ -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,
}
}

Expand Down
9 changes: 5 additions & 4 deletions lib/srv/desktop/windows_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit b3994e3

Please sign in to comment.