From 0a986869cc817b3bd798c76278fe48d65b1015e1 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:24:58 +0100 Subject: [PATCH 1/7] dechunker: Add handler for Dynamic channels --- common/dechunker.c | 182 ++++++++++++++++++++++++++++++++++++++++++++- common/dechunker.h | 105 ++++++++++++++++++++++++-- 2 files changed, 277 insertions(+), 10 deletions(-) diff --git a/common/dechunker.c b/common/dechunker.c index 7ead6d0c..c20e1801 100644 --- a/common/dechunker.c +++ b/common/dechunker.c @@ -57,11 +57,17 @@ struct vc_dechunker struct stream *reassembly_s; }; +struct dyn_dechunker +{ + char name[64]; + struct stream *reassembly_s; +}; + enum { // This is a rather arbitrary figure, but one we are unlikely to // go below. It's a sanity check for vc_dechunker_init() - E_MAX_CHUNK_SIZE_LOWER_LIMIT = 50 + E_MAX_VC_CHUNK_SIZE_LOWER_LIMIT = 50 }; /*****************************************************************************/ struct vc_dechunker * @@ -72,7 +78,7 @@ vc_dechunker_init(const char *chan_name, int max_chunk_size) { LOG(LOG_LEVEL_ERROR, "vc_dechunker_init() called with no channel name"); } - else if (max_chunk_size < E_MAX_CHUNK_SIZE_LOWER_LIMIT) + else if (max_chunk_size < E_MAX_VC_CHUNK_SIZE_LOWER_LIMIT) { LOG(LOG_LEVEL_ERROR, "Dechunker: Max chunk size for %s is too small", chan_name); @@ -237,6 +243,9 @@ handle_reading_state(struct vc_dechunker *self, // Tell the caller the stream is available. self->state = E_DATA; rv = E_VC_READY; + LOG_DEVEL(LOG_LEVEL_INFO, + "Dechunker: Reassembled PDU of size %d on %s", + self->reassembly_s->size, self->name); } else { @@ -369,3 +378,172 @@ vc_dechunker_get_stream(struct vc_dechunker *self) return s; } +/*****************************************************************************/ +struct dyn_dechunker * +dyn_dechunker_init(const char *chan_name) +{ + struct dyn_dechunker *self = NULL; + if (chan_name == NULL) + { + LOG(LOG_LEVEL_ERROR, + "dyn_dechunker_init() called with no channel name"); + } + else if ((self = g_new(struct dyn_dechunker, 1)) == NULL) + { + LOG(LOG_LEVEL_ERROR, "Dechunker: no memory for %s", chan_name); + } + else + { + strlcpy(self->name, chan_name, sizeof(self->name)); + self->reassembly_s = NULL; + } + + return self; +} + +/*****************************************************************************/ +void +dyn_dechunker_free(struct dyn_dechunker *self) +{ + if (self != NULL) + { + free_stream(self->reassembly_s); + free(self); + } +} + +/*****************************************************************************/ +enum dyn_dechunker_status +dyn_dechunker_process_first_chunk(struct dyn_dechunker *self, + struct stream *s, int total_size) +{ + enum dyn_dechunker_status status = E_DYN_ERROR; + + int frag_size = s ? s_rem(s) : 0; + if (self == NULL || s == NULL) + { + ; // Nothing to be done + } + else if (total_size <= 1590 || frag_size > total_size) + { + // See [MS-RDPEDYC] 2.2.3 + LOG(LOG_LEVEL_ERROR, + "Badly sized DYNVC_DATA_FIRST PDU received on dynamic channel %s", + self->name); + } + else if (self->reassembly_s != NULL) + { + LOG(LOG_LEVEL_ERROR, + "unexpected DYNVC_DATA_FIRST received on dynamic channel %s", + self->name); + } + else if (frag_size == total_size) + { + // This chunk ccontains all the data + status = E_DYN_INLINE_CHUNK; + } + else + { + make_stream(self->reassembly_s); + if (self->reassembly_s) + { + init_stream(self->reassembly_s, total_size); + } + if (self->reassembly_s == NULL || self->reassembly_s->data == NULL) + { + LOG(LOG_LEVEL_ERROR, + "Out of memory for dynamic PDU reassembly on %s", + self->name); + } + else + { + out_uint8p(self->reassembly_s, s->p, frag_size); + in_uint8s(s, frag_size); + status = E_DYN_IN_PROGRESS; + } + } + + return status; +} + +/*****************************************************************************/ +enum dyn_dechunker_status +dyn_dechunker_process_data_chunk(struct dyn_dechunker *self, + struct stream *s) +{ + enum dyn_dechunker_status rv; + + if (self == NULL || s == NULL) + { + rv = E_DYN_ERROR; + } + else if (self->reassembly_s == NULL) + { + rv = E_DYN_INLINE_CHUNK; + } + else + { + int frag_size = s_rem(s); + // We're currently reconstructing a data PDU from fragments + if (!s_check_rem_out(self-> reassembly_s, frag_size)) + { + LOG(LOG_LEVEL_ERROR, + "Oversized DYNVC_DATA when reconstructing PDU on %s", + self->name); + rv = E_DYN_ERROR; + } + else + { + out_uint8p(self->reassembly_s, s->p, frag_size); + in_uint8s(s, frag_size); + + if (s_rem_out(self->reassembly_s) == 0) + { + // Finished defragging + s_mark_end(self->reassembly_s); + self->reassembly_s->p = self->reassembly_s->data; + rv = E_DYN_READY; + LOG_DEVEL(LOG_LEVEL_INFO, + "Dechunker: Reassembled PDU of size %d on %s", + self->reassembly_s->size, self->name); + } + else + { + rv = E_DYN_IN_PROGRESS; + } + } + } + + return rv; +} + +/*****************************************************************************/ +struct stream * +dyn_dechunker_get_stream(struct dyn_dechunker *self) +{ + struct stream *s; + const char *name = (self != NULL) ? self->name : ""; + if (self == NULL || self->reassembly_s == NULL || + self->reassembly_s->end == self->reassembly_s->data) + { + LOG (LOG_LEVEL_ERROR, + "Dechunker: get stream called for %s with no data available", + name); + s = NULL; + } + else + { + // Pass ownership of the stream to the caller + s = self->reassembly_s; + self->reassembly_s = NULL; // So we don't free it ourselves! + } + + return s; +} + +/*****************************************************************************/ +int +dyn_dechunker_pending(struct dyn_dechunker *self) +{ + return (self != NULL && self->reassembly_s != NULL); +} diff --git a/common/dechunker.h b/common/dechunker.h index f3dcdbda..5bc6c8ce 100644 --- a/common/dechunker.h +++ b/common/dechunker.h @@ -34,11 +34,12 @@ struct stream; -/* Private type */ -struct vc_dechunker; +/* Private types */ +struct vc_dechunker; // static virtual channel dechunker +struct dyn_dechunker; // dynamic channel dechunker /** - * Returned from dechunker_process_vc_chunk() + * Returned from vc_dechunker_process_chunk() */ enum vc_dechunker_status { @@ -49,7 +50,19 @@ enum vc_dechunker_status }; /** - * Initialise a virtual channel dechunker + * Returned from dyn_dechunker_process_chunk() and + * Returned from dyn_dechunker_process_first_chunk() + */ +enum dyn_dechunker_status +{ + E_DYN_INLINE_CHUNK = 0, ///< This chunk is complete in itself + E_DYN_IN_PROGRESS, ///< The dechunker is processing chunks + E_DYN_READY, ///< A dechunked stream is now complete + E_DYN_ERROR ///< An error occurred (logged) +}; + +/** + * Initialise a static virtual channel dechunker * * @param chan_name - Name of channel * @param max_chunk_size - Max size of chunks allowed on channel @@ -59,14 +72,14 @@ struct vc_dechunker * vc_dechunker_init(const char *chan_name, int max_chunk_size); /** - * Free a virtual channel dechunker + * Free a static virtual channel dechunker * @param self vc dechunker to free */ void vc_dechunker_free(struct vc_dechunker *self); /** - * Process a virtual channel chunk + * Process a static virtual channel chunk * * @param self dechunker * @param s Stream for chunk, positioned at start of chunk @@ -84,9 +97,9 @@ enum vc_dechunker_status vc_dechunker_process_chunk(struct vc_dechunker *self, struct stream *s, int flags, int total_size); /** - * Get the stream from a ready dechunker + * Get the stream from a ready static virtual dechunker * - * @param self virtual channel dechunker + * @param self static virtual channel dechunker * @return input stream containing completed chunk * * Ownership of the stream passes to the caller @@ -97,4 +110,80 @@ struct stream * vc_dechunker_get_stream(struct vc_dechunker *self); +/** + * Initialise a dynamic virtual channel dechunker + * + * @param chan_name - Name of channel + * @return dyn_dechunker + */ +struct dyn_dechunker * +dyn_dechunker_init(const char *chan_name); + +/** + * Free a dynamic channel dechunker + * @param self dynamic dechunker to free + */ +void +dyn_dechunker_free(struct dyn_dechunker *self); + +/** + * Process a dynamic channel DYNVC_DATA_FIRST PDU ([MS-RDPEDYC] 2.2.3.1) + * + * @param self dechunker + * @param s Stream for chunk, positioned at start of data + * @param total_size length from DYNVC_DATA_FIRST header + * @return status of dechunker + * + * For PDUs of size > 1590 bytes, but < 1600, it is possible for the + * status E_DYN_INLINE_CHUNK to be returned, as the first chunk + * is complete in itself. This follows from [MS-RDPEDYC] 1.3.3.2.1 and + * 2.2.3.1 + * + * E_DYN_READY will not be returned by this call. + * + * On error, it is not possible to recover the stream, because of the + * impossibility of distinguishing self-contained DATA PDUs, and + * DATA PDUs which are part of a larger PDU. + */ +enum dyn_dechunker_status +dyn_dechunker_process_first_chunk(struct dyn_dechunker *self, + struct stream *s, int total_size); + +/** + * Process a dynamic channel DYNVC_DATA PDU ([MS-RDPEDYC] 2.2.3.2) + * + * @param self dechunker + * @param s Stream for chunk, positioned at start of data + * @return status of dechunker + * + * On error, it is not possible to recover the stream, because of the + * impossibility of distinguishing self-contained DATA PDUs, and + * DATA PDUs which are part of a larger PDU. + */ +enum dyn_dechunker_status +dyn_dechunker_process_data_chunk(struct dyn_dechunker *self, + struct stream *s); + +/** + * Get the stream from a ready dynamic dechunker + * + * @param self dynamic channel dechunker + * @return input stream containing completed chunk + * + * Ownership of the stream passes to the caller + * + * Resets the dechunker state so that further chunks can be processed. + */ +struct stream * +dyn_dechunker_get_stream(struct dyn_dechunker *self); + +/** + * Queries a dynamic dechunker for pending data + * + * @param self dynamic channel dechunker + * @return != 0 if data is stored in the dechunker + */ +int +dyn_dechunker_pending(struct dyn_dechunker *self); + #endif // DECHUNKER_H From e4b062122927312e4f11dab9626710af71b09609 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:22:56 +0100 Subject: [PATCH 2/7] tests: Add tests for dynamic dechunker --- tests/common/test_dechunker.c | 441 ++++++++++++++++++++++++++++++---- 1 file changed, 400 insertions(+), 41 deletions(-) diff --git a/tests/common/test_dechunker.c b/tests/common/test_dechunker.c index c2d917dd..9f945cb1 100644 --- a/tests/common/test_dechunker.c +++ b/tests/common/test_dechunker.c @@ -143,13 +143,24 @@ static const char frankenstein[] = "all your love and kindness.\n\n" "Your affectionate brother,\nR. Walton "; -// Number of CHANNEL_CHUNK_LENGTH chunks required to send the -// above text into the dechunker -#define FRANKENSTEIN_CHUNK_COUNT \ - ((sizeof(frankenstein) + (CHANNEL_CHUNK_LENGTH -1)) \ +// Number of CHANNEL_CHUNK_LENGTH (1600) chunks required to send the +// above text into the vc dechunker +#define FRANKENSTEIN_VC_CHUNK_COUNT \ + ((sizeof(frankenstein) + (CHANNEL_CHUNK_LENGTH - 1)) \ / CHANNEL_CHUNK_LENGTH) -// See the private E_MAX_CHUNK_SIZE_LOWER_LIMIT in dechunker.c +// The dynamic dechunker works on total data block sizes of 1600 bytes, +// including the block header as well. +// The FIRST block header is 6-12 bytes long, and the DATA block header +// is 5-8 bytes long. For simplicity we're assume a header size of 8 +// bytes, and hence a data size of 1592 bytes. +#define FRANKENSTEIN_DYN_CHUNK_SIZE 1592 + +#define FRANKENSTEIN_DYN_CHUNK_COUNT \ + ((sizeof(frankenstein) + (FRANKENSTEIN_DYN_CHUNK_SIZE - 1)) \ + / FRANKENSTEIN_DYN_CHUNK_SIZE) + +// See the private E_MAX_VC_CHUNK_SIZE_LOWER_LIMIT in dechunker.c #define PAD50 " " /******************************************************************************/ @@ -175,7 +186,7 @@ make_stream_from_data(const char *data, int data_len) /* * Check bad parameters passed to the dechunker functions */ -START_TEST(test_dechunker_bad_params) +START_TEST(test_vc_dechunker_bad_params) { struct vc_dechunker *dc; const char data[] = "Some stream data"; @@ -216,7 +227,7 @@ START_TEST(test_dechunker_bad_params) * immediately returned to the caller with E_VC_INLINE_CHUNK. If * however, we are currently dechunking, a passthrough chunk is not allowed */ -START_TEST(test_dechunker_passthrough) +START_TEST(test_vc_dechunker_passthrough) { const char data[] = "Some data to dechunk"; struct stream *s = make_stream_from_data(data, sizeof(data)); @@ -272,7 +283,7 @@ END_TEST * An intermediate chunk (neither first of last) must be rejected if we * are not dechunking */ -START_TEST(test_dechunker_intermediate) +START_TEST(test_vc_dechunker_intermediate) { const char data[] = PAD50 "Some data to dechunk"; struct stream *s = make_stream_from_data(data, sizeof(data)); @@ -297,7 +308,7 @@ END_TEST /* * A LAST chunk must be rejected if we are not dechunking */ -START_TEST(test_dechunker_last) +START_TEST(test_vc_dechunker_last) { const char data[] = PAD50 "Some data to dechunk"; struct stream *s = make_stream_from_data(data, sizeof(data)); @@ -322,7 +333,7 @@ END_TEST /** * Two consecutive FIRST chunks are not allowed */ -START_TEST(test_dechunker_first_first) +START_TEST(test_vc_dechunker_first_first) { const char data[] = PAD50 "Some data to dechunk"; struct stream *s = make_stream_from_data(data, sizeof(data)); @@ -367,7 +378,7 @@ END_TEST * > Virtual channel data that fits in a single Virtual Channel PDU MUST * > specify both flags */ -START_TEST(test_dechunker_chunk_overflow) +START_TEST(test_vc_dechunker_chunk_overflow) { const char data[] = PAD50 "Some data to dechunk"; struct stream *s = make_stream_from_data(data, sizeof(data)); @@ -408,15 +419,15 @@ END_TEST // up to CHANNEL_CHUNK_LENGTH bytes from Frankenstein chapter 1 // The stream pointer will be positioned at the start of the text. static struct stream * -make_bigtest_chunk(unsigned int chunk_num, int *flags) +make_vc_bigtest_chunk(unsigned int chunk_num, int *flags) { struct stream *s = NULL; - if (chunk_num < FRANKENSTEIN_CHUNK_COUNT) + if (chunk_num < FRANKENSTEIN_VC_CHUNK_COUNT) { int chunk_size; // Work out the size of this chunk - if (chunk_num == (FRANKENSTEIN_CHUNK_COUNT - 1)) + if (chunk_num == (FRANKENSTEIN_VC_CHUNK_COUNT - 1)) { chunk_size = sizeof(frankenstein) % CHANNEL_CHUNK_LENGTH; if (chunk_size == 0) @@ -456,7 +467,7 @@ make_bigtest_chunk(unsigned int chunk_num, int *flags) // Sort out the flags *flags = (chunk_num == 0) ? XR_CHANNEL_FLAG_FIRST : - (chunk_num == (FRANKENSTEIN_CHUNK_COUNT - 1)) ? XR_CHANNEL_FLAG_LAST : + (chunk_num == (FRANKENSTEIN_VC_CHUNK_COUNT - 1)) ? XR_CHANNEL_FLAG_LAST : 0; } @@ -468,7 +479,7 @@ make_bigtest_chunk(unsigned int chunk_num, int *flags) * Streams a lot of data through the dechunker and checks it's all * assembled correctly at the end */ -START_TEST(test_dechunker_big_test) +START_TEST(test_vc_dechunker_big_test) { enum vc_dechunker_status stat; struct vc_dechunker *dc = vc_dechunker_init("test", CHANNEL_CHUNK_LENGTH); @@ -476,15 +487,15 @@ START_TEST(test_dechunker_big_test) struct stream *s; int i; - for (i = 0 ; i < FRANKENSTEIN_CHUNK_COUNT; ++i) + for (i = 0 ; i < FRANKENSTEIN_VC_CHUNK_COUNT; ++i) { int flags; - s = make_bigtest_chunk(i, &flags); + s = make_vc_bigtest_chunk(i, &flags); stat = vc_dechunker_process_chunk( dc, s, flags, sizeof(frankenstein)); - if (i < FRANKENSTEIN_CHUNK_COUNT - 1) + if (i < FRANKENSTEIN_VC_CHUNK_COUNT - 1) { ck_assert_int_eq(stat, E_VC_IN_PROGRESS); } @@ -513,8 +524,8 @@ START_TEST(test_dechunker_big_test) } /******************************************************************************/ -// Like test_dechunker_big_test, but the last chunk is oversized -START_TEST(test_dechunker_big_test_oversize_fail) +// Like test_vc_dechunker_big_test, but the last chunk is oversized +START_TEST(test_vc_dechunker_big_test_oversize_fail) { enum vc_dechunker_status stat; struct vc_dechunker *dc = vc_dechunker_init("test", CHANNEL_CHUNK_LENGTH); @@ -522,11 +533,11 @@ START_TEST(test_dechunker_big_test_oversize_fail) struct stream *s; int i; - for (i = 0 ; i < FRANKENSTEIN_CHUNK_COUNT; ++i) + for (i = 0 ; i < FRANKENSTEIN_VC_CHUNK_COUNT; ++i) { int flags; - s = make_bigtest_chunk(i, &flags); - if (i == (FRANKENSTEIN_CHUNK_COUNT - 1)) + s = make_vc_bigtest_chunk(i, &flags); + if (i == (FRANKENSTEIN_VC_CHUNK_COUNT - 1)) { // Add a byte to the end of the text in the chunk struct stream *s2; @@ -546,7 +557,7 @@ START_TEST(test_dechunker_big_test_oversize_fail) dc, s, flags, sizeof(frankenstein)); - if (i < FRANKENSTEIN_CHUNK_COUNT - 1) + if (i < FRANKENSTEIN_VC_CHUNK_COUNT - 1) { ck_assert_int_eq(stat, E_VC_IN_PROGRESS); } @@ -561,8 +572,8 @@ START_TEST(test_dechunker_big_test_oversize_fail) } /******************************************************************************/ -// Like test_dechunker_big_test, but the last chunk is undersized -START_TEST(test_dechunker_big_test_undersize_fail) +// Like test_vc_dechunker_big_test, but the last chunk is undersized +START_TEST(test_vc_dechunker_big_test_undersize_fail) { enum vc_dechunker_status stat; struct vc_dechunker *dc = vc_dechunker_init("test", CHANNEL_CHUNK_LENGTH); @@ -570,11 +581,11 @@ START_TEST(test_dechunker_big_test_undersize_fail) struct stream *s; int i; - for (i = 0 ; i < FRANKENSTEIN_CHUNK_COUNT; ++i) + for (i = 0 ; i < FRANKENSTEIN_VC_CHUNK_COUNT; ++i) { int flags; - s = make_bigtest_chunk(i, &flags); - if (i == (FRANKENSTEIN_CHUNK_COUNT - 1)) + s = make_vc_bigtest_chunk(i, &flags); + if (i == (FRANKENSTEIN_VC_CHUNK_COUNT - 1)) { // Skip a byte in the stream, so there is one // less byte than expected @@ -584,7 +595,7 @@ START_TEST(test_dechunker_big_test_undersize_fail) dc, s, flags, sizeof(frankenstein)); - if (i < FRANKENSTEIN_CHUNK_COUNT - 1) + if (i < FRANKENSTEIN_VC_CHUNK_COUNT - 1) { ck_assert_int_eq(stat, E_VC_IN_PROGRESS); } @@ -600,6 +611,344 @@ START_TEST(test_dechunker_big_test_undersize_fail) /******************************************************************************/ +/******************************************************************************/ +/* + * Check bad parameters passed to the dechunker functions + */ +START_TEST(test_dyn_dechunker_bad_params) +{ + struct dyn_dechunker *dc; + const char data[] = "Some stream data"; + struct stream *s = make_stream_from_data(data, sizeof(data)); + enum dyn_dechunker_status stat; + + // dyn_dechunker_init + dc = dyn_dechunker_init(NULL); // No channel name + ck_assert_ptr_eq(dc, NULL); + dc = dyn_dechunker_init("test"); // Should be OK + ck_assert_ptr_ne(dc, NULL); + + // dyn_dechunker_free + dyn_dechunker_free(NULL); // Musn't crash! + + // dyn_dechunker_get_stream + dyn_dechunker_get_stream(NULL); // Musn't crash! + + // dyn_dechunker_process_first_chunk + stat = dyn_dechunker_process_first_chunk(NULL, s, 1600); // No dechunker + ck_assert_int_eq(stat, E_DYN_ERROR); + stat = dyn_dechunker_process_first_chunk(dc, NULL, 1600); // No stream + ck_assert_int_eq(stat, E_DYN_ERROR); + stat = dyn_dechunker_process_first_chunk(dc, s, -1); // bad total_size + ck_assert_int_eq(stat, E_DYN_ERROR); + + // dyn_dechunker_process_data_chunk + stat = dyn_dechunker_process_data_chunk(NULL, s); // No dechunker + ck_assert_int_eq(stat, E_DYN_ERROR); + stat = dyn_dechunker_process_data_chunk(dc, NULL); // No stream + ck_assert_int_eq(stat, E_DYN_ERROR); + + free_stream(s); + dyn_dechunker_free(dc); +} + +/******************************************************************************/ +/* + * Check passthrough DATA chunks (i.e. those not following a FIRST) + * + * When the dechunker is in normal operation, these chunks are + * immediately returned to the caller with E_DYN_INLINE_CHUNK. + */ +START_TEST(test_dyn_dechunker_passthrough) +{ + const char data[] = "Some data to dechunk"; + struct stream *s = make_stream_from_data(data, sizeof(data)); + enum dyn_dechunker_status stat; + + struct dyn_dechunker *dc = dyn_dechunker_init("test"); + ck_assert_ptr_ne(dc, NULL); + + // Save the stream pointer so we can reset the stream in between calls + s_push_layer(s, iso_hdr, 0); + + // Check a DATA chunk is normally recognised immediately + stat = dyn_dechunker_process_data_chunk( + dc, s); + ck_assert_int_eq(stat, E_DYN_INLINE_CHUNK); + + dyn_dechunker_free(dc); + free_stream(s); +} +END_TEST + + +/******************************************************************************/ +/** + * Two consecutive FIRST chunks are not allowed + */ +START_TEST(test_dyn_dechunker_first_first) +{ + const char data[] = PAD50 "Some data to dechunk"; + struct stream *s = make_stream_from_data(data, sizeof(data)); + enum dyn_dechunker_status stat; + + struct dyn_dechunker *dc = dyn_dechunker_init("test"); + ck_assert_ptr_ne(dc, NULL); + + // Save the stream pointer so we can reset the stream in between calls + s_push_layer(s, iso_hdr, 0); + + // Check a FIRST chunk is accepted... + stat = dyn_dechunker_process_first_chunk( + dc, s, 1600); + ck_assert_int_eq(stat, E_DYN_IN_PROGRESS); + + // ... and another FIRST chunk is an error + s_pop_layer(s, iso_hdr); + stat = dyn_dechunker_process_first_chunk( + dc, s, 1600); + ck_assert_int_eq(stat, E_DYN_ERROR); + + dyn_dechunker_free(dc); + free_stream(s); +} +END_TEST + + +/******************************************************************************/ +/** + * A FIRST chunk bigger than 1590 bytes but less than 1600 is passed + * through to the application, if it is the total length + */ +START_TEST(test_dyn_dechunker_first_inline) +{ + const char data[1591] = {0}; + + enum dyn_dechunker_status stat; + struct dyn_dechunker *dc; + struct stream *s; + + // Check a FIRST chunk of 1590 bytes with a total of 1590 is rejected + // (too small to fragment) + dc = dyn_dechunker_init("test"); + ck_assert_ptr_ne(dc, NULL); + s = make_stream_from_data(data, 1590); + ck_assert_ptr_ne(s, NULL); + stat = dyn_dechunker_process_first_chunk( dc, s, 1590); + ck_assert_int_eq(stat, E_DYN_ERROR); + dyn_dechunker_free(dc); + free_stream(s); + + // Check a FIRST chunk of 1591 bytes with a total size of 1591 is + // accepted as inline + dc = dyn_dechunker_init("test"); + ck_assert_ptr_ne(dc, NULL); + s = make_stream_from_data(data, 1591); + ck_assert_ptr_ne(s, NULL); + stat = dyn_dechunker_process_first_chunk( dc, s, 1591); + ck_assert_int_eq(stat, E_DYN_INLINE_CHUNK); + dyn_dechunker_free(dc); + free_stream(s); +} +END_TEST + + +/******************************************************************************/ +/** + * Checks that a FIRST chunk cannot be bigger than the total size + */ +START_TEST(test_dyn_dechunker_chunk_overflow) +{ + const char data[1592] = {0}; + + enum dyn_dechunker_status stat; + struct dyn_dechunker *dc; + struct stream *s; + + // We know a FIRST chunk of size 1591 for a total of 1591 is + // inline (see test_dyn_dechunker_first_inline()). Check if the + // first chunk is 1592, it is rejected + dc = dyn_dechunker_init("test"); + ck_assert_ptr_ne(dc, NULL); + s = make_stream_from_data(data, 1592); + ck_assert_ptr_ne(s, NULL); + stat = dyn_dechunker_process_first_chunk( dc, s, 1591); + ck_assert_int_eq(stat, E_DYN_ERROR); + dyn_dechunker_free(dc); + free_stream(s); +} +END_TEST + +/******************************************************************************/ +// Returns a stream with some random data, then a chunk of +// up to FRANKENSTEIN_DYN_CHUNK_COUNT bytes from Frankenstein chapter 1 +// The stream pointer will be positioned at the start of the text. +static struct stream * +make_dyn_bigtest_chunk(unsigned int chunk_num) +{ + struct stream *s = NULL; + + if (chunk_num < FRANKENSTEIN_DYN_CHUNK_COUNT) + { + int chunk_size; + // Work out the size of this chunk + if (chunk_num == (FRANKENSTEIN_DYN_CHUNK_COUNT - 1)) + { + chunk_size = sizeof(frankenstein) % FRANKENSTEIN_DYN_CHUNK_SIZE; + if (chunk_size == 0) + { + chunk_size = FRANKENSTEIN_DYN_CHUNK_SIZE; + } + } + else + { + chunk_size = FRANKENSTEIN_DYN_CHUNK_SIZE; + } + + // Add some random data at the start of the stream, so we + // can check the dechunker works for non-zero positioned + // streams + int rand_size = 4 * 4 * chunk_num; + + make_stream(s); + init_stream(s, rand_size + chunk_size); + + // Write the random data + int i; + for (i = 0 ; i < rand_size; ++i) + { + out_uint8(s, rand() & 255); + } + s_push_layer(s, iso_hdr, 0); + + // Copy the chapter data + out_uint8a(s, &frankenstein[chunk_num * FRANKENSTEIN_DYN_CHUNK_SIZE], + chunk_size); + + // Get the stream ready for reading from the Frankenstein text + s_mark_end(s); + s_pop_layer(s, iso_hdr); + } + + return s; +} + +/******************************************************************************/ +/* + * Streams a lot of data through the dechunker and checks it's all + * assembled correctly at the end + */ +START_TEST(test_dyn_dechunker_big_test) +{ + enum dyn_dechunker_status stat; + struct dyn_dechunker *dc = dyn_dechunker_init("test"); + ck_assert_ptr_ne(dc, NULL); + struct stream *s; + int pending; + + int i; + for (i = 0 ; i < FRANKENSTEIN_DYN_CHUNK_COUNT; ++i) + { + s = make_dyn_bigtest_chunk(i); + if (i == 0) + { + stat = dyn_dechunker_process_first_chunk( + dc, s, sizeof(frankenstein)); + } + else + { + stat = dyn_dechunker_process_data_chunk( dc, s); + } + pending = dyn_dechunker_pending(dc); + ck_assert_int_ne(pending, 0); + if (i < FRANKENSTEIN_DYN_CHUNK_COUNT - 1) + { + ck_assert_int_eq(stat, E_DYN_IN_PROGRESS); + } + else + { + ck_assert_int_eq(stat, E_DYN_READY); + } + free_stream(s); + } + + // Check we have a result + s = dyn_dechunker_get_stream(dc); + ck_assert_ptr_ne(s, NULL); + pending = dyn_dechunker_pending(dc); + ck_assert_int_eq(pending, 0); + + // Is it the right size? + int stream_size = s_rem(s); + ck_assert_int_eq(stream_size, sizeof(frankenstein)); + + // Check the data + const char *p; + in_uint8p(s, p, stream_size); + ck_assert_mem_eq(frankenstein, p, stream_size); + + free_stream(s); + dyn_dechunker_free(dc); +} + +/******************************************************************************/ +// Like test_dyn_dechunker_big_test, but the last chunk is oversized +START_TEST(test_dyn_dechunker_big_test_oversize_fail) +{ + enum dyn_dechunker_status stat; + struct dyn_dechunker *dc = dyn_dechunker_init("test"); + ck_assert_ptr_ne(dc, NULL); + struct stream *s; + + int i; + for (i = 0 ; i < FRANKENSTEIN_DYN_CHUNK_COUNT; ++i) + { + s = make_dyn_bigtest_chunk(i); + if (i == 0) + { + stat = dyn_dechunker_process_first_chunk( + dc, s, sizeof(frankenstein)); + } + else + { + if (i == (FRANKENSTEIN_DYN_CHUNK_COUNT - 1)) + { + // Add a byte to the end of the text in the chunk + struct stream *s2; + make_stream(s2); + init_stream(s2, s_rem(s) + 1); + s_push_layer(s2, iso_hdr, 0); + out_uint8p(s2, s->p, s_rem(s)); + out_uint8(s2, 'x'); + s_mark_end(s2); + s_pop_layer(s2, iso_hdr); // Rewind for reading + // Swap s2 and s and delete the original stream + struct stream *tmp = s; + s = s2; + free_stream(tmp); + } + stat = dyn_dechunker_process_data_chunk( dc, s); + } + if (i < FRANKENSTEIN_DYN_CHUNK_COUNT - 1) + { + ck_assert_int_eq(stat, E_DYN_IN_PROGRESS); + } + else + { + ck_assert_int_eq(stat, E_DYN_ERROR); + } + free_stream(s); + } + + // Check we do not have a result + s = dyn_dechunker_get_stream(dc); + ck_assert_ptr_eq(s, NULL); + + dyn_dechunker_free(dc); +} + +/******************************************************************************/ + Suite * make_suite_test_dechunker(void) { @@ -608,17 +957,27 @@ make_suite_test_dechunker(void) s = suite_create("dechunker"); - rc_dechunker = tcase_create("dechunker_basic"); + rc_dechunker = tcase_create("vc_dechunker"); suite_add_tcase(s, rc_dechunker); - tcase_add_test(rc_dechunker, test_dechunker_bad_params); - tcase_add_test(rc_dechunker, test_dechunker_passthrough); - tcase_add_test(rc_dechunker, test_dechunker_intermediate); - tcase_add_test(rc_dechunker, test_dechunker_last); - tcase_add_test(rc_dechunker, test_dechunker_first_first); - tcase_add_test(rc_dechunker, test_dechunker_chunk_overflow); - tcase_add_test(rc_dechunker, test_dechunker_big_test); - tcase_add_test(rc_dechunker, test_dechunker_big_test_oversize_fail); - tcase_add_test(rc_dechunker, test_dechunker_big_test_undersize_fail); + tcase_add_test(rc_dechunker, test_vc_dechunker_bad_params); + tcase_add_test(rc_dechunker, test_vc_dechunker_passthrough); + tcase_add_test(rc_dechunker, test_vc_dechunker_intermediate); + tcase_add_test(rc_dechunker, test_vc_dechunker_last); + tcase_add_test(rc_dechunker, test_vc_dechunker_first_first); + tcase_add_test(rc_dechunker, test_vc_dechunker_chunk_overflow); + tcase_add_test(rc_dechunker, test_vc_dechunker_big_test); + tcase_add_test(rc_dechunker, test_vc_dechunker_big_test_oversize_fail); + tcase_add_test(rc_dechunker, test_vc_dechunker_big_test_undersize_fail); + + rc_dechunker = tcase_create("dyn_dechunker"); + suite_add_tcase(s, rc_dechunker); + tcase_add_test(rc_dechunker, test_dyn_dechunker_bad_params); + tcase_add_test(rc_dechunker, test_dyn_dechunker_passthrough); + tcase_add_test(rc_dechunker, test_dyn_dechunker_first_first); + tcase_add_test(rc_dechunker, test_dyn_dechunker_first_inline); + tcase_add_test(rc_dechunker, test_dyn_dechunker_chunk_overflow); + tcase_add_test(rc_dechunker, test_dyn_dechunker_big_test); + tcase_add_test(rc_dechunker, test_dyn_dechunker_big_test_oversize_fail); return s; } From f745c9152d4967f0e8fc4c935e7538948997254d Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:43:19 +0100 Subject: [PATCH 3/7] drdynvc: Change channel processing to use streams The channel processor in xrdp_channel.c for dynamic streams uses a data pointer and a length for passing PDUs or PDU fragments. We replace this with a standard stream pointer, so that the usual facilities can be used for checking length violcations. --- libxrdp/libxrdp.h | 6 ++-- libxrdp/libxrdpinc.h | 6 ++-- libxrdp/xrdp_channel.c | 5 ++-- xrdp/xrdp_egfx.c | 19 +++++------- xrdp/xrdp_mm.c | 68 +++++++++++++++++++----------------------- 5 files changed, 46 insertions(+), 58 deletions(-) diff --git a/libxrdp/libxrdp.h b/libxrdp/libxrdp.h index 5aa6075c..80593c4c 100644 --- a/libxrdp/libxrdp.h +++ b/libxrdp/libxrdp.h @@ -146,9 +146,9 @@ struct xrdp_drdynvc int (*open_response)(struct xrdp_process *id, int chan_id, int creation_status); int (*close_response)(struct xrdp_process *id, int chan_id); - int (*data_first)(struct xrdp_process *id, int chan_id, char *data, - int bytes, int total_bytes); - int (*data)(struct xrdp_process *id, int chan_id, char *data, int bytes); + int (*data_first)(struct xrdp_process *id, int chan_id, struct stream *s, + int total_bytes); + int (*data)(struct xrdp_process *id, int chan_id, struct stream *s); }; struct vc_dechunker; // Forward declaration diff --git a/libxrdp/libxrdpinc.h b/libxrdp/libxrdpinc.h index d84b544a..0b48d356 100644 --- a/libxrdp/libxrdpinc.h +++ b/libxrdp/libxrdpinc.h @@ -91,9 +91,9 @@ struct xrdp_drdynvc_procs int (*open_response)(struct xrdp_process *id, int chan_id, int creation_status); int (*close_response)(struct xrdp_process *id, int chan_id); - int (*data_first)(struct xrdp_process *id, int chan_id, - char *data, int bytes, int total_bytes); - int (*data)(struct xrdp_process *id, int chan_id, char *data, int bytes); + int (*data_first)(struct xrdp_process *id, int chan_id, struct stream *s, + int total_bytes); + int (*data)(struct xrdp_process *id, int chan_id, struct stream *s); }; /* Defined in xrdp_client_info.h */ diff --git a/libxrdp/xrdp_channel.c b/libxrdp/xrdp_channel.c index 1925b0d1..46202b86 100644 --- a/libxrdp/xrdp_channel.c +++ b/libxrdp/xrdp_channel.c @@ -488,8 +488,7 @@ drdynvc_process_data_first(struct xrdp_channel *self, drdynvc = self->drdynvcs + chan_id; if (drdynvc->data_first != NULL) { - return drdynvc->data_first(session->id, chan_id, s->p, - bytes, total_bytes); + return drdynvc->data_first(session->id, chan_id, s, total_bytes); } LOG_DEVEL(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): " "callback 'data_first' is NULL", @@ -530,7 +529,7 @@ drdynvc_process_data(struct xrdp_channel *self, drdynvc = self->drdynvcs + chan_id; if (drdynvc->data != NULL) { - return drdynvc->data(session->id, chan_id, s->p, bytes); + return drdynvc->data(session->id, chan_id, s); } LOG_DEVEL(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): " "callback 'data' is NULL", diff --git a/xrdp/xrdp_egfx.c b/xrdp/xrdp_egfx.c index 255f7646..6cc381ae 100644 --- a/xrdp/xrdp_egfx.c +++ b/xrdp/xrdp_egfx.c @@ -946,9 +946,10 @@ xrdp_egfx_close_response(struct xrdp_process *id, int chan_id) /* from client */ static int xrdp_egfx_data_first(struct xrdp_process *id, int chan_id, - char *data, int bytes, int total_bytes) + struct stream *s, int total_bytes) { struct xrdp_egfx *egfx; + int bytes = s_rem(s); LOG(LOG_LEVEL_TRACE, "xrdp_egfx_data_first: bytes %d" " total_bytes %d", bytes, total_bytes); @@ -962,17 +963,16 @@ xrdp_egfx_data_first(struct xrdp_process *id, int chan_id, make_stream(egfx->s); // Caller has checked total_bytes is >= 0 and bytes is < total_bytes init_stream(egfx->s, total_bytes); - out_uint8a(egfx->s, data, bytes); + out_uint8a(egfx->s, s->p, bytes); return 0; } /******************************************************************************/ /* from client */ static int -xrdp_egfx_data(struct xrdp_process *id, int chan_id, char *data, int bytes) +xrdp_egfx_data(struct xrdp_process *id, int chan_id, struct stream *s) { int error; - struct stream ls; struct xrdp_wm *wm; struct xrdp_mm *mm; struct xrdp_egfx *egfx; @@ -1004,20 +1004,17 @@ xrdp_egfx_data(struct xrdp_process *id, int chan_id, char *data, int bytes) if (egfx->s == NULL) { - g_memset(&ls, 0, sizeof(ls)); - ls.data = data; - ls.size = bytes; - ls.p = data; - ls.end = data + bytes; - return xrdp_egfx_process(egfx, &ls); + return xrdp_egfx_process(egfx, s); } + int bytes = s_rem(s); + if (!s_check_rem_out(egfx->s, bytes)) { LOG(LOG_LEVEL_ERROR, "DYNVC_DATA PDU data overflow on channel %d", chan_id); return 1; } - out_uint8a(egfx->s, data, bytes); + out_uint8a(egfx->s, s->p, bytes); if (!s_check_rem_out(egfx->s, 1)) { s_mark_end(egfx->s); diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 0c3b7306..def835df 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -1047,7 +1047,7 @@ dynamic_monitor_close_response(struct xrdp_process *id, int chan_id) /******************************************************************************/ static int dynamic_monitor_data_first(struct xrdp_process *id, int chan_id, - char *data, int bytes, int total_bytes) + struct stream *s, int total_bytes) { LOG_DEVEL(LOG_LEVEL_TRACE, "dynamic_monitor_data_first:"); return 0; @@ -1529,12 +1529,9 @@ add_resize_request_to_queue(struct xrdp_mm *self, /******************************************************************************/ static int -dynamic_monitor_data(struct xrdp_process *id, int chan_id, - char *data, int bytes) +dynamic_monitor_data(struct xrdp_process *id, int chan_id, struct stream *s) { int error = 0; - struct stream ls; - struct stream *s; int msg_type; int msg_length; struct xrdp_wm *wm; @@ -1552,12 +1549,6 @@ dynamic_monitor_data(struct xrdp_process *id, int chan_id, return error; } - g_memset(&ls, 0, sizeof(ls)); - ls.data = data; - ls.p = ls.data; - ls.size = bytes; - ls.end = ls.data + bytes; - s = &ls; in_uint32_le(s, msg_type); in_uint32_le(s, msg_length); LOG_DEVEL(LOG_LEVEL_DEBUG, @@ -2156,65 +2147,66 @@ xrdp_mm_drdynvc_close_response(struct xrdp_process *id, int chan_id) /*****************************************************************************/ /* part data from client going to channel server */ static int -xrdp_mm_drdynvc_data_first(struct xrdp_process *id, int chan_id, char *data, - int bytes, int total_bytes) +xrdp_mm_drdynvc_data_first(struct xrdp_process *id, int chan_id, + struct stream *s, int total_bytes) { struct trans *trans; - struct stream *s; + struct stream *out_s; struct xrdp_wm *wm; int chansrv_chan_id; + int bytes = s_rem(s); // Size of PDU sent to chansrv int pdu_size = 8 + 8 + 4 + 4 + 4 + bytes; wm = id->wm; trans = wm->mm->chan_trans; - s = trans_get_out_s(trans, pdu_size); - if (s == NULL) + out_s = trans_get_out_s(trans, pdu_size); + if (out_s == NULL) { return 1; } - out_uint32_le(s, 0); /* version */ - out_uint32_le(s, pdu_size); - out_uint32_le(s, 17); /* msg id */ - out_uint32_le(s, pdu_size - 8); + out_uint32_le(out_s, 0); /* version */ + out_uint32_le(out_s, pdu_size); + out_uint32_le(out_s, 17); /* msg id */ + out_uint32_le(out_s, pdu_size - 8); chansrv_chan_id = wm->mm->xr2cr_cid_map[chan_id]; - out_uint32_le(s, chansrv_chan_id); - out_uint32_le(s, bytes); - out_uint32_le(s, total_bytes); - out_uint8a(s, data, bytes); - s_mark_end(s); + out_uint32_le(out_s, chansrv_chan_id); + out_uint32_le(out_s, bytes); + out_uint32_le(out_s, total_bytes); + out_uint8p(out_s, s->p, bytes); + s_mark_end(out_s); return trans_write_copy(trans); } /*****************************************************************************/ /* data from client going to channel server */ static int -xrdp_mm_drdynvc_data(struct xrdp_process *id, int chan_id, - char *data, int bytes) +xrdp_mm_drdynvc_data(struct xrdp_process *id, int chan_id, struct stream *s) { struct trans *trans; - struct stream *s; + struct stream *out_s; struct xrdp_wm *wm; int chansrv_chan_id; + int bytes = s_rem(s); // Size of PDU sent to chansrv int pdu_size = 8 + 8 + 4 + 4 + bytes; wm = id->wm; trans = wm->mm->chan_trans; - s = trans_get_out_s(trans, pdu_size); - if (s == NULL) + out_s = trans_get_out_s(trans, pdu_size); + if (out_s == NULL) { return 1; } - out_uint32_le(s, 0); /* version */ - out_uint32_le(s, pdu_size); - out_uint32_le(s, 19); /* msg id */ - out_uint32_le(s, pdu_size - 8); + out_uint32_le(out_s, 0); /* version */ + out_uint32_le(out_s, pdu_size); + out_uint32_le(out_s, 19); /* msg id */ + out_uint32_le(out_s, pdu_size - 8); chansrv_chan_id = wm->mm->xr2cr_cid_map[chan_id]; - out_uint32_le(s, chansrv_chan_id); - out_uint32_le(s, bytes); - out_uint8a(s, data, bytes); - s_mark_end(s); + out_uint32_le(out_s, chansrv_chan_id); + out_uint32_le(out_s, bytes); + out_uint8p(out_s, s->p, bytes); + s_mark_end(out_s); return trans_write_copy(trans); } From 63afb676e6ab9f40a918dee92ec7ff0f7da74c33 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 7 Aug 2026 12:04:36 +0100 Subject: [PATCH 4/7] drdynvc: Improve dynamic channel support The dynamic channel handler in xrdp_channel.c is updated to allow the procs `data_first` pointer to be NULL. If this is done, the channel handler performs all the dechunking necessary for the channel, and only complete data PDUs are passed to procs 'data' callback. This facility is applied to the dynamic channels supported by xrdp_mm.c. The incoming callbacks for these channels now provide complete support for the specification in [MS-RDPEDYC]. The existing channels were incomplete in these respects: 1) The "Microsoft::Windows::RDS::Graphics" channel handler did not support incoming PDUs between 1591 and 1600 bytes. The specification calls for these to be sent as a single DATA_FIRST PDU. 2) The "Microsoft::Windows::RDS::DisplayControl" channel handler did not support incoming PDUs over 1590 bytes. --- common/xrdp_constants.h | 3 + libxrdp/libxrdp.h | 3 +- libxrdp/libxrdpinc.h | 3 + libxrdp/xrdp_channel.c | 333 ++++++++++++++++++++++++++++------------ xrdp/xrdp_egfx.c | 52 +------ xrdp/xrdp_egfx.h | 1 - xrdp/xrdp_mm.c | 11 +- 7 files changed, 242 insertions(+), 164 deletions(-) diff --git a/common/xrdp_constants.h b/common/xrdp_constants.h index 8983d922..77398829 100644 --- a/common/xrdp_constants.h +++ b/common/xrdp_constants.h @@ -87,6 +87,9 @@ #define XRDP_MAX_BITMAP_CACHE_IDX 2000 #define XRDP_BITMAP_CACHE_ENTRIES 2048 +/* Max number of dynamic channels we support */ +#define DRDYNVC_CHANNEL_COUNT 256 + /* * Constants come from ITU-T Recommendations */ diff --git a/libxrdp/libxrdp.h b/libxrdp/libxrdp.h index 80593c4c..2663bf92 100644 --- a/libxrdp/libxrdp.h +++ b/libxrdp/libxrdp.h @@ -143,6 +143,7 @@ struct xrdp_drdynvc int status; /* see XRDP_DRDYNVC_STATUS_* */ int flags; int pad0; + struct dyn_dechunker *dc; // Use to dechunk fragments int (*open_response)(struct xrdp_process *id, int chan_id, int creation_status); int (*close_response)(struct xrdp_process *id, int chan_id); @@ -161,7 +162,7 @@ struct xrdp_channel int drdynvc_channel_id; int drdynvc_state; struct vc_dechunker *drdynvc_dc; - struct xrdp_drdynvc drdynvcs[256]; + struct xrdp_drdynvc drdynvcs[DRDYNVC_CHANNEL_COUNT]; }; /* rdp */ diff --git a/libxrdp/libxrdpinc.h b/libxrdp/libxrdpinc.h index 0b48d356..0745d594 100644 --- a/libxrdp/libxrdpinc.h +++ b/libxrdp/libxrdpinc.h @@ -91,6 +91,9 @@ struct xrdp_drdynvc_procs int (*open_response)(struct xrdp_process *id, int chan_id, int creation_status); int (*close_response)(struct xrdp_process *id, int chan_id); + // Only set data_first if you want to be responsible for + // defragging your own PDUs. Otherwise the channel + // process will do it for you. int (*data_first)(struct xrdp_process *id, int chan_id, struct stream *s, int total_bytes); int (*data)(struct xrdp_process *id, int chan_id, struct stream *s); diff --git a/libxrdp/xrdp_channel.c b/libxrdp/xrdp_channel.c index 46202b86..09926a57 100644 --- a/libxrdp/xrdp_channel.c +++ b/libxrdp/xrdp_channel.c @@ -74,11 +74,16 @@ xrdp_channel_create(struct xrdp_sec *owner, struct xrdp_mcs *mcs_layer) void xrdp_channel_delete(struct xrdp_channel *self) { + int i; if (self == 0) { return; } vc_dechunker_free(self->drdynvc_dc); + for (i = 0 ; i < DRDYNVC_CHANNEL_COUNT ; ++i) + { + dyn_dechunker_free(self->drdynvcs[i].dc); + } g_memset(self, 0, sizeof(struct xrdp_channel)); g_free(self); } @@ -338,10 +343,10 @@ drdynvc_process_open_channel_response(struct xrdp_channel *self, in_uint32_le(s, creation_status); /* CreationStatus */ LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_CREATE_RSP " "ChannelId %d, CreationStatus %d", chan_id, creation_status); - if (chan_id > 255) + if (chan_id >= DRDYNVC_CHANNEL_COUNT) { - LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_CREATE_RSP for an " - "invalid channel id. Max allowed 255, received %d", chan_id); + LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_CREATE_RSP " + "for an invalid channel id %d", chan_id); return 1; } @@ -354,6 +359,8 @@ drdynvc_process_open_channel_response(struct xrdp_channel *self, else { drdynvc->status = XRDP_DRDYNVC_STATUS_CLOSED; + dyn_dechunker_free(drdynvc->dc); + drdynvc->dc = NULL; } LOG_DEVEL(LOG_LEVEL_DEBUG, "Dynamic Virtual Channel %s (%d) updated: status = %s", @@ -392,15 +399,28 @@ drdynvc_process_close_channel_response(struct xrdp_channel *self, LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_CLOSE " "ChannelId %d", chan_id); session = self->sec_layer->rdp_layer->session; - if (chan_id > 255) + if (chan_id >= DRDYNVC_CHANNEL_COUNT) { LOG(LOG_LEVEL_ERROR, "Received message for an invalid " - "channel id. channel id %d", chan_id); + "channel id %d", chan_id); return 1; } drdynvc = self->drdynvcs + chan_id; + + if (dyn_dechunker_pending(drdynvc->dc)) + { + // The last PDU wasn't completed + LOG(LOG_LEVEL_WARNING, + "Dynamic Virtual Channel %s (%d) closing with outstanding data", + XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id), + chan_id); + } + drdynvc->status = XRDP_DRDYNVC_STATUS_CLOSED; + dyn_dechunker_free(drdynvc->dc); + drdynvc->dc = NULL; + LOG_DEVEL(LOG_LEVEL_DEBUG, "Dynamic Virtual Channel %s (%d) updated: status = %s", XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id), @@ -426,75 +446,113 @@ static int drdynvc_process_data_first(struct xrdp_channel *self, int cmd, struct stream *s) { - struct xrdp_session *session; uint32_t chan_id; - int len; - int bytes; - int total_bytes; - struct xrdp_drdynvc *drdynvc; - + int rv = 0; if (drdynvc_get_chan_id(s, cmd, &chan_id) != 0) /* ChannelId */ { LOG(LOG_LEVEL_ERROR, - "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST failed"); - return 1; - } - len = (cmd >> 2) & 0x03; - if (len == 0) - { - if (!s_check_rem_and_log(s, 1, "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST")) - { - return 1; - } - in_uint8(s, total_bytes); /* Length */ - } - else if (len == 1) - { - if (!s_check_rem_and_log(s, 2, "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST")) - { - return 1; - } - in_uint16_le(s, total_bytes); /* Length */ + "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST channel ID failed"); + rv = 1; } else { - if (!s_check_rem_and_log(s, 4, "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST")) + int len = (cmd >> 2) & 0x03; + int total_bytes; + switch (len) { - return 1; + case 0: + // Technically this can't happen as DATA_FIRST is only used for + // PDUs over 1590 bytes ([MS-RDPEDYC] 2.2.3) + if (!s_check_rem(s, 1)) + { + goto short_pdu; + } + in_uint8(s, total_bytes); /* Length */ + break; + + case 1: + if (!s_check_rem(s, 2)) + { + goto short_pdu; + } + in_uint16_le(s, total_bytes); /* Length */ + break; + + case 2: + if (!s_check_rem(s, 4)) + { + goto short_pdu; + } + in_uint32_le(s, total_bytes); /* Length */ + break; + + default: + LOG(LOG_LEVEL_ERROR, + "[MS-RDPEDYC] DYNVC_DATA_FIRST has bad Len field"); + return 1; } - in_uint32_le(s, total_bytes); /* Length */ - } - bytes = (int) (s->end - s->p); - LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_DATA_FIRST " - "ChannelId %d, Length %d, Data (omitted from the log)", - chan_id, total_bytes); - // See [MS-RDPBCGR] 2.2.3 - if (total_bytes < 1590 || bytes > total_bytes) - { - LOG(LOG_LEVEL_ERROR, - "Badly formed DYNVC_DATA_FIRST PDU received on dynamic channel %d", - chan_id); - return 1; + LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_DATA_FIRST " + "ChannelId %d, Length %d, Data (omitted from the log)", + chan_id, total_bytes); + + if (chan_id >= DRDYNVC_CHANNEL_COUNT) + { + LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_DATA_FIRST " + "for an invalid channel id %d", chan_id); + rv = 1; + } + else + { + struct xrdp_drdynvc *drdynvc = self->drdynvcs + chan_id; + + if (drdynvc->data_first != NULL) + { + // Caller has requested to defragment PDUs themself + struct xrdp_session *session = + self->sec_layer->rdp_layer->session; + rv = drdynvc->data_first(session->id, chan_id, s, total_bytes); + } + else + { + // Get the dechunker working on the stream + enum dyn_dechunker_status status; + status = dyn_dechunker_process_first_chunk(drdynvc->dc, + s, total_bytes); + switch (status) + { + case E_DYN_INLINE_CHUNK: + // Pass through as data PDU + if (drdynvc->data != NULL) + { + struct xrdp_session *session = + self->sec_layer->rdp_layer->session; + rv = drdynvc->data(session->id, chan_id, s); + } + break; + + case E_DYN_IN_PROGRESS: + break; + + case E_DYN_ERROR: + rv = 1; + break; + + default: + LOG(LOG_LEVEL_ERROR, + "Dechunker returned unknown error %d", + (int)status); + rv = 1; + } + } + } } - session = self->sec_layer->rdp_layer->session; - if (chan_id > 255) - { - LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_DATA_FIRST for an " - "invalid channel id. Max allowed 255, received %d", chan_id); - return 1; - } - drdynvc = self->drdynvcs + chan_id; - if (drdynvc->data_first != NULL) - { - return drdynvc->data_first(session->id, chan_id, s, total_bytes); - } - LOG_DEVEL(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): " - "callback 'data_first' is NULL", - XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id), - chan_id); - return 0; + return rv; + +short_pdu: + LOG(LOG_LEVEL_ERROR, "[MS-RDPEDYC] DYNVC_DATA_FIRST is too short"); + return 1; } /*****************************************************************************/ @@ -505,37 +563,86 @@ static int drdynvc_process_data(struct xrdp_channel *self, int cmd, struct stream *s) { - struct xrdp_session *session; + int rv = 0; uint32_t chan_id; - int bytes; - struct xrdp_drdynvc *drdynvc; if (drdynvc_get_chan_id(s, cmd, &chan_id) != 0) /* ChannelId */ { - LOG(LOG_LEVEL_ERROR, "drdynvc_process_data: drdynvc_get_chan_id failed"); - return 1; + LOG(LOG_LEVEL_ERROR, + "Parsing [MS-RDPEDYC] DYNVC_DATA channel ID failed"); + rv = 1; } - bytes = (int) (s->end - s->p); - LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_DATA " - "ChannelId %d, (re-assembled) Length %d, Data (omitted from the log)", - chan_id, bytes); - session = self->sec_layer->rdp_layer->session; - if (chan_id > 255) + else { - LOG(LOG_LEVEL_ERROR, "Received DYNVC_DATA PDU for an invalid " - "channel id. channel id %d", chan_id); - return 1; + LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_DATA " + "ChannelId %d, Length %d, Data (omitted from the log)", + chan_id, s_rem(s)); + if (chan_id >= DRDYNVC_CHANNEL_COUNT) + { + LOG(LOG_LEVEL_ERROR, "Received DYNVC_DATA PDU for an invalid " + "channel id %d", chan_id); + rv = 1; + } + else + { + struct stream *ls = NULL; // Set if the application to be called + int free_ls = 0; // Set if we need to clear ls when we're done + struct xrdp_drdynvc *drdynvc = self->drdynvcs + chan_id; + if (drdynvc->data_first != NULL) + { + // Caller is processing all PDUs directly + ls = s; + } + else + { + // Pass the PDU to the dechunker + enum dyn_dechunker_status dechunker_status = + dyn_dechunker_process_data_chunk(drdynvc->dc, s); + switch (dechunker_status) + { + case E_DYN_INLINE_CHUNK: + ls = s; + break; + + case E_DYN_IN_PROGRESS: + break; + + case E_DYN_READY: + ls = dyn_dechunker_get_stream(drdynvc->dc); + // We now own the stream, so must delete it + free_ls = 1; + break; + + case E_DYN_ERROR: + // Error has been logged + rv = 1; + break; + + default: + LOG(LOG_LEVEL_ERROR, + "Dechunker returned unknown error %d", + (int)dechunker_status); + rv = 1; + } + } + + if (ls != NULL) + { + if (drdynvc->data != NULL) + { + struct xrdp_session *session = + self->sec_layer->rdp_layer->session; + rv = drdynvc->data(session->id, chan_id, ls); + } + + if (free_ls) + { + free_stream(ls); + } + } + } } - drdynvc = self->drdynvcs + chan_id; - if (drdynvc->data != NULL) - { - return drdynvc->data(session->id, chan_id, s); - } - LOG_DEVEL(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): " - "callback 'data' is NULL", - XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id), - chan_id); - return 0; + return rv; } /*****************************************************************************/ @@ -819,6 +926,7 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name, int total_data_len; int static_flags; char *cmd_ptr; + struct dyn_dechunker *dc = NULL; make_stream(s); init_stream(s, 8192); @@ -826,8 +934,18 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name, { LOG(LOG_LEVEL_ERROR, "xrdp_channel_drdynvc_open: xrdp_channel_init failed"); - free_stream(s); - return 1; + goto cleanup; + } + + // If there's no 'data_first' proc, we need a dechunker for the + // channel + if (procs->data_first == NULL) + { + if ((dc = dyn_dechunker_init(name)) == NULL) + { + // Error logged + goto cleanup; + } } cmd_ptr = s->p; out_uint8(s, 0); /* set later */ @@ -835,14 +953,12 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name, while (self->drdynvcs[ChId].status != XRDP_DRDYNVC_STATUS_CLOSED) { ChId++; - if (ChId > 255) + if (ChId >= DRDYNVC_CHANNEL_COUNT) { LOG(LOG_LEVEL_ERROR, "Attempting to create a new channel when the maximum " - "number of channels have already been created. " - "XRDP only supports 255 open channels."); - free_stream(s); - return 1; + "number of channels have already been created."); + goto cleanup; } } cbChId = drdynvc_insert_uint_124(s, ChId); /* ChannelId */ @@ -864,17 +980,30 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name, { LOG(LOG_LEVEL_ERROR, "Sending [MS-RDPEDYC] DYNVC_CREATE_REQ failed"); - free_stream(s); - return 1; + goto cleanup; } - free_stream(s); + + if (procs->data == NULL) + { + // Not expected. Log this once. + LOG(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): " + "callback 'data' is NULL", name, ChId); + } + *chan_id = ChId; self->drdynvcs[ChId].open_response = procs->open_response; self->drdynvcs[ChId].close_response = procs->close_response; self->drdynvcs[ChId].data_first = procs->data_first; self->drdynvcs[ChId].data = procs->data; self->drdynvcs[ChId].status = XRDP_DRDYNVC_STATUS_OPEN_SENT; + self->drdynvcs[ChId].dc = dc; + free_stream(s); return 0; + +cleanup: + free_stream(s); + dyn_dechunker_free(dc); + return 1; } /*****************************************************************************/ @@ -892,9 +1021,9 @@ xrdp_channel_drdynvc_close(struct xrdp_channel *self, int chan_id) int static_flags; char *cmd_ptr; - if ((chan_id < 0) || (chan_id > 255)) + if ((chan_id < 0) || (chan_id >= DRDYNVC_CHANNEL_COUNT)) { - LOG(LOG_LEVEL_ERROR, "Attempting to close an invalid channel id. " + LOG(LOG_LEVEL_ERROR, "Attempting to close an invalid " "channel id %d", chan_id); return 1; } @@ -962,10 +1091,10 @@ xrdp_channel_drdynvc_data_first(struct xrdp_channel *self, int chan_id, int static_flags; char *cmd_ptr; - if ((chan_id < 0) || (chan_id > 255)) + if ((chan_id < 0) || (chan_id >= DRDYNVC_CHANNEL_COUNT)) { LOG(LOG_LEVEL_ERROR, "Attempting to send data to an invalid " - "channel id. channel id %d", chan_id); + "channel id %d", chan_id); return 1; } if (self->drdynvcs[chan_id].status != XRDP_DRDYNVC_STATUS_OPEN) @@ -1034,10 +1163,10 @@ xrdp_channel_drdynvc_data(struct xrdp_channel *self, int chan_id, int static_flags; char *cmd_ptr; - if ((chan_id < 0) || (chan_id > 255)) + if ((chan_id < 0) || (chan_id >= DRDYNVC_CHANNEL_COUNT)) { LOG(LOG_LEVEL_ERROR, "Attempting to send data to an invalid " - "channel id. channel id %d", chan_id); + "channel id %d", chan_id); return 1; } if (self->drdynvcs[chan_id].status != XRDP_DRDYNVC_STATUS_OPEN) diff --git a/xrdp/xrdp_egfx.c b/xrdp/xrdp_egfx.c index 6cc381ae..84f33790 100644 --- a/xrdp/xrdp_egfx.c +++ b/xrdp/xrdp_egfx.c @@ -942,37 +942,11 @@ xrdp_egfx_close_response(struct xrdp_process *id, int chan_id) return 0; } -/******************************************************************************/ -/* from client */ -static int -xrdp_egfx_data_first(struct xrdp_process *id, int chan_id, - struct stream *s, int total_bytes) -{ - struct xrdp_egfx *egfx; - int bytes = s_rem(s); - - LOG(LOG_LEVEL_TRACE, "xrdp_egfx_data_first: bytes %d" - " total_bytes %d", bytes, total_bytes); - egfx = id->wm->mm->egfx; - if (egfx->s != NULL) - { - LOG(LOG_LEVEL_ERROR, "DYNVC_DATA_FIRST PDU received while" - " another stream is active on channel %d", chan_id); - return 1; - } - make_stream(egfx->s); - // Caller has checked total_bytes is >= 0 and bytes is < total_bytes - init_stream(egfx->s, total_bytes); - out_uint8a(egfx->s, s->p, bytes); - return 0; -} - /******************************************************************************/ /* from client */ static int xrdp_egfx_data(struct xrdp_process *id, int chan_id, struct stream *s) { - int error; struct xrdp_wm *wm; struct xrdp_mm *mm; struct xrdp_egfx *egfx; @@ -1002,29 +976,7 @@ xrdp_egfx_data(struct xrdp_process *id, int chan_id, struct stream *s) return 0; } - if (egfx->s == NULL) - { - return xrdp_egfx_process(egfx, s); - } - int bytes = s_rem(s); - - if (!s_check_rem_out(egfx->s, bytes)) - { - LOG(LOG_LEVEL_ERROR, "DYNVC_DATA PDU data overflow on channel %d", - chan_id); - return 1; - } - out_uint8a(egfx->s, s->p, bytes); - if (!s_check_rem_out(egfx->s, 1)) - { - s_mark_end(egfx->s); - egfx->s->p = egfx->s->data; - error = xrdp_egfx_process(egfx, egfx->s); - free_stream(egfx->s); - egfx->s = NULL; - return error; - } - return 0; + return xrdp_egfx_process(egfx, s); } /******************************************************************************/ @@ -1045,7 +997,7 @@ xrdp_egfx_create(struct xrdp_mm *mm, struct xrdp_egfx **egfx) } procs.open_response = xrdp_egfx_open_response; procs.close_response = xrdp_egfx_close_response; - procs.data_first = xrdp_egfx_data_first; + procs.data_first = NULL; // Defragging handled elsewere procs.data = xrdp_egfx_data; process = mm->wm->pro_layer; error = libxrdp_drdynvc_open(process->session, diff --git a/xrdp/xrdp_egfx.h b/xrdp/xrdp_egfx.h index 18d32c90..455f15d2 100644 --- a/xrdp/xrdp_egfx.h +++ b/xrdp/xrdp_egfx.h @@ -127,7 +127,6 @@ struct xrdp_egfx int channel_id; int surface_id; int frame_id; - struct stream *s; void *user; struct xrdp_egfx_bulk *bulk; int (*caps_advertise)(void *user, int num_caps, int *version, int *flags); diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index def835df..4b6489db 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -1044,15 +1044,6 @@ dynamic_monitor_close_response(struct xrdp_process *id, int chan_id) return 0; } -/******************************************************************************/ -static int -dynamic_monitor_data_first(struct xrdp_process *id, int chan_id, - struct stream *s, int total_bytes) -{ - LOG_DEVEL(LOG_LEVEL_TRACE, "dynamic_monitor_data_first:"); - return 0; -} - /******************************************************************************/ int advance_resize_state_machine(struct xrdp_mm *mm, @@ -1967,7 +1958,7 @@ dynamic_monitor_initialize(struct xrdp_mm *self) g_memset(&d_procs, 0, sizeof(d_procs)); d_procs.open_response = dynamic_monitor_open_response; d_procs.close_response = dynamic_monitor_close_response; - d_procs.data_first = dynamic_monitor_data_first; + d_procs.data_first = NULL; // Defragging handled elsewhere d_procs.data = dynamic_monitor_data; flags = 0; error = libxrdp_drdynvc_open(self->wm->session, From 77d9b494324a111ad05d5ea7bea4262627335837 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:53:13 +0100 Subject: [PATCH 5/7] chansrv: Use streams for dynamic channel processing Change the dynamic channel processing to use streams rather than a data pointer and a length. This mirrors an earlier commit for xrdp. The reason for the change is to make it easier to check for buffer overflows using standard stream features. --- sesman/chansrv/audin.c | 24 ++++++---------- sesman/chansrv/chansrv.c | 60 +++++++++++++++------------------------- sesman/chansrv/chansrv.h | 4 +-- xrdp/xrdp_mm.c | 8 +++--- 4 files changed, 37 insertions(+), 59 deletions(-) diff --git a/sesman/chansrv/audin.c b/sesman/chansrv/audin.c index 2aa5ac03..9c59310c 100644 --- a/sesman/chansrv/audin.c +++ b/sesman/chansrv/audin.c @@ -429,10 +429,10 @@ audin_close_response(int chan_id) /*****************************************************************************/ static int -audin_data_fragment(int chan_id, char *data, int bytes) +audin_data_fragment(int chan_id, struct stream *s) { int rv; - + int bytes = s_rem(s); LOG_DEVEL(LOG_LEVEL_DEBUG, "audin_data_fragment:"); if (!s_check_rem(g_in_s, bytes)) { @@ -440,7 +440,7 @@ audin_data_fragment(int chan_id, char *data, int bytes) bytes, (int) (g_in_s->end - g_in_s->p)); return 1; } - out_uint8a(g_in_s, data, bytes); + out_uint8a(g_in_s, s->p, bytes); if (g_in_s->p == g_in_s->end) { g_in_s->p = g_in_s->data; @@ -454,7 +454,7 @@ audin_data_fragment(int chan_id, char *data, int bytes) /*****************************************************************************/ static int -audin_data_first(int chan_id, char *data, int bytes, int total_bytes) +audin_data_first(int chan_id, struct stream *s, int total_bytes) { LOG_DEVEL(LOG_LEVEL_DEBUG, "audin_data_first:"); if (g_in_s != NULL) @@ -465,25 +465,19 @@ audin_data_first(int chan_id, char *data, int bytes, int total_bytes) make_stream(g_in_s); init_stream(g_in_s, total_bytes); g_in_s->end = g_in_s->data + total_bytes; - return audin_data_fragment(chan_id, data, bytes); + return audin_data_fragment(chan_id, s); } /*****************************************************************************/ static int -audin_data(int chan_id, char *data, int bytes) +audin_data(int chan_id, struct stream *s) { - struct stream ls; - - LOG_DEVEL_HEXDUMP(LOG_LEVEL_TRACE, "audin_data:", data, bytes); + LOG_DEVEL_HEXDUMP(LOG_LEVEL_TRACE, "audin_data:", s->p, s_rem(s)); if (g_in_s == NULL) { - g_memset(&ls, 0, sizeof(ls)); - ls.data = data; - ls.p = ls.data; - ls.end = ls.p + bytes; - return audin_process_msg(chan_id, &ls); + return audin_process_msg(chan_id, s); } - return audin_data_fragment(chan_id, data, bytes); + return audin_data_fragment(chan_id, s); } /*****************************************************************************/ diff --git a/sesman/chansrv/chansrv.c b/sesman/chansrv/chansrv.c index dbadfc5f..96ec3e1d 100644 --- a/sesman/chansrv/chansrv.c +++ b/sesman/chansrv/chansrv.c @@ -100,8 +100,8 @@ struct chansrv_drdynvc int pad0; int (*open_response)(int chan_id, int creation_status); int (*close_response)(int chan_id); - int (*data_first)(int chan_id, char *data, int bytes, int total_bytes); - int (*data)(int chan_id, char *data, int bytes); + int (*data_first)(int chan_id, struct stream *s, int total_bytes); + int (*data)(int chan_id, struct stream *s); struct trans *xrdp_api_trans; }; @@ -657,32 +657,24 @@ static int process_message_drdynvc_data_first(struct stream *s) { struct chansrv_drdynvc *drdynvc; - int chan_id; - int bytes; + uint32_t chan_id; int total_bytes; - char *data; LOG_DEVEL(LOG_LEVEL_DEBUG, "process_message_drdynvc_data_first:"); - if (!s_check_rem(s, 12)) + if (!s_check_rem(s, 8)) { return 1; } in_uint32_le(s, chan_id); - in_uint32_le(s, bytes); in_uint32_le(s, total_bytes); - if (!s_check_rem(s, bytes)) - { - return 1; - } - in_uint8p(s, data, bytes); - if ((chan_id < 0) || (chan_id > 255)) + if (chan_id > 255) { return 1; } drdynvc = g_drdynvcs + chan_id; if (drdynvc->data_first != NULL) { - if (drdynvc->data_first(chan_id, data, bytes, total_bytes) != 0) + if (drdynvc->data_first(chan_id, s, total_bytes) != 0) { return 1; } @@ -697,9 +689,7 @@ static int process_message_drdynvc_data(struct stream *s) { struct chansrv_drdynvc *drdynvc; - int chan_id; - int bytes; - char *data; + uint32_t chan_id; LOG_DEVEL(LOG_LEVEL_DEBUG, "process_message_drdynvc_data:"); if (!s_check_rem(s, 8)) @@ -707,16 +697,10 @@ process_message_drdynvc_data(struct stream *s) return 1; } in_uint32_le(s, chan_id); - in_uint32_le(s, bytes); - if (!s_check_rem(s, bytes)) - { - return 1; - } - in_uint8p(s, data, bytes); drdynvc = g_drdynvcs + chan_id; if (drdynvc->data != NULL) { - if (drdynvc->data(chan_id, data, bytes) != 0) + if (drdynvc->data(chan_id, s) != 0) { return 1; } @@ -1136,24 +1120,24 @@ my_api_close_response(int chan_id) /*****************************************************************************/ static int -my_api_data_first(int chan_id, char *data, int bytes, int total_bytes) +my_api_data_first(int chan_id, struct stream *s, int total_bytes) { struct trans *trans; - struct stream *s; - + struct stream *out_s; + int bytes = s_rem(s); //g_writeln("my_api_data_first: bytes %d total_bytes %d", bytes, total_bytes); trans = get_api_trans_from_chan_id(chan_id); if (trans == NULL) { return 1; } - s = trans_get_out_s(trans, bytes); - if (s == NULL) + out_s = trans_get_out_s(trans, bytes); + if (out_s == NULL) { return 1; } - out_uint8a(s, data, bytes); - s_mark_end(s); + out_uint8a(out_s, s->p, bytes); + s_mark_end(out_s); if (trans_write_copy(trans) != 0) { return 1; @@ -1163,24 +1147,24 @@ my_api_data_first(int chan_id, char *data, int bytes, int total_bytes) /*****************************************************************************/ static int -my_api_data(int chan_id, char *data, int bytes) +my_api_data(int chan_id, struct stream *s) { struct trans *trans; - struct stream *s; + struct stream *out_s; + int bytes = s_rem(s); - //g_writeln("my_api_data: bytes %d", bytes); trans = get_api_trans_from_chan_id(chan_id); if (trans == NULL) { return 1; } - s = trans_get_out_s(trans, bytes); - if (s == NULL) + out_s = trans_get_out_s(trans, bytes); + if (out_s == NULL) { return 1; } - out_uint8a(s, data, bytes); - s_mark_end(s); + out_uint8a(out_s, s->p, bytes); + s_mark_end(out_s); if (trans_write_copy(trans) != 0) { return 1; diff --git a/sesman/chansrv/chansrv.h b/sesman/chansrv/chansrv.h index 6e86f737..d085f695 100644 --- a/sesman/chansrv/chansrv.h +++ b/sesman/chansrv/chansrv.h @@ -62,8 +62,8 @@ struct chansrv_drdynvc_procs { int (*open_response)(int chan_id, int creation_status); int (*close_response)(int chan_id); - int (*data_first)(int chan_id, char *data, int bytes, int total_bytes); - int (*data)(int chan_id, char *data, int bytes); + int (*data_first)(int chan_id, struct stream *s, int total_bytes); + int (*data)(int chan_id, struct stream *s); }; int diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 4b6489db..5ab1183c 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -2148,7 +2148,7 @@ xrdp_mm_drdynvc_data_first(struct xrdp_process *id, int chan_id, int bytes = s_rem(s); // Size of PDU sent to chansrv - int pdu_size = 8 + 8 + 4 + 4 + 4 + bytes; + int pdu_size = 8 + 8 + 4 + 4 + bytes; wm = id->wm; trans = wm->mm->chan_trans; out_s = trans_get_out_s(trans, pdu_size); @@ -2162,8 +2162,8 @@ xrdp_mm_drdynvc_data_first(struct xrdp_process *id, int chan_id, out_uint32_le(out_s, pdu_size - 8); chansrv_chan_id = wm->mm->xr2cr_cid_map[chan_id]; out_uint32_le(out_s, chansrv_chan_id); - out_uint32_le(out_s, bytes); out_uint32_le(out_s, total_bytes); + // Caller works out 'bytes' value from incoming stream length out_uint8p(out_s, s->p, bytes); s_mark_end(out_s); return trans_write_copy(trans); @@ -2181,7 +2181,7 @@ xrdp_mm_drdynvc_data(struct xrdp_process *id, int chan_id, struct stream *s) int bytes = s_rem(s); // Size of PDU sent to chansrv - int pdu_size = 8 + 8 + 4 + 4 + bytes; + int pdu_size = 8 + 8 + 4 + bytes; wm = id->wm; trans = wm->mm->chan_trans; out_s = trans_get_out_s(trans, pdu_size); @@ -2195,7 +2195,7 @@ xrdp_mm_drdynvc_data(struct xrdp_process *id, int chan_id, struct stream *s) out_uint32_le(out_s, pdu_size - 8); chansrv_chan_id = wm->mm->xr2cr_cid_map[chan_id]; out_uint32_le(out_s, chansrv_chan_id); - out_uint32_le(out_s, bytes); + // Caller works out 'bytes' value from incoming stream length out_uint8p(out_s, s->p, bytes); s_mark_end(out_s); return trans_write_copy(trans); From f4249b3ca60ff412fc5e59d8ff0e24c796e5fdef Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:11:03 +0100 Subject: [PATCH 6/7] chansrv: Use common dynamic dechunker Addresses CVE-2026-69169 The dynamic channel processing in chansrv is updated to allow the dechunker to be invoked automatically if the 'data_first' proc is set to NULL. This mirrors a change made to the xrdp_channel.c. The processor for the AUDIO_IN channel is updated to take advantage of this, significantly simplifing the code. --- sesman/chansrv/audin.c | 61 +--------------- sesman/chansrv/chansrv.c | 154 +++++++++++++++++++++++++++++++++------ sesman/chansrv/chansrv.h | 2 + 3 files changed, 135 insertions(+), 82 deletions(-) diff --git a/sesman/chansrv/audin.c b/sesman/chansrv/audin.c index 9c59310c..a8900a73 100644 --- a/sesman/chansrv/audin.c +++ b/sesman/chansrv/audin.c @@ -77,7 +77,6 @@ static struct xr_wave_format_ex g_pcm_44100 = static struct chansrv_drdynvc_procs g_audin_info; static int g_audin_chanid; -static struct stream *g_in_s; static struct xr_wave_format_ex *g_server_formats[] = { @@ -422,64 +421,9 @@ audin_close_response(int chan_id) LOG_DEVEL(LOG_LEVEL_INFO, "audin_close_response:"); g_audin_chanid = 0; cleanup_client_formats(); - free_stream(g_in_s); - g_in_s = NULL; return 0; } -/*****************************************************************************/ -static int -audin_data_fragment(int chan_id, struct stream *s) -{ - int rv; - int bytes = s_rem(s); - LOG_DEVEL(LOG_LEVEL_DEBUG, "audin_data_fragment:"); - if (!s_check_rem(g_in_s, bytes)) - { - LOG_DEVEL(LOG_LEVEL_ERROR, "audin_data_fragment: error bytes %d left %d", - bytes, (int) (g_in_s->end - g_in_s->p)); - return 1; - } - out_uint8a(g_in_s, s->p, bytes); - if (g_in_s->p == g_in_s->end) - { - g_in_s->p = g_in_s->data; - rv = audin_process_msg(chan_id, g_in_s); - free_stream(g_in_s); - g_in_s = NULL; - return rv; - } - return 0; -} - -/*****************************************************************************/ -static int -audin_data_first(int chan_id, struct stream *s, int total_bytes) -{ - LOG_DEVEL(LOG_LEVEL_DEBUG, "audin_data_first:"); - if (g_in_s != NULL) - { - LOG_DEVEL(LOG_LEVEL_ERROR, "audin_data_first: warning g_in_s is not nil"); - free_stream(g_in_s); - } - make_stream(g_in_s); - init_stream(g_in_s, total_bytes); - g_in_s->end = g_in_s->data + total_bytes; - return audin_data_fragment(chan_id, s); -} - -/*****************************************************************************/ -static int -audin_data(int chan_id, struct stream *s) -{ - LOG_DEVEL_HEXDUMP(LOG_LEVEL_TRACE, "audin_data:", s->p, s_rem(s)); - if (g_in_s == NULL) - { - return audin_process_msg(chan_id, s); - } - return audin_data_fragment(chan_id, s); -} - /*****************************************************************************/ int audin_init(void) @@ -488,10 +432,9 @@ audin_init(void) g_memset(&g_audin_info, 0, sizeof(g_audin_info)); g_audin_info.open_response = audin_open_response; g_audin_info.close_response = audin_close_response; - g_audin_info.data_first = audin_data_first; - g_audin_info.data = audin_data; + g_audin_info.data_first = NULL; + g_audin_info.data = audin_process_msg; g_audin_chanid = 0; - g_in_s = NULL; return 0; } diff --git a/sesman/chansrv/chansrv.c b/sesman/chansrv/chansrv.c index 96ec3e1d..066ddd06 100644 --- a/sesman/chansrv/chansrv.c +++ b/sesman/chansrv/chansrv.c @@ -46,6 +46,7 @@ #include "xrdp_constants.h" #include "audin.h" #include "channel_defs.h" +#include "dechunker.h" #include "scp.h" #include "scp_sync.h" @@ -98,6 +99,7 @@ struct chansrv_drdynvc int status; /* see CHANSRV_DRDYNVC_STATUS_* */ int flags; int pad0; + struct dyn_dechunker *dc; // Use to dechunk fragments int (*open_response)(int chan_id, int creation_status); int (*close_response)(int chan_id); int (*data_first)(int chan_id, struct stream *s, int total_bytes); @@ -105,7 +107,7 @@ struct chansrv_drdynvc struct trans *xrdp_api_trans; }; -static struct chansrv_drdynvc g_drdynvcs[256]; +static struct chansrv_drdynvc g_drdynvcs[DRDYNVC_CHANNEL_COUNT]; /* data in struct trans::callback_data */ struct xrdp_api_data @@ -576,7 +578,7 @@ static int process_message_drdynvc_open_response(struct stream *s) { struct chansrv_drdynvc *drdynvc; - int chan_id; + uint32_t chan_id; int creation_status; LOG_DEVEL(LOG_LEVEL_DEBUG, "process_message_drdynvc_open_response:"); @@ -586,7 +588,7 @@ process_message_drdynvc_open_response(struct stream *s) } in_uint32_le(s, chan_id); in_uint32_le(s, creation_status); - if ((chan_id < 0) || (chan_id > 255)) + if (chan_id >= DRDYNVC_CHANNEL_COUNT) { return 1; } @@ -603,6 +605,8 @@ process_message_drdynvc_open_response(struct stream *s) else { drdynvc->status = CHANSRV_DRDYNVC_STATUS_CLOSED; + dyn_dechunker_free(drdynvc->dc); + drdynvc->dc = NULL; } if (drdynvc->open_response != NULL) { @@ -621,7 +625,7 @@ static int process_message_drdynvc_close_response(struct stream *s) { struct chansrv_drdynvc *drdynvc; - int chan_id; + uint32_t chan_id; LOG_DEVEL(LOG_LEVEL_DEBUG, "process_message_drdynvc_close_response:"); if (!s_check_rem(s, 4)) @@ -629,7 +633,7 @@ process_message_drdynvc_close_response(struct stream *s) return 1; } in_uint32_le(s, chan_id); - if ((chan_id < 0) || (chan_id > 255)) + if (chan_id >= DRDYNVC_CHANNEL_COUNT) { return 1; } @@ -640,6 +644,8 @@ process_message_drdynvc_close_response(struct stream *s) return 0; } drdynvc->status = CHANSRV_DRDYNVC_STATUS_CLOSED; + dyn_dechunker_free(drdynvc->dc); + drdynvc->dc = NULL; if (drdynvc->close_response != NULL) { if (drdynvc->close_response(chan_id) != 0) @@ -659,6 +665,7 @@ process_message_drdynvc_data_first(struct stream *s) struct chansrv_drdynvc *drdynvc; uint32_t chan_id; int total_bytes; + int rv = 0; LOG_DEVEL(LOG_LEVEL_DEBUG, "process_message_drdynvc_data_first:"); if (!s_check_rem(s, 8)) @@ -667,19 +674,47 @@ process_message_drdynvc_data_first(struct stream *s) } in_uint32_le(s, chan_id); in_uint32_le(s, total_bytes); - if (chan_id > 255) + if (chan_id >= DRDYNVC_CHANNEL_COUNT) { return 1; } drdynvc = g_drdynvcs + chan_id; if (drdynvc->data_first != NULL) { - if (drdynvc->data_first(chan_id, s, total_bytes) != 0) + // Caller has requested to defragment PDUs themself + rv = drdynvc->data_first(chan_id, s, total_bytes); + } + else + { + // Get the dechunker working on the stream + enum dyn_dechunker_status status; + status = dyn_dechunker_process_first_chunk(drdynvc->dc, + s, total_bytes); + switch (status) { - return 1; + case E_DYN_INLINE_CHUNK: + // Pass through as data PDU + if (drdynvc->data != NULL) + { + rv = drdynvc->data(chan_id, s); + } + break; + + case E_DYN_IN_PROGRESS: + break; + + case E_DYN_ERROR: + rv = 1; + break; + + default: + LOG(LOG_LEVEL_ERROR, + "Dechunker returned unknown error %d", + (int)status); + rv = 1; } } - return 0; + return rv; } /*****************************************************************************/ @@ -689,23 +724,73 @@ static int process_message_drdynvc_data(struct stream *s) { struct chansrv_drdynvc *drdynvc; - uint32_t chan_id; + int chan_id; + struct stream *ls = NULL; // Set if the application to be called + int free_ls = 0; // Set if we need to clear ls when we're done + int rv = 0; LOG_DEVEL(LOG_LEVEL_DEBUG, "process_message_drdynvc_data:"); - if (!s_check_rem(s, 8)) + if (!s_check_rem(s, 4)) { return 1; } in_uint32_le(s, chan_id); - drdynvc = g_drdynvcs + chan_id; - if (drdynvc->data != NULL) + if (chan_id >= DRDYNVC_CHANNEL_COUNT) { - if (drdynvc->data(chan_id, s) != 0) + return 1; + } + drdynvc = g_drdynvcs + chan_id; + if (drdynvc->data_first != NULL) + { + // Caller is processing all PDUs directly + ls = s; + } + else + { + // Pass the PDU to the dechunker + enum dyn_dechunker_status dechunker_status = + dyn_dechunker_process_data_chunk(drdynvc->dc, s); + switch (dechunker_status) { - return 1; + case E_DYN_INLINE_CHUNK: + ls = s; + break; + + case E_DYN_IN_PROGRESS: + rv = 0; + break; + + case E_DYN_READY: + ls = dyn_dechunker_get_stream(drdynvc->dc); + // We now own the stream, so must delete it + free_ls = 1; + break; + + case E_DYN_ERROR: + // Error has been logged + rv = 1; + break; + + default: + LOG(LOG_LEVEL_ERROR, + "Dechunker returned unknown error %d", + (int)dechunker_status); + rv = 1; } } - return 0; + + if (ls != NULL) + { + if (drdynvc->data != NULL) + { + rv = drdynvc->data(chan_id, ls); + } + if (free_ls) + { + free_stream(ls); + } + } + return rv; } /*****************************************************************************/ @@ -718,12 +803,13 @@ chansrv_drdynvc_open(const char *name, int flags, int name_bytes; int lchan_id; int error; + struct dyn_dechunker *dc = NULL; lchan_id = 1; while (g_drdynvcs[lchan_id].status != CHANSRV_DRDYNVC_STATUS_CLOSED) { lchan_id++; - if (lchan_id > 255) + if (lchan_id >= DRDYNVC_CHANNEL_COUNT) { return 1; } @@ -733,6 +819,18 @@ chansrv_drdynvc_open(const char *name, int flags, { return 1; } + + // If there's no 'data_first' proc, we need a dechunker for the + // channel + if (procs->data_first == NULL) + { + if ((dc = dyn_dechunker_init(name)) == NULL) + { + // Error logged + return 1; + } + } + name_bytes = g_strlen(name); out_uint32_le(s, 0); /* version */ out_uint32_le(s, 8 + 8 + 4 + name_bytes + 4 + 4); @@ -749,13 +847,17 @@ chansrv_drdynvc_open(const char *name, int flags, if (chan_id != NULL) { *chan_id = lchan_id; - g_drdynvcs[lchan_id].open_response = procs->open_response; - g_drdynvcs[lchan_id].close_response = procs->close_response; - g_drdynvcs[lchan_id].data_first = procs->data_first; - g_drdynvcs[lchan_id].data = procs->data; - g_drdynvcs[lchan_id].status = CHANSRV_DRDYNVC_STATUS_OPEN_SENT; - } + g_drdynvcs[lchan_id].open_response = procs->open_response; + g_drdynvcs[lchan_id].close_response = procs->close_response; + g_drdynvcs[lchan_id].data_first = procs->data_first; + g_drdynvcs[lchan_id].data = procs->data; + g_drdynvcs[lchan_id].status = CHANSRV_DRDYNVC_STATUS_OPEN_SENT; + g_drdynvcs[lchan_id].dc = dc; + } + else + { + dyn_dechunker_free(dc); } return error; } @@ -1738,6 +1840,8 @@ x_server_fatal_handler(void) int main_cleanup(void) { + int i; + if (g_term_event != 0) { g_delete_wait_obj(g_term_event); @@ -1756,6 +1860,10 @@ main_cleanup(void) tc_mutex_delete(g_exec_mutex); tc_sem_delete(g_exec_sem); } + for (i = 0 ; i < DRDYNVC_CHANNEL_COUNT; ++i) + { + dyn_dechunker_free(g_drdynvcs[i].dc); + } log_end(); config_free(g_cfg); g_deinit(); /* os_calls */ diff --git a/sesman/chansrv/chansrv.h b/sesman/chansrv/chansrv.h index d085f695..2434cb35 100644 --- a/sesman/chansrv/chansrv.h +++ b/sesman/chansrv/chansrv.h @@ -62,6 +62,8 @@ struct chansrv_drdynvc_procs { int (*open_response)(int chan_id, int creation_status); int (*close_response)(int chan_id); + // Set data_first to NULL to have the dechunker automatically + // handle channel fragments int (*data_first)(int chan_id, struct stream *s, int total_bytes); int (*data)(int chan_id, struct stream *s); }; From a6d80e17ab5afb9664317c3ef2a22fef19891a42 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:37:11 +0100 Subject: [PATCH 7/7] Code quality: Address Copilot review comments All accesses to g_drdynvcs[] in chansrv.c have been checked for unbounded access. --- common/dechunker.c | 2 +- common/dechunker.h | 2 +- sesman/chansrv/chansrv.c | 14 +++++++++----- tests/common/test_dechunker.c | 10 +++++----- xrdp/xrdp_egfx.c | 2 +- 5 files changed, 17 insertions(+), 13 deletions(-) diff --git a/common/dechunker.c b/common/dechunker.c index c20e1801..19fb6d1c 100644 --- a/common/dechunker.c +++ b/common/dechunker.c @@ -439,7 +439,7 @@ dyn_dechunker_process_first_chunk(struct dyn_dechunker *self, } else if (frag_size == total_size) { - // This chunk ccontains all the data + // This chunk contains all the data status = E_DYN_INLINE_CHUNK; } else diff --git a/common/dechunker.h b/common/dechunker.h index 5bc6c8ce..43f640fe 100644 --- a/common/dechunker.h +++ b/common/dechunker.h @@ -50,7 +50,7 @@ enum vc_dechunker_status }; /** - * Returned from dyn_dechunker_process_chunk() and + * Returned from dyn_dechunker_process_data_chunk() and * Returned from dyn_dechunker_process_first_chunk() */ enum dyn_dechunker_status diff --git a/sesman/chansrv/chansrv.c b/sesman/chansrv/chansrv.c index 066ddd06..2e70fe1b 100644 --- a/sesman/chansrv/chansrv.c +++ b/sesman/chansrv/chansrv.c @@ -83,7 +83,6 @@ tbus g_exec_mutex; tbus g_exec_sem; int g_exec_pid = 0; -#define ARRAYSIZE(x) (sizeof(x)/sizeof(*(x))) /* max total channel bytes size */ #define MAX_CHANNEL_BYTES (1 * 1024 * 1024 * 1024) /* 1 GB */ #define MAX_CHANNEL_FRAG_BYTES 1600 @@ -724,7 +723,7 @@ static int process_message_drdynvc_data(struct stream *s) { struct chansrv_drdynvc *drdynvc; - int chan_id; + uint32_t chan_id; struct stream *ls = NULL; // Set if the application to be called int free_ls = 0; // Set if we need to clear ls when we're done int rv = 0; @@ -890,6 +889,10 @@ chansrv_drdynvc_close(int chan_id) struct stream *s; int error; + if (chan_id < 0 || chan_id >= DRDYNVC_CHANNEL_COUNT) + { + return 1; + } s = trans_get_out_s(g_con_trans, 8192); if (s == NULL) { @@ -1179,9 +1182,10 @@ my_trans_data_in(struct trans *trans) /*****************************************************************************/ static struct trans * -get_api_trans_from_chan_id(int chan_id) +get_api_trans_from_chan_id(uint32_t chan_id) { - return g_drdynvcs[chan_id].xrdp_api_trans; + return (chan_id >= DRDYNVC_CHANNEL_COUNT) + ? NULL : g_drdynvcs[chan_id].xrdp_api_trans; } /*****************************************************************************/ @@ -1605,7 +1609,7 @@ api_con_trans_list_check_wait_objs(void) chansrv_drdynvc_close(ad->chan_id); } for (drdynvc_index = 0; - drdynvc_index < (int) ARRAYSIZE(g_drdynvcs); + drdynvc_index < DRDYNVC_CHANNEL_COUNT; drdynvc_index++) { if (g_drdynvcs[drdynvc_index].xrdp_api_trans == ltran) diff --git a/tests/common/test_dechunker.c b/tests/common/test_dechunker.c index 9f945cb1..ded2cdd8 100644 --- a/tests/common/test_dechunker.c +++ b/tests/common/test_dechunker.c @@ -152,7 +152,7 @@ static const char frankenstein[] = // The dynamic dechunker works on total data block sizes of 1600 bytes, // including the block header as well. // The FIRST block header is 6-12 bytes long, and the DATA block header -// is 5-8 bytes long. For simplicity we're assume a header size of 8 +// is 5-8 bytes long. For simplicity we assume a header size of 8 // bytes, and hence a data size of 1592 bytes. #define FRANKENSTEIN_DYN_CHUNK_SIZE 1592 @@ -202,10 +202,10 @@ START_TEST(test_vc_dechunker_bad_params) ck_assert_ptr_ne(dc, NULL); // vc_dechunker_free - vc_dechunker_free(NULL); // Musn't crash! + vc_dechunker_free(NULL); // Must not crash! // vc_dechunker_get_stream - vc_dechunker_get_stream(NULL); // Musn't crash! + vc_dechunker_get_stream(NULL); // Must not crash! // vc_dechunker_process_chunk stat = vc_dechunker_process_chunk(NULL, s, 0, 1600); // No dechunker @@ -629,10 +629,10 @@ START_TEST(test_dyn_dechunker_bad_params) ck_assert_ptr_ne(dc, NULL); // dyn_dechunker_free - dyn_dechunker_free(NULL); // Musn't crash! + dyn_dechunker_free(NULL); // Must not crash! // dyn_dechunker_get_stream - dyn_dechunker_get_stream(NULL); // Musn't crash! + dyn_dechunker_get_stream(NULL); // Must not crash! // dyn_dechunker_process_first_chunk stat = dyn_dechunker_process_first_chunk(NULL, s, 1600); // No dechunker diff --git a/xrdp/xrdp_egfx.c b/xrdp/xrdp_egfx.c index 84f33790..90a0f491 100644 --- a/xrdp/xrdp_egfx.c +++ b/xrdp/xrdp_egfx.c @@ -997,7 +997,7 @@ xrdp_egfx_create(struct xrdp_mm *mm, struct xrdp_egfx **egfx) } procs.open_response = xrdp_egfx_open_response; procs.close_response = xrdp_egfx_close_response; - procs.data_first = NULL; // Defragging handled elsewere + procs.data_first = NULL; // Defragging handled elsewhere procs.data = xrdp_egfx_data; process = mm->wm->pro_layer; error = libxrdp_drdynvc_open(process->session,