mike-jumper commented on a change in pull request #274:
URL: https://github.com/apache/guacamole-server/pull/274#discussion_r419897219
##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-messages.c
##########
@@ -278,6 +290,14 @@ void
guac_rdpdr_process_device_iorequest(guac_rdp_common_svc* svc,
guac_rdpdr* rdpdr = (guac_rdpdr*) svc->data;
guac_rdpdr_iorequest iorequest;
+ /* Check to make sure the Stream contains at least 20 bytes (5 x UINT32 ).
*/
+ if (Stream_GetRemainingLength(input_stream) < 20) {
+ guac_client_log(svc->client, GUAC_LOG_WARNING, "Device Stream does not
"
Review comment:
I would say "Device I/O Request" (the name of the PDU) rather than
"Device Stream".
##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-messages.c
##########
@@ -243,6 +247,14 @@ void guac_rdpdr_process_device_reply(guac_rdp_common_svc*
svc,
unsigned int device_id, ntstatus;
int severity, c, n, facility, code;
+ /* Stream should contain at least 8 bytes (UINT32 + UINT32 ) */
+ if (Stream_GetRemainingLength(input_stream) < 8) {
+ guac_client_log(svc->client, GUAC_LOG_WARNING, "Device Stream does not
"
Review comment:
I would say "Server Device Announce Response" (the name of the PDU)
rather than "Device Stream".
##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-fs-messages.c
##########
@@ -48,6 +48,14 @@ void guac_rdpdr_fs_process_create(guac_rdp_common_svc* svc,
int create_disposition, create_options, path_length;
char path[GUAC_RDP_FS_MAX_PATH];
+ /* Check remaining stream data prior to reading. */
+ if (Stream_GetRemainingLength(input_stream) < 32) {
+ guac_client_log(svc->client, GUAC_LOG_WARNING, "File stream does not "
+ "contain the expected number of bytes. File sharing may not "
Review comment:
I don't believe we have historically referred to this as file sharing.
"File transfer" or "drive redirection" would probably be better, as they would
match the relevant terminology of the manual and general RDP concepts.
##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-fs-messages-file-info.c
##########
@@ -135,6 +135,14 @@ void
guac_rdpdr_fs_process_set_rename_info(guac_rdp_common_svc* svc,
wStream* output_stream;
char destination_path[GUAC_RDP_FS_MAX_PATH];
+ /* Check stream size prior to reading. */
+ if (Stream_GetRemainingLength(input_stream) < 6) {
+ guac_client_log(svc->client, GUAC_LOG_WARNING, "File Stream does not "
Review comment:
I would say "Server Drive Set Information Request
(FileRenameInformation)" (the name of the PDU and data being set) rather than
"File Stream".
##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-fs-messages.c
##########
@@ -172,6 +188,14 @@ void guac_rdpdr_fs_process_write(guac_rdp_common_svc* svc,
wStream* output_stream;
+ /* Check remaining length. */
+ if (Stream_GetRemainingLength(input_stream) < 32) {
+ guac_client_log(svc->client, GUAC_LOG_WARNING, "File Stream does not "
Review comment:
And here - something which notes the specific PDU involved ("Server
Create Drive Request") would be better than "File Stream".
##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-fs-messages.c
##########
@@ -123,6 +131,14 @@ void guac_rdpdr_fs_process_read(guac_rdp_common_svc* svc,
wStream* output_stream;
+ /* Check remaining bytes before reading stream. */
+ if (Stream_GetRemainingLength(input_stream) < 12) {
+ guac_client_log(svc->client, GUAC_LOG_WARNING, "File Stream does not "
Review comment:
Same here - I would say "Server Create Drive Request" (the name of the
PDU) rather than "File stream".
##########
File path: src/protocols/rdp/plugins/guacai/guacai-messages.c
##########
@@ -48,8 +52,9 @@ static void guac_rdp_ai_read_format(wStream* stream,
Stream_Read_UINT16(stream, format->bps); /* wBitsPerSample */
Stream_Read_UINT16(stream, format->data_size); /* cbSize */
- /* Read arbitrary data block (if applicable) */
- if (format->data_size != 0) {
+ /* Read arbitrary data block (if applicable) and data is available. */
+ if (format->data_size != 0
+ && Stream_GetRemainingLength(stream) >= format->data_size) {
Review comment:
This may produce some odd behavior. The check is necessary, but the
failure status may need to be carried outward to the caller of
`guac_rdp_ai_read_format()`.
This function is invoked by `guac_rdp_ai_process_formats()` in a loop, using
each call to process one of the received audio input formats. If this check
fails for one of the formats, and other formats remain to be read, then their
headers will start to be read within what theoretically is the data portion of
the preceding format.
##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-fs-messages.c
##########
@@ -48,6 +48,14 @@ void guac_rdpdr_fs_process_create(guac_rdp_common_svc* svc,
int create_disposition, create_options, path_length;
char path[GUAC_RDP_FS_MAX_PATH];
+ /* Check remaining stream data prior to reading. */
+ if (Stream_GetRemainingLength(input_stream) < 32) {
+ guac_client_log(svc->client, GUAC_LOG_WARNING, "File stream does not "
Review comment:
I would say "Server Create Drive Request" (the name of the PDU) rather
than "File stream".
##########
File path: src/protocols/rdp/channels/rdpsnd/rdpsnd-messages.c
##########
@@ -266,6 +307,14 @@ void guac_rdpsnd_wave_handler(guac_rdp_common_svc* svc,
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
guac_audio_stream* audio = rdp_client->audio;
+
+ /* Verify we have at least 4 bytes, which is manually copied below. */
+ if (Stream_Length(input_stream) < 4) {
Review comment:
That 4 bytes is actually copied from `rdpsnd->initial_wave_data`, not
the `input_stream`. For `input_stream`, the read happens when
`guac_audio_stream_write_pcm()` is called, and reads
`rdpsnd->incoming_wave_size + 4` bytes:
https://github.com/apache/guacamole-server/blob/9c37fc56171d55b8fa838949d7fd40db85506857/src/protocols/rdp/channels/rdpsnd/rdpsnd-messages.c#L281-L282
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
