From 0389cb129ed83de3b6ea31ed944dfc9f21ab5570 Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Tue, 19 Dec 2017 10:21:03 +0100 Subject: [PATCH 1/4] core: Fix array overrunning during FIPS keys generation p is 20 and r is 1 in the last iteration of fips_expand_key_bits, which means that buf[21] is read (of BYTE buf[21];). However, the value is not needed, because it is consequently discarded by "c & 0xfe" statement. Let's do not read buf[p + 1] when r is 1 to avoid this. --- libfreerdp/core/security.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libfreerdp/core/security.c b/libfreerdp/core/security.c index be03311dc..a6d8bd3dd 100644 --- a/libfreerdp/core/security.c +++ b/libfreerdp/core/security.c @@ -524,9 +524,9 @@ static void fips_expand_key_bits(BYTE* in, BYTE* out) p = b / 8; r = b % 8; - if (r == 0) + if (r <= 1) { - out[i] = buf[p] & 0xfe; + out[i] = (buf[p] << r) & 0xfe; } else { From e2f9a08107548705a13e9f007814c4ad426aec48 Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Tue, 19 Dec 2017 12:21:34 +0100 Subject: [PATCH 2/4] tsmf: Prevent string overflow and unterminated strings Device variable can overflow, or be unterminated. Replace strcpy by strncpy and be sure that the string is terminated (sizeof() - 1). --- channels/tsmf/client/alsa/tsmf_alsa.c | 2 +- channels/tsmf/client/oss/tsmf_oss.c | 2 +- channels/tsmf/client/pulse/tsmf_pulse.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/channels/tsmf/client/alsa/tsmf_alsa.c b/channels/tsmf/client/alsa/tsmf_alsa.c index 02f3941a1..78a0a6694 100644 --- a/channels/tsmf/client/alsa/tsmf_alsa.c +++ b/channels/tsmf/client/alsa/tsmf_alsa.c @@ -72,7 +72,7 @@ static BOOL tsmf_alsa_open(ITSMFAudioDevice *audio, const char *device) } else { - strncpy(alsa->device, device, sizeof(alsa->device)); + strncpy(alsa->device, device, sizeof(alsa->device) - 1); } return tsmf_alsa_open_device(alsa); } diff --git a/channels/tsmf/client/oss/tsmf_oss.c b/channels/tsmf/client/oss/tsmf_oss.c index 305f0ce46..db88ddbf7 100644 --- a/channels/tsmf/client/oss/tsmf_oss.c +++ b/channels/tsmf/client/oss/tsmf_oss.c @@ -81,7 +81,7 @@ static BOOL tsmf_oss_open(ITSMFAudioDevice* audio, const char* device) } else { - strncpy(oss->dev_name, device, sizeof(oss->dev_name)); + strncpy(oss->dev_name, device, sizeof(oss->dev_name) - 1); } if ((oss->pcm_handle = open(oss->dev_name, O_WRONLY)) < 0) diff --git a/channels/tsmf/client/pulse/tsmf_pulse.c b/channels/tsmf/client/pulse/tsmf_pulse.c index b61f98948..e6da92509 100644 --- a/channels/tsmf/client/pulse/tsmf_pulse.c +++ b/channels/tsmf/client/pulse/tsmf_pulse.c @@ -115,7 +115,7 @@ static BOOL tsmf_pulse_open(ITSMFAudioDevice *audio, const char *device) TSMFPulseAudioDevice *pulse = (TSMFPulseAudioDevice *) audio; if(device) { - strcpy(pulse->device, device); + strncpy(pulse->device, device, sizeof(pulse->device) - 1); } pulse->mainloop = pa_threaded_mainloop_new(); if(!pulse->mainloop) From 4791970c09b07d09cfa9ce7ff7899c1b47dc710d Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Tue, 19 Dec 2017 13:02:55 +0100 Subject: [PATCH 3/4] core: Remove redundant stream position changes Stream_Seek() is used, but consequently Stream_SetPosition() is used for position obtained by Stream_GetPosition() immediatelly before Stream_Seek(). Let's remove this stream position changes due to its redundancy. --- libfreerdp/core/rdp.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/libfreerdp/core/rdp.c b/libfreerdp/core/rdp.c index ebd6de5c4..767f8941a 100644 --- a/libfreerdp/core/rdp.c +++ b/libfreerdp/core/rdp.c @@ -615,17 +615,11 @@ BOOL rdp_send_data_pdu(rdpRdp* rdp, wStream* s, BYTE type, UINT16 channel_id) BOOL rdp_send_message_channel_pdu(rdpRdp* rdp, wStream* s, UINT16 sec_flags) { UINT16 length; - UINT32 sec_bytes; - size_t sec_hold; UINT32 pad; length = Stream_GetPosition(s); Stream_SetPosition(s, 0); rdp_write_header(rdp, s, length, rdp->mcs->messageChannelId); - sec_bytes = rdp_get_sec_bytes(rdp, sec_flags); - sec_hold = Stream_GetPosition(s); - Stream_Seek(s, sec_bytes); - Stream_SetPosition(s, sec_hold); if (!rdp_security_stream_out(rdp, s, length, sec_flags, &pad)) return FALSE; From 9f5d0d4c4d05020bec2201693e355c5010cfb097 Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Tue, 19 Dec 2017 14:42:06 +0100 Subject: [PATCH 4/4] crypto: Improve PER OID calculations "(oid[0] << 4) & (oid[1] & 0x0F)" statement is always 0. It is not problem currently because the only OID which is written by this function should have 0 there. The function to read/write are pretty limited anyway and can't work properly with all kind of OIDs. Maybe it would be better to hardcode the OID there without decoding and encoding. But those functions are already there so let's improve them a bit according the spec and warn about limited set of supported OIDs. See: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540809 --- libfreerdp/crypto/per.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libfreerdp/crypto/per.c b/libfreerdp/crypto/per.c index 6a7bbf809..41f0ea822 100644 --- a/libfreerdp/crypto/per.c +++ b/libfreerdp/crypto/per.c @@ -309,6 +309,7 @@ void per_write_enumerated(wStream* s, BYTE enumerated, BYTE count) * Read PER OBJECT_IDENTIFIER (OID). * @param s stream * @param oid object identifier (OID) + * @warning It works correctly only for limited set of OIDs. * @return */ @@ -328,8 +329,8 @@ BOOL per_read_object_identifier(wStream* s, BYTE oid[6]) return FALSE; Stream_Read_UINT8(s, t12); /* first two tuples */ - a_oid[0] = (t12 >> 4); - a_oid[1] = (t12 & 0x0F); + a_oid[0] = t12 / 40; + a_oid[1] = t12 % 40; Stream_Read_UINT8(s, a_oid[2]); /* tuple 3 */ Stream_Read_UINT8(s, a_oid[3]); /* tuple 4 */ @@ -352,11 +353,12 @@ BOOL per_read_object_identifier(wStream* s, BYTE oid[6]) * Write PER OBJECT_IDENTIFIER (OID) * @param s stream * @param oid object identifier (oid) + * @warning It works correctly only for limited set of OIDs. */ void per_write_object_identifier(wStream* s, BYTE oid[6]) { - BYTE t12 = (oid[0] << 4) & (oid[1] & 0x0F); + BYTE t12 = oid[0] * 40 + oid[1]; Stream_Write_UINT8(s, 5); /* length */ Stream_Write_UINT8(s, t12); /* first two tuples */ Stream_Write_UINT8(s, oid[2]); /* tuple 3 */