Hi Ilja, Those are all very valid points that we are aware of. Historically, a lot of code in FreeRDP wasn't written without strict safety standards. One of our users, "hardening", has been working hard on hardening the FreeRDP code base to bring it up to strict safety standards. Other users have been helping fixing such errors, such as diget (bmiklautz) and myself. Whenever I get to work on some older code I usually spend time fixing common problems like malloc checks, out of bounds parsing, etc. It takes a lot of time to go over everything and these types of changes need to be done very carefully and tested because they can often break existing features.
As for changing the stream utils to functions instead of macros and putting safety checks there, this has been discussed in the past and it is not something we wish to do. It may look like an easy one-size-fits-all solution, but what really needs to be done is intelligent parsing and checking. Changing the stream utils from macros to functions + add safety checks for every single variable read/write would really impact performance without really adding much more safety, as the real safety comes with the intelligent checks. For instance, it is better to check that 20 bytes are available before reading 20 bytes of different variables rather than check individually for each variable within those 20 bytes. Most of the time, a few checks are necessary to ensure that enough data is available to read before doing the reads. As for the safeint stuff, maybe it would be worth implementing in WinPR. I haven't put much thought into preventing integer overflows. Best regards, -Marc-Andre On Sat, Mar 7, 2015 at 1:06 PM, Ilja Van Sprundel <ivansprun...@ioactive.com > wrote: > Hey, > > I spend some time reading FreeRDP code this week. It's a really cool > project. However, I found a number of security issues I think you should > take a look at. > > Below is a sample of them. There are seemingly many more, too many I have > time to look for right now. I understand that handling RDP doesn't easily > lend itself to safe parsing, and that actual functionality is the focus, > but given the success of FreeRDP, it would make sense to give some > attention to implementation security. I think there's a number of > relatively easy things you could do to improve it: > > > > - run Cppcheck over the code (http://cppcheck.sourceforge.net/) and fix > issues identified. > > - since FreeRDP can be compiled with visual studio, run vs code analysis > over the code, and fix those issues. > > - add boundchecks to the stream_*() APIs (turning the macros into > functions maybe?). This would probably fix a ton of out of bound read > issues. > > - a lot of calls to malloc()/calloc()/realloc() are in dire need of return > value checks. > > - a lot of calls to realloc() can cause memory leaks (ptr = realloc(ptr, > ...) leaks ptr in case realloc return NULL). these should get fixed. > > - there's quite a few integer overflows. Using or creating something like > safeint ( > https://msdn.microsoft.com/en-us/library/windows/desktop/ff521693%28v=vs.85%29.aspx) > could be applied to fix a bunch of them in a systemic manner. > > > > Bugs identified: > > > ------------------------------------------------------------------------------------------------------ > > Integer overflow (leading to memory corruption), also out of bound read > > > > freerdp\freerdp\channels\audin\server\Audin.c > > static BOOL audin_server_recv_formats(audin_server* audin, wStream* s, > UINT32 length) > > { > > int i; > > > > if (length < 8) > > return FALSE; > > > > Stream_Read_UINT32(s, audin->context.num_client_formats); > /* NumFormats (4 bytes) */ > > Stream_Seek_UINT32(s); /* cbSizeFormatsPacket (4 bytes) */ > > length -= 8; > > > > if (audin->context.num_client_formats <= 0) > > return FALSE; > > > > audin->context.client_formats = > malloc(audin->context.num_client_formats * sizeof(AUDIO_FORMAT)); <-- > integer overflow, can cause buffer overflow in for loop > > ZeroMemory(audin->context.client_formats, > audin->context.num_client_formats * sizeof(AUDIO_FORMAT)); > > > > for (i = 0; i < audin->context.num_client_formats; i++) > > { > > if (length < 18) <-- not adjusting length > for every iteration! can cause out of bound read > > { > > > free(audin->context.client_formats); > > > audin->context.client_formats = NULL; > > return FALSE; > > } > > > > Stream_Read_UINT16(s, > audin->context.client_formats[i].wFormatTag); > > Stream_Read_UINT16(s, > audin->context.client_formats[i].nChannels); > > Stream_Read_UINT32(s, > audin->context.client_formats[i].nSamplesPerSec); > > Stream_Seek_UINT32(s); /* nAvgBytesPerSec > */ > > Stream_Read_UINT16(s, > audin->context.client_formats[i].nBlockAlign); > > Stream_Read_UINT16(s, > audin->context.client_formats[i].wBitsPerSample); > > Stream_Read_UINT16(s, > audin->context.client_formats[i].cbSize); > > > > if > (audin->context.client_formats[i].cbSize > 0) > > { > > Stream_Seek(s, > audin->context.client_formats[i].cbSize); > > } > > } > > > > IFCALL(audin->context.Opening, &audin->context); > > > > return TRUE; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > unchecked return value => NULL deref > > > > > > freerdp\freerdp\libfreerdp\core\Server.c > > BOOL WINAPI FreeRDP_WTSVirtualChannelWrite(HANDLE hChannelHandle, PCHAR > Buffer, ULONG Length, PULONG pBytesWritten) > > { > > wStream* s; > > ... > > s = Stream_New(NULL, > channel->client->settings->VirtualChannelChunkSize); <-- need to check for > NULL > > buffer = Stream_Buffer(s); > <-- NULL deref > > > > ... > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > out of bound read in rdp_read_bitmap_codecs_capability_set() > > > > freerdp\freerdp\libfreerdp\core\Capabilities.c > > BOOL rdp_read_bitmap_codecs_capability_set(wStream* s, UINT16 length, > rdpSettings* settings) > > { > > ... > > > while (numIcaps--) > > > { > > > UINT16 version; > > > UINT16 tileSize; > > > BYTE codecFlags; > > > BYTE colConvBits; > > > BYTE transformBits; > > > BYTE entropyBits; > > > > > /* TS_RFX_ICAP */ > > > > > Stream_Read_UINT16(s, version); /* version (2 bytes) */ > > > Stream_Read_UINT16(s, tileSize); /* tileSize (2 bytes) */ > > > Stream_Read_UINT8(s, codecFlags); /* flags (1 byte) */ > > > Stream_Read_UINT8(s, colConvBits); /* colConvBits (1 byte) */ > > > Stream_Read_UINT8(s, transformBits); /* transformBits (1 byte) */ > > > Stream_Read_UINT8(s, entropyBits); /* entropyBits (1 byte) */ > > > > > if (version != 0x0100) > > > return FALSE; > > > > > if (tileSize != 0x0040) > > > return FALSE; > > > > > if (colConvBits != 1) > > > return FALSE; > > > > > if (transformBits != 1) > > > return FALSE; > > > } > > ... > > } > > > > > ----------------------------------------------------------------------------------------------------- > > > > integer underflow and overflow => out of bound read > > > > freerdp\freerdp\libfreerdp\core\Certificate.c > > static BOOL certificate_process_server_public_key(rdpCertificate* > certificate, wStream* s, UINT32 length) > > { > > BYTE magic[4]; > > UINT32 keylen; > > UINT32 bitlen; > > UINT32 datalen; > > UINT32 modlen; > > > > if (Stream_GetRemainingLength(s) < 20) > > return FALSE; > > > > Stream_Read(s, magic, 4); > > > > if (memcmp(magic, "RSA1", 4) != 0) > > { > > WLog_ERR(TAG, "magic error"); > > return FALSE; > > } > > > > Stream_Read_UINT32(s, keylen); <-- small keylen > > Stream_Read_UINT32(s, bitlen); > > Stream_Read_UINT32(s, datalen); > > Stream_Read(s, certificate->cert_info.exponent, 4); > > modlen = keylen - 8; <-- integer underflow > > > > if (Stream_GetRemainingLength(s) < modlen + 8) > // count padding <-- modlen + 8, integer overflow > > return FALSE; > > > > certificate->cert_info.ModulusLength = modlen; > > certificate->cert_info.Modulus = > malloc(certificate->cert_info.ModulusLength); > > > > if (!certificate->cert_info.Modulus) > > return FALSE; > > > > Stream_Read(s, certificate->cert_info.Modulus, > certificate->cert_info.ModulusLength); <-- out of bound read > > Stream_Seek(s, 8); /* 8 bytes of zero padding */ > > > > return TRUE; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > unchecked return value => NULL deref > > > > freerdp\freerdp\channels\cliprdr\client\Cliprdr_format.c > > void cliprdr_process_format_data_response(cliprdrPlugin* cliprdr, wStream* > s, UINT32 dataLen, UINT16 msgFlags) > > { > > CLIPRDR_FORMAT_DATA_RESPONSE formatDataResponse; > > CliprdrClientContext* context = > cliprdr_get_client_interface(cliprdr); > > > > WLog_Print(cliprdr->log, WLOG_DEBUG, > "ServerFormatDataResponse"); > > > > if (!context->custom) > > return; > > > > formatDataResponse.msgType = CB_FORMAT_DATA_RESPONSE; > > formatDataResponse.msgFlags = msgFlags; > > formatDataResponse.dataLen = dataLen; > > formatDataResponse.requestedFormatData = NULL; > > > > if (dataLen) > > { > > formatDataResponse.requestedFormatData = > (BYTE*) malloc(dataLen); <-- need to check return value > > Stream_Read(s, > formatDataResponse.requestedFormatData, dataLen); <-- NULL deref > > } > > > > if (context->ServerFormatDataResponse) > > context->ServerFormatDataResponse(context, > &formatDataResponse); > > > > free(formatDataResponse.requestedFormatData); > > } > > > > > ------------------------------------------------------------------------------------------------------ > > integer overflow => endless loop > > > > freerdp\freerdp\winpr\libwinpr\utils\Stream.c > > void Stream_EnsureCapacity(wStream* s, size_t size) > > { > > if (s->capacity < size) > > { > > size_t position; > > size_t old_capacity; > > size_t new_capacity; > > > > old_capacity = s->capacity; > > new_capacity = old_capacity; > > > > do > > { > > new_capacity *= 2; > > } > > while (new_capacity < size); <-- endless > loop on really big sizes > > > > s->capacity = new_capacity; > > s->length = new_capacity; > > > > position = Stream_GetPosition(s); > > > > s->buffer = (BYTE*) realloc(s->buffer, > s->capacity); > > > > ZeroMemory(&s->buffer[old_capacity], > s->capacity - old_capacity); > > > > Stream_SetPosition(s, position); > > } > > } > > > > > ------------------------------------------------------------------------------------------------------ > > integer overflow in Stream_EnsureRemainingCapacity() > > > > freerdp\freerdp\winpr\libwinpr\utils\Stream.c > > void Stream_EnsureRemainingCapacity(wStream* s, size_t size) > > { > > if (Stream_GetPosition(s) + size > Stream_Capacity(s)) <-- > integer overflow > > Stream_EnsureCapacity(s, > Stream_Capacity(s) + size); > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > unvalidated length field => out of bound read/info leak > > > > freerdp\freerdp\channels\drive\client\Drive_main.c > > static void drive_process_irp_write(DRIVE_DEVICE* drive, IRP* irp) > > { > > DRIVE_FILE* file; > > UINT32 Length; > > UINT64 Offset; > > > > Stream_Read_UINT32(irp->input, Length); <-- unvalidated > length field > > Stream_Read_UINT64(irp->input, Offset); > > Stream_Seek(irp->input, 20); /* Padding */ > > > > file = drive_get_file_by_id(drive, irp->FileId); > > > > if (!file) > > { > > irp->IoStatus = STATUS_UNSUCCESSFUL; > > Length = 0; > > } > > else if (!drive_file_seek(file, Offset)) > > { > > irp->IoStatus = STATUS_UNSUCCESSFUL; > > Length = 0; > > } > > else if (!drive_file_write(file, > Stream_Pointer(irp->input), Length)) <-- out of bound read, information > leak (writing out of bound data to file, potential heartbleed-like bug) > > { > > irp->IoStatus = STATUS_UNSUCCESSFUL; > > Length = 0; > > } > > else > > { > > > > } > > > > Stream_Write_UINT32(irp->output, Length); > > Stream_Write_UINT8(irp->output, 0); /* Padding */ > > > > irp->Complete(irp); > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > integer overflow => memory corruption. > > > > freerdp\freerdp\libfreerdp\core\Gcc.c > > BOOL gcc_read_client_monitor_data(wStream* s, rdpMcs* mcs, UINT16 > blockLength) > > { > > UINT32 index; > > UINT32 flags; > > UINT32 monitorCount; > > MONITOR_DEF* monitorDefArray; > > > > if (blockLength < 8) > > return FALSE; > > > > Stream_Read_UINT32(s, flags); /* flags */ > > Stream_Read_UINT32(s, monitorCount); /* monitorCount */ > > > > if (blockLength < (8 + (monitorCount * 20))) <-- integer > overflow > > return FALSE; > > > > monitorDefArray = (MONITOR_DEF*) > malloc(sizeof(MONITOR_DEF) * monitorCount); <-- integer overflow > > ZeroMemory(monitorDefArray, sizeof(MONITOR_DEF) * > monitorCount); > > > > for (index = 0; index < monitorCount; index++) <-- memory > corruption in this loop in case of integer overflow > > { > > Stream_Read_UINT32(s, > monitorDefArray[index].left); /* left */ > > Stream_Read_UINT32(s, > monitorDefArray[index].top); /* top */ > > Stream_Read_UINT32(s, > monitorDefArray[index].right); /* right */ > > Stream_Read_UINT32(s, > monitorDefArray[index].bottom); /* bottom */ > > Stream_Read_UINT32(s, > monitorDefArray[index].flags); /* flags */ > > } > > > > free(monitorDefArray); > > > > return TRUE; > > } > > > > similar bug in gcc_read_client_monitor_extended_data() > > > > > ------------------------------------------------------------------------------------------------------ > > > > integer overflow in rdp_recv_logon_info_v2() > > > > freerdp\freerdp\libfreerdp\core\Info.c > > BOOL rdp_recv_logon_info_v2(rdpRdp* rdp, wStream* s) > > { > > UINT16 Version; > > UINT32 Size; > > UINT32 SessionId; > > UINT32 cbDomain; > > UINT32 cbUserName; > > > > if (Stream_GetRemainingLength(s) < 576) > > return FALSE; > > > > Stream_Read_UINT16(s, Version); /* Version (2 bytes) */ > > Stream_Read_UINT32(s, Size); /* Size (4 bytes) */ > > Stream_Read_UINT32(s, SessionId); /* SessionId (4 bytes) */ > > Stream_Read_UINT32(s, cbDomain); /* cbDomain (4 bytes) */ > > Stream_Read_UINT32(s, cbUserName); /* cbUserName (4 bytes) > */ > > Stream_Seek(s, 558); /* pad (558 bytes) */ > > > > if (Stream_GetRemainingLength(s) < (cbDomain + > cbUserName)) <-- integer overflow > > return FALSE; > > > > Stream_Seek(s, cbDomain); /* domain */ > > Stream_Seek(s, cbUserName); /* userName */ > > > > WLog_DBG(TAG, "LogonInfoV2: SessionId: 0x%04X", SessionId); > > > > return TRUE; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > potential integer overflow in license_read_scope_list() > > > > freerdp\freerdp\libfreerdp\core\License.c > > BOOL license_read_scope_list(wStream* s, SCOPE_LIST* scopeList) > > { > > UINT32 i; > > UINT32 scopeCount; > > > > if (Stream_GetRemainingLength(s) < 4) > > return FALSE; > > > > Stream_Read_UINT32(s, scopeCount); /* ScopeCount (4 bytes) > */ > > > > if (scopeCount > Stream_GetRemainingLength(s) / 4) /* > every blob is at least 4 bytes */ > > return FALSE; > > > > scopeList->count = scopeCount; > > scopeList->array = (LICENSE_BLOB*) > malloc(sizeof(LICENSE_BLOB) * scopeCount); <-- integer overflow > > > > /* ScopeArray */ > > for (i = 0; i < scopeCount; i++) > > { > > scopeList->array[i].type = BB_SCOPE_BLOB; > > > > if (!license_read_binary_blob(s, > &scopeList->array[i])) > > return FALSE; > > } > > > > return TRUE; > > } > > > > > > This one may not be feasible to trigger/exploit, since it requires a huge > remaining stream length, but it's insecure coding pratice at the least. > > > > > ------------------------------------------------------------------------------------------------------ > > > > unvalidated length field in => out of bound read/info leak > > > > freerdp\freerdp\channels\printer\client\Printer_main.c > > static void printer_process_irp_write(PRINTER_DEVICE* printer_dev, IRP* > irp) > > { > > UINT32 Length; > > UINT64 Offset; > > rdpPrintJob* printjob = NULL; > > > > Stream_Read_UINT32(irp->input, Length); > > Stream_Read_UINT64(irp->input, Offset); > > Stream_Seek(irp->input, 20); /* Padding */ > > > > if (printer_dev->printer) > > printjob = > printer_dev->printer->FindPrintJob(printer_dev->printer, irp->FileId); > > > > if (!printjob) > > { > > irp->IoStatus = STATUS_UNSUCCESSFUL; > > Length = 0; > > } > > else > > { > > printjob->Write(printjob, > Stream_Pointer(irp->input), Length); <-- Length never validated. could run > out of bound (which could crash, or leak information) > > } > > > > Stream_Write_UINT32(irp->output, Length); > > Stream_Write_UINT8(irp->output, 0); /* Padding */ > > > > irp->Complete(irp); > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > integer overflow => memory corruption > > > > freerdp\freerdp\libfreerdp\core\Rdp.c > > BOOL rdp_recv_monitor_layout_pdu(rdpRdp* rdp, wStream* s) > > { > > UINT32 index; > > UINT32 monitorCount; > > MONITOR_DEF* monitor; > > MONITOR_DEF* monitorDefArray; > > > > if (Stream_GetRemainingLength(s) < 4) > > return FALSE; > > > > Stream_Read_UINT32(s, monitorCount); /* monitorCount (4 > bytes) */ > > > > if (Stream_GetRemainingLength(s) < (monitorCount * 20)) > <-- integer overflow > > return FALSE; > > > > monitorDefArray = (MONITOR_DEF*) calloc(monitorCount, > sizeof(MONITOR_DEF)); <-- integer overflow > > > > if (!monitorDefArray) > > return FALSE; > > > > for (index = 0; index < monitorCount; index++) <-- memory > corruption in loop if integer overflow occurs > > { > > monitor = &(monitorDefArray[index]); > > Stream_Read_UINT32(s, monitor->left); /* > left (4 bytes) */ > > Stream_Read_UINT32(s, monitor->top); /* > top (4 bytes) */ > > Stream_Read_UINT32(s, monitor->right); /* > right (4 bytes) */ > > Stream_Read_UINT32(s, monitor->bottom); /* > bottom (4 bytes) */ > > Stream_Read_UINT32(s, monitor->flags); /* > flags (4 bytes) */ > > } > > > > free(monitorDefArray); > > > > return TRUE; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > unvalidated length field > > > > freerdp\freerdp\channels\rdpdr\server\Rdpdr_main.c > > static int > rdpdr_server_receive_device_list_announce_request(RdpdrServerContext* > context, wStream* s, RDPDR_HEADER* header) > > { > > int i; > > UINT32 DeviceCount; > > UINT32 DeviceType; > > UINT32 DeviceId; > > char PreferredDosName[9]; > > UINT32 DeviceDataLength; > > PreferredDosName[8] = 0; > > Stream_Read_UINT32(s, DeviceCount); /* DeviceCount (4 > bytes) */ <-- count never validated > > WLog_DBG(TAG, "DeviceCount: %d", DeviceCount); > > > > for (i = 0; i < DeviceCount; i++) <-- could cause out of > bound read > > { > > Stream_Read_UINT32(s, DeviceType); /* > DeviceType (4 bytes) */ > > Stream_Read_UINT32(s, DeviceId); /* > DeviceId (4 bytes) */ > > Stream_Read(s, PreferredDosName, 8); /* > PreferredDosName (8 bytes) */ > > Stream_Read_UINT32(s, DeviceDataLength); > /* DeviceDataLength (4 bytes) */ <-- length never validated > > WLog_DBG(TAG, "Device %d Name: %s Id: > 0x%04X DataLength: %d", > > i, > PreferredDosName, DeviceId, DeviceDataLength); > > > > switch (DeviceId) > > { > > case RDPDR_DTYP_FILESYSTEM: > > break; > > > > case RDPDR_DTYP_PRINT: > > break; > > > > case RDPDR_DTYP_SERIAL: > > break; > > > > case RDPDR_DTYP_PARALLEL: > > break; > > > > case RDPDR_DTYP_SMARTCARD: > > break; > > > > default: > > break; > > } > > > > Stream_Seek(s, DeviceDataLength); <-- > could cause out of bound read or endless loop > > } > > > > return 0; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > integer overflow in rdpgfx_read_h264_metablock() could cause memory > corruption. > > > > freerdp\freerdp\channels\rdpgfx\client\Rdpgfx_codec.c > > int rdpgfx_read_h264_metablock(RDPGFX_PLUGIN* gfx, wStream* s, > RDPGFX_H264_METABLOCK* meta) > > { > > UINT32 index; > > RDPGFX_RECT16* regionRect; > > RDPGFX_H264_QUANT_QUALITY* quantQualityVal; > > > > meta->regionRects = NULL; > > meta->quantQualityVals = NULL; > > > > if (Stream_GetRemainingLength(s) < 4) > > return -1; > > > > Stream_Read_UINT32(s, meta->numRegionRects); /* > numRegionRects (4 bytes) */ > > > > if (Stream_GetRemainingLength(s) < (meta->numRegionRects * > 8)) <-- integer overflow > > return -1; > > > > meta->regionRects = (RDPGFX_RECT16*) > malloc(meta->numRegionRects * sizeof(RDPGFX_RECT16)); <-- integer overflow > > > > if (!meta->regionRects) > > return -1; > > > > meta->quantQualityVals = (RDPGFX_H264_QUANT_QUALITY*) > malloc(meta->numRegionRects * sizeof(RDPGFX_H264_QUANT_QUALITY)); <-- > integer overflow > > > > if (!meta->quantQualityVals) > > return -1; > > > > WLog_DBG(TAG, "H264_METABLOCK: numRegionRects: %d", (int) > meta->numRegionRects); > > > > for (index = 0; index < meta->numRegionRects; index++) <-- > memory corruption in case integer overflow occurs > > { > > regionRect = &(meta->regionRects[index]); > > rdpgfx_read_rect16(s, regionRect); > > WLog_DBG(TAG, "regionRects[%d]: left: %d > top: %d right: %d bottom: %d", > > index, > regionRect->left, regionRect->top, regionRect->right, regionRect->bottom); > > } > > > > if (Stream_GetRemainingLength(s) < (meta->numRegionRects * > 2)) > > return -1; > > > > for (index = 0; index < meta->numRegionRects; index++) > > { > > quantQualityVal = > &(meta->quantQualityVals[index]); > > Stream_Read_UINT8(s, > quantQualityVal->qpVal); /* qpVal (1 byte) */ > > Stream_Read_UINT8(s, > quantQualityVal->qualityVal); /* qualityVal (1 byte) */ > > > > quantQualityVal->qp = > quantQualityVal->qpVal & 0x3F; > > quantQualityVal->r = > (quantQualityVal->qpVal >> 6) & 1; > > quantQualityVal->p = > (quantQualityVal->qpVal >> 7) & 1; > > WLog_DBG(TAG, "quantQualityVals[%d]: qp: > %d r: %d p: %d qualityVal: %d", > > index, > quantQualityVal->qp, quantQualityVal->r, quantQualityVal->p, > quantQualityVal->qualityVal); > > } > > > > return 1; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > integer overflow in rdpgfx_recv_reset_graphics_pdu() could cause memory > corruption > > > > freerdp\freerdp\channels\rdpgfx\client\Rdpgfx_main.c > > int rdpgfx_recv_reset_graphics_pdu(RDPGFX_CHANNEL_CALLBACK* callback, > wStream* s) > > { > > int pad; > > UINT32 index; > > MONITOR_DEF* monitor; > > RDPGFX_RESET_GRAPHICS_PDU pdu; > > RDPGFX_PLUGIN* gfx = (RDPGFX_PLUGIN*) callback->plugin; > > RdpgfxClientContext* context = (RdpgfxClientContext*) > gfx->iface.pInterface; > > > > if (Stream_GetRemainingLength(s) < 12) > > return -1; > > > > Stream_Read_UINT32(s, pdu.width); /* width (4 bytes) */ > > Stream_Read_UINT32(s, pdu.height); /* height (4 bytes) */ > > Stream_Read_UINT32(s, pdu.monitorCount); /* monitorCount > (4 bytes) */ > > > > if (Stream_GetRemainingLength(s) < (pdu.monitorCount * > 20)) <-- integer overflow > > return -1; > > > > pdu.monitorDefArray = (MONITOR_DEF*) > calloc(pdu.monitorCount, sizeof(MONITOR_DEF)); <-- integer overflow > > > > if (!pdu.monitorDefArray) > > return -1; > > > > for (index = 0; index < pdu.monitorCount; index++) <-- > memory corruption in case of integer overflow > > { > > monitor = &(pdu.monitorDefArray[index]); > > Stream_Read_UINT32(s, monitor->left); /* > left (4 bytes) */ > > Stream_Read_UINT32(s, monitor->top); /* > top (4 bytes) */ > > Stream_Read_UINT32(s, monitor->right); /* > right (4 bytes) */ > > Stream_Read_UINT32(s, monitor->bottom); /* > bottom (4 bytes) */ > > Stream_Read_UINT32(s, monitor->flags); /* > flags (4 bytes) */ > > } > > > > pad = 340 - (RDPGFX_HEADER_SIZE + 12 + (pdu.monitorCount * > 20)); > > > > if (Stream_GetRemainingLength(s) < (size_t) pad) > > { > > free(pdu.monitorDefArray); > > return -1; > > } > > > > Stream_Seek(s, pad); /* pad (total size is 340 bytes) */ > > > > WLog_Print(gfx->log, WLOG_DEBUG, "RecvResetGraphicsPdu: > width: %d height: %d count: %d", > > pdu.width, pdu.height, > pdu.monitorCount); > > > > if (context && context->ResetGraphics) > > { > > context->ResetGraphics(context, &pdu); > > } > > > > free(pdu.monitorDefArray); > > > > return 1; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > integer overflow in smartcard_unpack_connect_a_call() could cause memory > corruption > > > > freerdp\freerdp\channels\smartcard\client\Smartcard_pack.c > > UINT32 smartcard_unpack_connect_a_call(SMARTCARD_DEVICE* smartcard, > wStream* s, ConnectA_Call* call) > > { > > UINT32 status; > > UINT32 count; > > > > call->szReader = NULL; > > > > if (Stream_GetRemainingLength(s) < 4) > > { > > WLog_WARN(TAG, "ConnectA_Call is too > short: %d", > > (int) > Stream_GetRemainingLength(s)); > > return STATUS_BUFFER_TOO_SMALL; > > } > > > > Stream_Seek_UINT32(s); /* szReaderNdrPtr (4 bytes) */ > > > > status = smartcard_unpack_connect_common(smartcard, s, > &(call->Common)); > > > > if (status) > > return status; > > > > /* szReader */ > > > > Stream_Seek_UINT32(s); /* NdrMaxCount (4 bytes) */ > > Stream_Seek_UINT32(s); /* NdrOffset (4 bytes) */ > > Stream_Read_UINT32(s, count); /* NdrActualCount (4 bytes) > */ > > > > call->szReader = (unsigned char*) malloc(count + 1); <-- > integer overflow > > > > if (!call->szReader) > > { > > WLog_WARN(TAG, "ConnectA_Call out of > memory error (call->szReader)"); > > return STATUS_NO_MEMORY; > > } > > > > Stream_Read(s, call->szReader, count); <-- memory > corruption in case of integer overflow > > smartcard_unpack_read_size_align(smartcard, s, count, 4); > > call->szReader[count] = '\0'; > > > > smartcard_unpack_redir_scard_context_ref(smartcard, s, > &(call->Common.hContext)); > > > > return SCARD_S_SUCCESS; > > } > > > > same issue in smartcard_unpack_connect_w_call() > > > > > ------------------------------------------------------------------------------------------------------ > > > > unvalidated length fields => out of bound read > > > > freerdp\freerdp\channels\tsmf\client\Tsmf_ifman.c > > int tsmf_ifman_exchange_capability_request(TSMF_IFMAN* ifman) > > { > > UINT32 i; > > UINT32 v; > > UINT32 pos; > > UINT32 CapabilityType; > > UINT32 cbCapabilityLength; > > UINT32 numHostCapabilities; > > > > pos = Stream_GetPosition(ifman->output); > > Stream_EnsureRemainingCapacity(ifman->output, > ifman->input_size + 4); > > Stream_Copy(ifman->output, ifman->input, > ifman->input_size); > > Stream_SetPosition(ifman->output, pos); > > Stream_Read_UINT32(ifman->output, numHostCapabilities); > <-- unvalidated lengthfield > > > > for (i = 0; i < numHostCapabilities; i++) <-- out of bound > read if malicious numHostCapabilities > > { > > Stream_Read_UINT32(ifman->output, > CapabilityType); > > Stream_Read_UINT32(ifman->output, > cbCapabilityLength); <-- unvalidated lengthfield > > pos = Stream_GetPosition(ifman->output); > > > > switch (CapabilityType) > > { > > case 1: /* Protocol > version request */ > > > Stream_Read_UINT32(ifman->output, v); > > > DEBUG_TSMF("server protocol version %d", v); > > break; > > case 2: /* Supported > platform */ > > > Stream_Peek_UINT32(ifman->output, v); > > > DEBUG_TSMF("server supported platform %d", v); > > /* Claim > that we support both MF and DShow platforms. */ > > > Stream_Write_UINT32(ifman->output, MMREDIR_CAPABILITY_PLATFORM_MF | > MMREDIR_CAPABILITY_PLATFORM_DSHOW); > > break; > > default: > > > WLog_ERR(TAG, "unknown capability type %d", CapabilityType); > > break; > > } > > Stream_SetPosition(ifman->output, pos + > cbCapabilityLength); <-- out of bound read if malicious cbCapabilityLength > > } > > > > Stream_Write_UINT32(ifman->output, 0); /* Result */ > > ifman->output_interface_id = TSMF_INTERFACE_DEFAULT | > STREAM_ID_STUB; > > > > return 0; > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > integer overflow in tsmf_stream_push_sample() could cause memory corruption > > > > freerdp\freerdp\channels\tsmf\client\Tsmf_media.c > > void tsmf_stream_push_sample(TSMF_STREAM* stream, > IWTSVirtualChannelCallback *pChannelCallback, > > UINT32 sample_id, UINT64 > start_time, UINT64 end_time, UINT64 duration, UINT32 extensions, > > UINT32 data_size, BYTE > *data) > > { > > TSMF_SAMPLE* sample; > > SetEvent(stream->ready); > > > > if (TERMINATING) > > return; > > > > sample = (TSMF_SAMPLE*) calloc(1, sizeof(TSMF_SAMPLE)); > > > > if (!sample) > > { > > WLog_ERR(TAG, "calloc failed!"); > > return; > > } > > > > sample->sample_id = sample_id; > > sample->start_time = start_time; > > sample->end_time = end_time; > > sample->duration = duration; > > sample->extensions = extensions; > > sample->stream = stream; > > sample->channel_callback = pChannelCallback; > > sample->data_size = data_size; > > sample->data = calloc(1, data_size + > TSMF_BUFFER_PADDING_SIZE); <-- integer overflow > > > > if (!sample->data) > > { > > WLog_ERR(TAG, "calloc failed!"); > > free(sample); > > return; > > } > > > > CopyMemory(sample->data, data, data_size); <-- memory > corruption if interger overflow occurs > > Queue_Enqueue(stream->sample_list, sample); > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > unvalidated index in update_read_cache_brush_order() can trigger out of > bound, which could cause memory corruption. > > > > freerdp\freerdp\libfreerdp\core\Orders.c > > BOOL update_read_cache_brush_order(wStream* s, CACHE_BRUSH_ORDER* > cache_brush, UINT16 flags) > > { > > int i; > > int size; > > BYTE iBitmapFormat; > > BOOL compressed = FALSE; > > > > if (Stream_GetRemainingLength(s) < 6) > > return FALSE; > > > > Stream_Read_UINT8(s, cache_brush->index); /* cacheEntry (1 > byte) */ > > > > Stream_Read_UINT8(s, iBitmapFormat); /* iBitmapFormat (1 > byte) */ <-- unvalidated index > > cache_brush->bpp = BMF_BPP[iBitmapFormat]; <-- out of > bound read (BMF_BPP contains only 7 elements). if returned value (due to > out of bound read) is big enough, memory corruption can occur below > > ... > > UINT32 > scanline = (cache_brush->bpp / 8) * 8; > > > > if > (Stream_GetRemainingLength(s) < scanline * 8) > > > return FALSE; > > > > for (i = > 7; i >= 0; i--) > > { > > > Stream_Read(s, &cache_brush->data[i * scanline], scanline); <-- memory > corruption could occur here if out of bound read occurs > > } > > > > ... > > } > > > > > ------------------------------------------------------------------------------------------------------ > > > > I hope this is useful. > > > > Regards, > > Ilja van Sprundel. > ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ FreeRDP-devel mailing list FreeRDP-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/freerdp-devel
