From a4580923e77b4690a5fe90a8e37a042bc5a8fa06 Mon Sep 17 00:00:00 2001 From: Norbert Federa Date: Tue, 20 Oct 2015 21:28:29 +0200 Subject: [PATCH] xfreerdp/clipr: fix self owned test and hardening - xf_cliprdr_is_self_owned() lied if multiple xfreerdp instances were running. - fixed a few unchecked callocs - added/modified and handled some return values in compliance with the new hardened channel api --- client/X11/xf_client.c | 4 +- client/X11/xf_cliprdr.c | 172 ++++++++++++++++++++++------------------ 2 files changed, 100 insertions(+), 76 deletions(-) diff --git a/client/X11/xf_client.c b/client/X11/xf_client.c index a788ab2b1..b45f18c35 100644 --- a/client/X11/xf_client.c +++ b/client/X11/xf_client.c @@ -1177,7 +1177,9 @@ BOOL xf_post_connect(freerdp* instance) update->PlaySound = xf_play_sound; update->SetKeyboardIndicators = xf_keyboard_set_indicators; - xfc->clipboard = xf_clipboard_new(xfc); + if (!(xfc->clipboard = xf_clipboard_new(xfc))) + return FALSE; + if (freerdp_channels_post_connect(channels, instance) < 0) return FALSE; diff --git a/client/X11/xf_cliprdr.c b/client/X11/xf_cliprdr.c index ea38b8ac9..c474e743e 100644 --- a/client/X11/xf_cliprdr.c +++ b/client/X11/xf_cliprdr.c @@ -63,7 +63,6 @@ struct xf_clipboard Window root_window; Atom clipboard_atom; Atom property_atom; - Atom identity_atom; int numClientFormats; xfCliprdrFormat clientFormats[20]; @@ -97,7 +96,7 @@ struct xf_clipboard BOOL xfixes_supported; }; -int xf_cliprdr_send_client_format_list(xfClipboard* clipboard); +UINT xf_cliprdr_send_client_format_list(xfClipboard* clipboard); static void xf_cliprdr_check_owner(xfClipboard* clipboard) { @@ -118,36 +117,9 @@ static void xf_cliprdr_check_owner(xfClipboard* clipboard) static BOOL xf_cliprdr_is_self_owned(xfClipboard* clipboard) { - Atom type; - UINT32 id = 0; - UINT32* pid = NULL; - int format, result = 0; - unsigned long length; - unsigned long bytes_left; xfContext* xfc = clipboard->xfc; - clipboard->owner = XGetSelectionOwner(xfc->display, clipboard->clipboard_atom); - - if (clipboard->owner != None) - { - result = XGetWindowProperty(xfc->display, clipboard->owner, - clipboard->identity_atom, 0, 4, 0, XA_INTEGER, - &type, &format, &length, &bytes_left, (BYTE**) &pid); - } - - if (pid) - { - id = *pid; - XFree(pid); - } - - if ((clipboard->owner == None) || (clipboard->owner == xfc->drawable)) - return FALSE; - - if (result != Success) - return FALSE; - - return (id ? TRUE : FALSE); + return XGetSelectionOwner(xfc->display, clipboard->clipboard_atom) == xfc->drawable; } static xfCliprdrFormat* xf_cliprdr_get_format_by_id(xfClipboard* clipboard, UINT32 formatId) @@ -191,7 +163,12 @@ static xfCliprdrFormat* xf_cliprdr_get_format_by_atom(xfClipboard* clipboard, At return NULL; } -static void xf_cliprdr_send_data_request(xfClipboard* clipboard, UINT32 formatId) +/** + * Function description + * + * @return 0 on success, otherwise a Win32 error code + */ +static UINT xf_cliprdr_send_data_request(xfClipboard* clipboard, UINT32 formatId) { CLIPRDR_FORMAT_DATA_REQUEST request; @@ -199,10 +176,15 @@ static void xf_cliprdr_send_data_request(xfClipboard* clipboard, UINT32 formatId request.requestedFormatId = formatId; - clipboard->context->ClientFormatDataRequest(clipboard->context, &request); + return clipboard->context->ClientFormatDataRequest(clipboard->context, &request); } -static void xf_cliprdr_send_data_response(xfClipboard* clipboard, BYTE* data, int size) +/** + * Function description + * + * @return 0 on success, otherwise a Win32 error code + */ +static UINT xf_cliprdr_send_data_response(xfClipboard* clipboard, BYTE* data, int size) { CLIPRDR_FORMAT_DATA_RESPONSE response; @@ -212,7 +194,7 @@ static void xf_cliprdr_send_data_response(xfClipboard* clipboard, BYTE* data, in response.dataLen = size; response.requestedFormatData = data; - clipboard->context->ClientFormatDataResponse(clipboard->context, &response); + return clipboard->context->ClientFormatDataResponse(clipboard->context, &response); } static void xf_cliprdr_get_requested_targets(xfClipboard* clipboard) @@ -236,7 +218,18 @@ static void xf_cliprdr_get_requested_targets(xfClipboard* clipboard) 0, 200, 0, XA_ATOM, &atom, &format_property, &length, &bytes_left, &data); if (length > 0) - formats = (CLIPRDR_FORMAT*) calloc(length, sizeof(CLIPRDR_FORMAT)); + { + if (!data) + { + WLog_ERR(TAG, "XGetWindowProperty set length = %d but data is NULL", length); + goto out; + } + if (!(formats = (CLIPRDR_FORMAT*) calloc(length, sizeof(CLIPRDR_FORMAT)))) + { + WLog_ERR(TAG, "failed to allocate %d CLIPRDR_FORMAT structs", length); + goto out; + } + } for (i = 0; i < length; i++) { @@ -252,8 +245,6 @@ static void xf_cliprdr_get_requested_targets(xfClipboard* clipboard) } } - XFree(data); - ZeroMemory(&formatList, sizeof(CLIPRDR_FORMAT_LIST)); formatList.msgFlags = CB_RESPONSE_OK; @@ -262,6 +253,9 @@ static void xf_cliprdr_get_requested_targets(xfClipboard* clipboard) clipboard->context->ClientFormatList(clipboard->context, &formatList); +out: + if (data) + XFree(data); free(formats); } @@ -515,7 +509,11 @@ static BOOL xf_cliprdr_process_selection_request(xfClipboard* clipboard, XEvent* delayRespond = FALSE; - respond = (XEvent*) calloc(1, sizeof(XEvent)); + if (!(respond = (XEvent*) calloc(1, sizeof(XEvent)))) + { + WLog_ERR(TAG, "failed to allocate XEvent data"); + return FALSE; + } respond->xselection.property = None; respond->xselection.type = SelectionNotify; @@ -702,7 +700,12 @@ void xf_cliprdr_handle_xevent(xfContext* xfc, XEvent* event) } } -int xf_cliprdr_send_client_capabilities(xfClipboard* clipboard) +/** + * Function description + * + * @return 0 on success, otherwise a Win32 error code + */ +UINT xf_cliprdr_send_client_capabilities(xfClipboard* clipboard) { CLIPRDR_CAPABILITIES capabilities; CLIPRDR_GENERAL_CAPABILITY_SET generalCapabilitySet; @@ -716,22 +719,33 @@ int xf_cliprdr_send_client_capabilities(xfClipboard* clipboard) generalCapabilitySet.version = CB_CAPS_VERSION_2; generalCapabilitySet.generalFlags = CB_USE_LONG_FORMAT_NAMES; - clipboard->context->ClientCapabilities(clipboard->context, &capabilities); - - return 1; + return clipboard->context->ClientCapabilities(clipboard->context, &capabilities); } -int xf_cliprdr_send_client_format_list(xfClipboard* clipboard) +/** + * Function description + * + * @return 0 on success, otherwise a Win32 error code + */ +UINT xf_cliprdr_send_client_format_list(xfClipboard* clipboard) { UINT32 i, numFormats; - CLIPRDR_FORMAT* formats; + CLIPRDR_FORMAT* formats = NULL; CLIPRDR_FORMAT_LIST formatList; xfContext* xfc = clipboard->xfc; + UINT ret; ZeroMemory(&formatList, sizeof(CLIPRDR_FORMAT_LIST)); numFormats = clipboard->numClientFormats; - formats = (CLIPRDR_FORMAT*) calloc(numFormats, sizeof(CLIPRDR_FORMAT)); + if (numFormats) + { + if (!(formats = (CLIPRDR_FORMAT*) calloc(numFormats, sizeof(CLIPRDR_FORMAT)))) + { + WLog_ERR(TAG, "failed to allocate %d CLIPRDR_FORMAT structs", numFormats); + return CHANNEL_RC_NO_MEMORY; + } + } for (i = 0; i < numFormats; i++) { @@ -743,7 +757,7 @@ int xf_cliprdr_send_client_format_list(xfClipboard* clipboard) formatList.numFormats = numFormats; formatList.formats = formats; - clipboard->context->ClientFormatList(clipboard->context, &formatList); + ret = clipboard->context->ClientFormatList(clipboard->context, &formatList); free(formats); @@ -754,10 +768,15 @@ int xf_cliprdr_send_client_format_list(xfClipboard* clipboard) clipboard->targets[1], clipboard->property_atom, xfc->drawable, CurrentTime); } - return 1; + return ret; } -int xf_cliprdr_send_client_format_list_response(xfClipboard* clipboard, BOOL status) +/** + * Function description + * + * @return 0 on success, otherwise a Win32 error code + */ +UINT xf_cliprdr_send_client_format_list_response(xfClipboard* clipboard, BOOL status) { CLIPRDR_FORMAT_LIST_RESPONSE formatListResponse; @@ -765,11 +784,14 @@ int xf_cliprdr_send_client_format_list_response(xfClipboard* clipboard, BOOL sta formatListResponse.msgFlags = status ? CB_RESPONSE_OK : CB_RESPONSE_FAIL; formatListResponse.dataLen = 0; - clipboard->context->ClientFormatListResponse(clipboard->context, &formatListResponse); - - return 1; + return clipboard->context->ClientFormatListResponse(clipboard->context, &formatListResponse); } +/** + * Function description + * + * @return 0 on success, otherwise a Win32 error code + */ int xf_cliprdr_send_client_format_data_request(xfClipboard* clipboard, UINT32 formatId) { CLIPRDR_FORMAT_DATA_REQUEST formatDataRequest; @@ -780,9 +802,7 @@ int xf_cliprdr_send_client_format_data_request(xfClipboard* clipboard, UINT32 fo formatDataRequest.requestedFormatId = formatId; clipboard->requestedFormatId = formatId; - clipboard->context->ClientFormatDataRequest(clipboard->context, &formatDataRequest); - - return 1; + return clipboard->context->ClientFormatDataRequest(clipboard->context, &formatDataRequest); } /** @@ -793,9 +813,13 @@ int xf_cliprdr_send_client_format_data_request(xfClipboard* clipboard, UINT32 fo static UINT xf_cliprdr_monitor_ready(CliprdrClientContext* context, CLIPRDR_MONITOR_READY* monitorReady) { xfClipboard* clipboard = (xfClipboard*) context->custom; + UINT ret; + + if ((ret = xf_cliprdr_send_client_capabilities(clipboard)) != CHANNEL_RC_OK) + return ret; + if ((ret = xf_cliprdr_send_client_format_list(clipboard)) != CHANNEL_RC_OK) + return ret; - xf_cliprdr_send_client_capabilities(clipboard); - xf_cliprdr_send_client_format_list(clipboard); clipboard->sync = TRUE; return CHANNEL_RC_OK; @@ -824,6 +848,7 @@ static UINT xf_cliprdr_server_format_list(CliprdrClientContext* context, CLIPRDR CLIPRDR_FORMAT* format; xfClipboard* clipboard = (xfClipboard*) context->custom; xfContext* xfc = clipboard->xfc; + UINT ret; if (clipboard->data) { @@ -843,10 +868,14 @@ static UINT xf_cliprdr_server_format_list(CliprdrClientContext* context, CLIPRDR } clipboard->numServerFormats = formatList->numFormats; - clipboard->serverFormats = (CLIPRDR_FORMAT*) calloc(clipboard->numServerFormats, sizeof(CLIPRDR_FORMAT)); - if (!clipboard->serverFormats) - return CHANNEL_RC_NO_MEMORY; + if (clipboard->numServerFormats) + { + if (!(clipboard->serverFormats = (CLIPRDR_FORMAT*) calloc(clipboard->numServerFormats, sizeof(CLIPRDR_FORMAT)))) { + WLog_ERR(TAG, "failed to allocate %d CLIPRDR_FORMAT structs", clipboard->numServerFormats); + return CHANNEL_RC_NO_MEMORY; + } + } for (i = 0; i < formatList->numFormats; i++) { @@ -863,7 +892,7 @@ static UINT xf_cliprdr_server_format_list(CliprdrClientContext* context, CLIPRDR clipboard->numServerFormats = 0; free(clipboard->serverFormats); clipboard->serverFormats = NULL; - return -1; + return CHANNEL_RC_NO_MEMORY; } } } @@ -883,13 +912,13 @@ static UINT xf_cliprdr_server_format_list(CliprdrClientContext* context, CLIPRDR } } - xf_cliprdr_send_client_format_list_response(clipboard, TRUE); + ret = xf_cliprdr_send_client_format_list_response(clipboard, TRUE); XSetSelectionOwner(xfc->display, clipboard->clipboard_atom, xfc->drawable, CurrentTime); XFlush(xfc->display); - return CHANNEL_RC_OK; + return ret; } /** @@ -924,15 +953,10 @@ static UINT xf_cliprdr_server_format_data_request(CliprdrClientContext* context, XA_INTEGER, 32, PropModeReplace, (BYTE*) &formatId, 1); } else - { format = xf_cliprdr_get_format_by_id(clipboard, formatId); - } if (!format) - { - xf_cliprdr_send_data_response(clipboard, NULL, 0); - return CHANNEL_RC_OK; - } + return xf_cliprdr_send_data_response(clipboard, NULL, 0); clipboard->requestedFormatId = formatId; @@ -1055,11 +1079,14 @@ static UINT xf_cliprdr_server_format_data_response(CliprdrClientContext* context xfClipboard* xf_clipboard_new(xfContext* xfc) { int n; - UINT32 id; rdpChannels* channels; xfClipboard* clipboard; - clipboard = (xfClipboard*) calloc(1, sizeof(xfClipboard)); + if (!(clipboard = (xfClipboard*) calloc(1, sizeof(xfClipboard)))) + { + WLog_ERR(TAG, "failed to allocate xfClipboard data"); + return NULL; + } xfc->clipboard = clipboard; @@ -1082,12 +1109,7 @@ xfClipboard* xf_clipboard_new(xfContext* xfc) return NULL; } - id = 1; clipboard->property_atom = XInternAtom(xfc->display, "_FREERDP_CLIPRDR", FALSE); - clipboard->identity_atom = XInternAtom(xfc->display, "_FREERDP_CLIPRDR_ID", FALSE); - - XChangeProperty(xfc->display, xfc->drawable, clipboard->identity_atom, - XA_INTEGER, 32, PropModeReplace, (BYTE*) &id, 1); XSelectInput(xfc->display, clipboard->root_window, PropertyChangeMask);