Merge pull request FreeRDP#10768 from akallabeth/stream-write-assert

[winpr,stream] assert Stream_Write_(U)INT[8|16|32] ranges

libfreerdp/core/multitransport.c

@@ -151,7 +151,11 @@ BOOL multitransport_client_send_response(rdpMultitransport* multi, UINT32 reqId,
 	}
 
 	Stream_Write_UINT32(s, reqId); /* requestId (4 bytes) */
-	Stream_Write_UINT32(s, hr);    /* HResult (4 bytes) */
+
+	/* [MS-RDPBCGR] 2.2.15.2 Client Initiate Multitransport Response PDU defines this as 4byte
+	 * UNSIGNED but https://learn.microsoft.com/en-us/windows/win32/learnwin32/error-codes-in-com
+	 * defines this as signed... assume the spec is (implicitly) assuming twos complement. */
+	Stream_Write_INT32(s, hr); /* HResult (4 bytes) */
 	return rdp_send_message_channel_pdu(multi->rdp, s, SEC_TRANSPORT_RSP);
 }
 

libfreerdp/core/orders.c

@@ -537,8 +537,25 @@ static INLINE BOOL update_read_coord(wStream* s, INT32* coord, BOOL delta)
 
 	return TRUE;
 }
-static INLINE BOOL update_write_coord(wStream* s, INT32 coord)
+
+#define update_write_coord(s, coord) \
+	update_write_coord_int((s), (coord), #coord, __FILE__, __func__, __LINE__)
+
+static INLINE BOOL update_write_coord_int(wStream* s, INT32 coord, const char* name,
+                                          const char* file, const char* fkt, size_t line)
 {
+	if ((coord < 0) || (coord > UINT16_MAX))
+	{
+		const DWORD level = WLOG_WARN;
+		wLog* log = WLog_Get(TAG);
+		if (WLog_IsLevelActive(log, level))
+		{
+			WLog_PrintMessage(log, WLOG_MESSAGE_TEXT, level, line, file, fkt,
+			                  "[%s] 0 <= %" PRId32 " <= %" PRIu16, name, coord, UINT16_MAX);
+		}
+		return FALSE;
+	}
+
 	Stream_Write_UINT16(s, coord);
 	return TRUE;
 }
@@ -1255,13 +1272,17 @@ BOOL update_write_dstblt_order(wStream* s, ORDER_INFO* orderInfo, const DSTBLT_O
 
 	orderInfo->fieldFlags = 0;
 	orderInfo->fieldFlags |= ORDER_FIELD_01;
-	update_write_coord(s, dstblt->nLeftRect);
+	if (!update_write_coord(s, dstblt->nLeftRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_02;
-	update_write_coord(s, dstblt->nTopRect);
+	if (!update_write_coord(s, dstblt->nTopRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_03;
-	update_write_coord(s, dstblt->nWidth);
+	if (!update_write_coord(s, dstblt->nWidth))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_04;
-	update_write_coord(s, dstblt->nHeight);
+	if (!update_write_coord(s, dstblt->nHeight))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_05;
 	Stream_Write_UINT8(s, dstblt->bRop);
 	return TRUE;
@@ -1296,13 +1317,17 @@ BOOL update_write_patblt_order(wStream* s, ORDER_INFO* orderInfo, PATBLT_ORDER*
 
 	orderInfo->fieldFlags = 0;
 	orderInfo->fieldFlags |= ORDER_FIELD_01;
-	update_write_coord(s, patblt->nLeftRect);
+	if (!update_write_coord(s, patblt->nLeftRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_02;
-	update_write_coord(s, patblt->nTopRect);
+	if (!update_write_coord(s, patblt->nTopRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_03;
-	update_write_coord(s, patblt->nWidth);
+	if (!update_write_coord(s, patblt->nWidth))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_04;
-	update_write_coord(s, patblt->nHeight);
+	if (!update_write_coord(s, patblt->nHeight))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_05;
 	Stream_Write_UINT8(s, patblt->bRop);
 	orderInfo->fieldFlags |= ORDER_FIELD_06;
@@ -1346,19 +1371,25 @@ BOOL update_write_scrblt_order(wStream* s, ORDER_INFO* orderInfo, const SCRBLT_O
 
 	orderInfo->fieldFlags = 0;
 	orderInfo->fieldFlags |= ORDER_FIELD_01;
-	update_write_coord(s, scrblt->nLeftRect);
+	if (!update_write_coord(s, scrblt->nLeftRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_02;
-	update_write_coord(s, scrblt->nTopRect);
+	if (!update_write_coord(s, scrblt->nTopRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_03;
-	update_write_coord(s, scrblt->nWidth);
+	if (!update_write_coord(s, scrblt->nWidth))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_04;
-	update_write_coord(s, scrblt->nHeight);
+	if (!update_write_coord(s, scrblt->nHeight))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_05;
 	Stream_Write_UINT8(s, scrblt->bRop);
 	orderInfo->fieldFlags |= ORDER_FIELD_06;
-	update_write_coord(s, scrblt->nXSrc);
+	if (!update_write_coord(s, scrblt->nXSrc))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_07;
-	update_write_coord(s, scrblt->nYSrc);
+	if (!update_write_coord(s, scrblt->nYSrc))
+		return FALSE;
 	return TRUE;
 }
 static BOOL update_read_opaque_rect_order(const char* orderName, wStream* s,
@@ -1422,13 +1453,17 @@ BOOL update_write_opaque_rect_order(wStream* s, ORDER_INFO* orderInfo,
 	// TODO: Color format conversion
 	orderInfo->fieldFlags = 0;
 	orderInfo->fieldFlags |= ORDER_FIELD_01;
-	update_write_coord(s, opaque_rect->nLeftRect);
+	if (!update_write_coord(s, opaque_rect->nLeftRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_02;
-	update_write_coord(s, opaque_rect->nTopRect);
+	if (!update_write_coord(s, opaque_rect->nTopRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_03;
-	update_write_coord(s, opaque_rect->nWidth);
+	if (!update_write_coord(s, opaque_rect->nWidth))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_04;
-	update_write_coord(s, opaque_rect->nHeight);
+	if (!update_write_coord(s, opaque_rect->nHeight))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_05;
 	byte = opaque_rect->color & 0x000000FF;
 	Stream_Write_UINT8(s, byte);
@@ -1702,13 +1737,17 @@ BOOL update_write_line_to_order(wStream* s, ORDER_INFO* orderInfo, const LINE_TO
 	orderInfo->fieldFlags |= ORDER_FIELD_01;
 	Stream_Write_UINT16(s, line_to->backMode);
 	orderInfo->fieldFlags |= ORDER_FIELD_02;
-	update_write_coord(s, line_to->nXStart);
+	if (!update_write_coord(s, line_to->nXStart))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_03;
-	update_write_coord(s, line_to->nYStart);
+	if (!update_write_coord(s, line_to->nYStart))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_04;
-	update_write_coord(s, line_to->nXEnd);
+	if (!update_write_coord(s, line_to->nXEnd))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_05;
-	update_write_coord(s, line_to->nYEnd);
+	if (!update_write_coord(s, line_to->nYEnd))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_06;
 	update_write_color(s, line_to->backColor);
 	orderInfo->fieldFlags |= ORDER_FIELD_07;
@@ -1800,19 +1839,25 @@ BOOL update_write_memblt_order(wStream* s, ORDER_INFO* orderInfo, const MEMBLT_O
 	orderInfo->fieldFlags |= ORDER_FIELD_01;
 	Stream_Write_UINT16(s, cacheId);
 	orderInfo->fieldFlags |= ORDER_FIELD_02;
-	update_write_coord(s, memblt->nLeftRect);
+	if (!update_write_coord(s, memblt->nLeftRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_03;
-	update_write_coord(s, memblt->nTopRect);
+	if (!update_write_coord(s, memblt->nTopRect))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_04;
-	update_write_coord(s, memblt->nWidth);
+	if (!update_write_coord(s, memblt->nWidth))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_05;
-	update_write_coord(s, memblt->nHeight);
+	if (!update_write_coord(s, memblt->nHeight))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_06;
 	Stream_Write_UINT8(s, memblt->bRop);
 	orderInfo->fieldFlags |= ORDER_FIELD_07;
-	update_write_coord(s, memblt->nXSrc);
+	if (!update_write_coord(s, memblt->nXSrc))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_08;
-	update_write_coord(s, memblt->nYSrc);
+	if (!update_write_coord(s, memblt->nYSrc))
+		return FALSE;
 	orderInfo->fieldFlags |= ORDER_FIELD_09;
 	Stream_Write_UINT16(s, memblt->cacheIndex);
 	return TRUE;

libfreerdp/core/timezone.c

@@ -226,10 +226,8 @@ BOOL rdp_read_client_time_zone(wStream* s, rdpSettings* settings)
 
 BOOL rdp_write_client_time_zone(wStream* s, rdpSettings* settings)
 {
-	LPTIME_ZONE_INFORMATION tz = { 0 };
-
 	WINPR_ASSERT(settings);
-	tz = settings->ClientTimeZone;
+	const LPTIME_ZONE_INFORMATION tz = settings->ClientTimeZone;
 
 	if (!tz)
 		return FALSE;
@@ -238,8 +236,12 @@ BOOL rdp_write_client_time_zone(wStream* s, rdpSettings* settings)
 	if (!Stream_EnsureRemainingCapacity(s, 4ull + sizeof(tz->StandardName)))
 		return FALSE;
 
-	/* Bias */
-	Stream_Write_UINT32(s, tz->Bias);
+	/* Bias defined in windows headers as LONG
+	 * but [MS-RDPBCGR] 2.2.1.11.1.1.1.1 Time Zone Information (TS_TIME_ZONE_INFORMATION) defines it
+	 * as unsigned.... assume the spec is buggy as an unsigned value only works on half of the
+	 * world.
+	 */
+	Stream_Write_INT32(s, tz->Bias);
 	/* standardName (64 bytes) */
 	Stream_Write(s, tz->StandardName, sizeof(tz->StandardName));
 	/* StandardDate */
@@ -250,7 +252,13 @@ BOOL rdp_write_client_time_zone(wStream* s, rdpSettings* settings)
 	/* StandardBias */
 	if (!Stream_EnsureRemainingCapacity(s, 4ull + sizeof(tz->DaylightName)))
 		return FALSE;
-	Stream_Write_UINT32(s, tz->StandardBias);
+
+	/* StandardBias defined in windows headers as LONG
+	 * but [MS-RDPBCGR] 2.2.1.11.1.1.1.1 Time Zone Information (TS_TIME_ZONE_INFORMATION) defines it
+	 * as unsigned.... assume the spec is buggy as an unsigned value only works on half of the
+	 * world.
+	 */
+	Stream_Write_INT32(s, tz->StandardBias);
 
 	/* daylightName (64 bytes) */
 	Stream_Write(s, tz->DaylightName, sizeof(tz->DaylightName));
@@ -261,7 +269,13 @@ BOOL rdp_write_client_time_zone(wStream* s, rdpSettings* settings)
 	/* DaylightBias */
 	if (!Stream_EnsureRemainingCapacity(s, 4ull))
 		return FALSE;
-	Stream_Write_UINT32(s, tz->DaylightBias);
+
+	/* DaylightBias defined in windows headers as LONG
+	 * but [MS-RDPBCGR] 2.2.1.11.1.1.1.1 Time Zone Information (TS_TIME_ZONE_INFORMATION) defines it
+	 * as unsigned.... assume the spec is buggy as an unsigned value only works on half of the
+	 * world.
+	 */
+	Stream_Write_INT32(s, tz->DaylightBias);
 
 	return TRUE;
 }