Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I Found Memory Leak Vulnerability #4866

Closed
tonix0114 opened this issue Sep 19, 2018 · 6 comments
Closed

I Found Memory Leak Vulnerability #4866

tonix0114 opened this issue Sep 19, 2018 · 6 comments

Comments

@tonix0114
Copy link

freerdp 2.0.0- rc3 has a memory leak vulnerability that can read the client's memory.
This vulnerability occurs in channels / drdynvc / client / drdynvc_main.c.
Please email me if you need more details.
tonix0114@gmail.com

@akallabeth
Copy link
Member

@tonix0114 mail sent

@tonix0114
Copy link
Author

tonix0114 commented Sep 19, 2018 via email

@tonix0114
Copy link
Author

tonix0114 commented Sep 19, 2018 via email

@tonix0114
Copy link
Author

tonix0114 commented Sep 20, 2018

Vulnerability Code : channels/drdynvc/client/drdynvc_main.c#858

static UINT drdynvc_process_capability_request(drdynvcPlugin* drdynvc, int Sp,
        int cbChId, wStream* s)
{
	UINT status;

	if (!drdynvc)
		return CHANNEL_RC_BAD_INIT_HANDLE;

	WLog_Print(drdynvc->log, WLOG_TRACE, "capability_request Sp=%d cbChId=%d", Sp, cbChId);
	Stream_Seek(s, 1); /* pad */
	Stream_Read_UINT16(s, drdynvc->version);

	/* RDP8 servers offer version 3, though Microsoft forgot to document it
	 * in their early documents.  It behaves the same as version 2.
	 */
	if ((drdynvc->version == 2) || (drdynvc->version == 3))
	{
		Stream_Read_UINT16(s, drdynvc->PriorityCharge0);
		Stream_Read_UINT16(s, drdynvc->PriorityCharge1);
		Stream_Read_UINT16(s, drdynvc->PriorityCharge2);
		Stream_Read_UINT16(s, drdynvc->PriorityCharge3);
	}

	status = drdynvc_send_capability_response(drdynvc);
	drdynvc->state = DRDYNVC_STATE_READY;
	return status;
}

Stream_Read_UINT16(s, drdynvc->version); Reads a wStream without performing a length value check.

Send To Server Code : channels/drdynvc/client/drdynvc_main.c#839

static UINT drdynvc_send_capability_response(drdynvcPlugin* drdynvc)
{
	UINT status;
	wStream* s;

	if (!drdynvc)
		return CHANNEL_RC_BAD_CHANNEL_HANDLE;

	WLog_Print(drdynvc->log, WLOG_TRACE, "capability_response");
	s = Stream_New(NULL, 4);

	if (!s)
	{
		WLog_Print(drdynvc->log, WLOG_ERROR, "Stream_Ndrdynvc_write_variable_uintew failed!");
		return CHANNEL_RC_NO_MEMORY;
	}

	Stream_Write_UINT16(s,
	                    0x0050); /* Cmd+Sp+cbChId+Pad. Note: MSTSC sends 0x005c */
	Stream_Write_UINT16(s, drdynvc->version);
	status = drdynvc_send(drdynvc, s);

	if (status != CHANNEL_RC_OK)
	{
		WLog_Print(drdynvc->log, WLOG_ERROR, "VirtualChannelWriteEx failed with %s [%08"PRIX32"]",
		           WTSErrorToString(status), status);
	}

	return status;
}

As a result, there is a two-byte outbound reading vulnerability that causes a client memory leak.

Other code like this prevents this vulnerability.

channels/cliprdr/client/cliprdr_main.c#227

	if (Stream_GetRemainingLength(s) < 4)
		return ERROR_INVALID_DATA;

	Stream_Read_UINT16(s, cCapabilitiesSets); /* cCapabilitiesSets (2 bytes) */
	Stream_Seek_UINT16(s); /* pad1 (2 bytes) */

Vulnerability Limits

When running xfreerdp on the client, you must enable the virtual channel through the / echo option.

akallabeth added a commit to akallabeth/FreeRDP that referenced this issue Sep 20, 2018
@akallabeth
Copy link
Member

@tonix0114 Thank you for this. Hope the referenced fix eliminates this entirely for this channel.

@akallabeth akallabeth added this to the 2.0.0 milestone Sep 20, 2018
mfleisz added a commit that referenced this issue Sep 21, 2018
Fix for #4866: Added additional length checks
@akallabeth
Copy link
Member

Fixed with #4871

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants