diff --git a/0004-Release-memory-after-handling-packet.patch b/0004-Release-memory-after-handling-packet.patch new file mode 100644 index 0000000..52c4e60 --- /dev/null +++ b/0004-Release-memory-after-handling-packet.patch @@ -0,0 +1,113 @@ +From f66f7d35b84a06629ef4d5d13b0f9e06076191d4 Mon Sep 17 00:00:00 2001 +From: Michael Hanselmann +Date: Sat, 29 May 2021 16:54:38 +0200 +Subject: [PATCH] Release memory after handling packet + +These memory leaks were found with the usbredirparser fuzzer. + +The API transfers ownership of the data allocation only for the *_packet +callbacks (also documented in "usbredirparser.h"). The hello packet handler +already freed the payload while the others didn't. + +In addition destroying the parser structure must also destroy the read data +buffer. + +With these changes it's finally possible to run in excess of 1.7 million fuzzing +iterations without noticable memory leaks. LeakSanitizer no longer reports after +a few iterations. + +Signed-off-by: Michael Hanselmann +--- + usbredirparser/usbredirparser.c | 21 +++++++++++++++++---- + 1 file changed, 17 insertions(+), 4 deletions(-) + +diff --git a/usbredirparser/usbredirparser.c b/usbredirparser/usbredirparser.c +index 67303d0..5d19e38 100644 +--- a/usbredirparser/usbredirparser.c ++++ b/usbredirparser/usbredirparser.c +@@ -22,6 +22,7 @@ + + #include + #include ++#include + #include + #include + #include +@@ -281,7 +282,6 @@ static void usbredirparser_handle_hello(struct usbredirparser *parser_pub, + } + usbredirparser_verify_caps(parser, parser->peer_caps, "peer"); + parser->have_peer_caps = 1; +- free(data); + + INFO("Peer version: %s, using %d-bits ids", buf, + usbredirparser_using_32bits_ids(parser_pub) ? 32 : 64); +@@ -773,7 +773,8 @@ static int usbredirparser_verify_type_header( + return 1; /* Verify ok */ + } + +-static void usbredirparser_call_type_func(struct usbredirparser *parser_pub) ++static void usbredirparser_call_type_func(struct usbredirparser *parser_pub, ++ bool *data_ownership_transferred) + { + struct usbredirparser_priv *parser = + (struct usbredirparser_priv *)parser_pub; +@@ -912,26 +913,31 @@ static void usbredirparser_call_type_func(struct usbredirparser *parser_pub) + parser->type_header); + break; + case usb_redir_control_packet: ++ *data_ownership_transferred = true; + parser->callb.control_packet_func(parser->callb.priv, id, + (struct usb_redir_control_packet_header *)parser->type_header, + parser->data, parser->data_len); + break; + case usb_redir_bulk_packet: ++ *data_ownership_transferred = true; + parser->callb.bulk_packet_func(parser->callb.priv, id, + (struct usb_redir_bulk_packet_header *)parser->type_header, + parser->data, parser->data_len); + break; + case usb_redir_iso_packet: ++ *data_ownership_transferred = true; + parser->callb.iso_packet_func(parser->callb.priv, id, + (struct usb_redir_iso_packet_header *)parser->type_header, + parser->data, parser->data_len); + break; + case usb_redir_interrupt_packet: ++ *data_ownership_transferred = true; + parser->callb.interrupt_packet_func(parser->callb.priv, id, + (struct usb_redir_interrupt_packet_header *)parser->type_header, + parser->data, parser->data_len); + break; + case usb_redir_buffered_bulk_packet: ++ *data_ownership_transferred = true; + parser->callb.buffered_bulk_packet_func(parser->callb.priv, id, + (struct usb_redir_buffered_bulk_packet_header *)parser->type_header, + parser->data, parser->data_len); +@@ -944,6 +950,7 @@ int usbredirparser_do_read(struct usbredirparser *parser_pub) + struct usbredirparser_priv *parser = + (struct usbredirparser_priv *)parser_pub; + int r, header_len, type_header_len, data_len; ++ bool data_ownership_transferred; + uint8_t *dest; + + header_len = usbredirparser_get_header_len(parser_pub); +@@ -1028,8 +1035,14 @@ int usbredirparser_do_read(struct usbredirparser *parser_pub) + r = usbredirparser_verify_type_header(parser_pub, + parser->header.type, parser->type_header, + parser->data, parser->data_len, 0); +- if (r) +- usbredirparser_call_type_func(parser_pub); ++ data_ownership_transferred = false; ++ if (r) { ++ usbredirparser_call_type_func(parser_pub, ++ &data_ownership_transferred); ++ } ++ if (!data_ownership_transferred) { ++ free(parser->data); ++ } + parser->header_read = 0; + parser->type_header_len = 0; + parser->type_header_read = 0; +-- +1.8.3.1 + diff --git a/0005-usbredirparser-free-parser-s-data-on-destroy.patch b/0005-usbredirparser-free-parser-s-data-on-destroy.patch new file mode 100644 index 0000000..b29da21 --- /dev/null +++ b/0005-usbredirparser-free-parser-s-data-on-destroy.patch @@ -0,0 +1,30 @@ +From 6d12abee7aadb757565a26f8243e46b1718fcb7e Mon Sep 17 00:00:00 2001 +From: Michael Hanselmann +Date: Tue, 15 Jun 2021 13:04:50 +0200 +Subject: [PATCH] usbredirparser: free parser's data on destroy + +Ref: https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/25#note_962929 + +Signed-off-by: Michael Hanselmann +Signed-off-by: Victor Toso +--- + usbredirparser/usbredirparser.c | 3 +++ + 1 file changed, 3 insertions(+) + +diff --git a/usbredirparser/usbredirparser.c b/usbredirparser/usbredirparser.c +index 5d19e38..c158c66 100644 +--- a/usbredirparser/usbredirparser.c ++++ b/usbredirparser/usbredirparser.c +@@ -193,6 +193,9 @@ void usbredirparser_destroy(struct usbredirparser *parser_pub) + (struct usbredirparser_priv *)parser_pub; + struct usbredirparser_buf *wbuf, *next_wbuf; + ++ free(parser->data); ++ parser->data = NULL; ++ + wbuf = parser->write_buf; + while (wbuf) { + next_wbuf = wbuf->next; +-- +1.8.3.1 + diff --git a/0006-Avoid-memory-leak-from-ill-formatted-serialization-d.patch b/0006-Avoid-memory-leak-from-ill-formatted-serialization-d.patch new file mode 100644 index 0000000..7a1f541 --- /dev/null +++ b/0006-Avoid-memory-leak-from-ill-formatted-serialization-d.patch @@ -0,0 +1,76 @@ +From babd82e4f83d479e9fb418cd2dd55be6a95a1fe7 Mon Sep 17 00:00:00 2001 +From: Michael Hanselmann +Date: Thu, 26 Aug 2021 22:32:55 +0200 +Subject: [PATCH] Avoid memory leak from ill-formatted serialization data + +At commit 060d7914ea the following `fuzzing/usbredirparserfuzz` input triggers +a memory leak: + +``` +$ base64 -d <<'EOF' | gunzip -c > testcase +H4sIAFDwJ2ECA2NgZIADBQWF/zD2HQgfAv4DgcJ/BTzgP0iRAlwRivL/ePX9 +R1VAyBaSjcShB8nPTBA9/xn+g5UAANvH+dkSAQAA +``` + +The data type header segment is empty while there's (supposed) payload data in +there. The internal buffers must be filled in the order of header, type header +and then data, an invariant important for `usbredirparser_do_read` and violated +by this input. + +With this change the input data is read the same way, but if the invariant would +be violated the data read is just ignored. + +The parser check at the beginning of `usbredirparser_unserialize` is also +improved and `write_buf_count` is no longer set explicitly. + +Signed-off-by: Michael Hanselmann +--- + usbredirparser/usbredirparser.c | 18 +++++++++++++----- + 1 file changed, 13 insertions(+), 5 deletions(-) + +diff --git a/usbredirparser/usbredirparser.c b/usbredirparser/usbredirparser.c +index c158c66..a0312fb 100644 +--- a/usbredirparser/usbredirparser.c ++++ b/usbredirparser/usbredirparser.c +@@ -1692,8 +1692,9 @@ int usbredirparser_unserialize(struct usbredirparser *parser_pub, + return -1; + } + +- if (parser->write_buf_count != 0 || parser->write_buf != NULL || +- parser->data != NULL) { ++ if (!(parser->write_buf_count == 0 && parser->write_buf == NULL && ++ parser->data == NULL && parser->header_read == 0 && ++ parser->type_header_read == 0 && parser->data_read == 0)) { + ERROR("unserialization must use a pristine parser"); + return -1; + } +@@ -1769,7 +1770,9 @@ int usbredirparser_unserialize(struct usbredirparser *parser_pub, + i = parser->type_header_len; + if (unserialize_data(parser, &state, &remain, &data, &i, "type_header")) + return -1; +- parser->type_header_read = i; ++ if (parser->header_read == header_len) { ++ parser->type_header_read = i; ++ } + + if (parser->data_len) { + parser->data = malloc(parser->data_len); +@@ -1781,8 +1784,13 @@ int usbredirparser_unserialize(struct usbredirparser *parser_pub, + i = parser->data_len; + if (unserialize_data(parser, &state, &remain, &parser->data, &i, "data")) + return -1; +- parser->data_read = i; +- parser->write_buf_count = 0; ++ if (parser->header_read == header_len && ++ parser->type_header_read == parser->type_header_len) { ++ parser->data_read = i; ++ } else if (parser->data != NULL) { ++ free(parser->data); ++ parser->data = NULL; ++ } + + /* Get the write buffer count and the write buffers */ + if (unserialize_int(parser, &state, &remain, &i, "write_buf_count")) +-- +1.8.3.1 + diff --git a/0007-Fix-some-issues-detected-by-fuzzer.patch b/0007-Fix-some-issues-detected-by-fuzzer.patch new file mode 100644 index 0000000..6d1c3b5 --- /dev/null +++ b/0007-Fix-some-issues-detected-by-fuzzer.patch @@ -0,0 +1,55 @@ +From f2d7dab7119615ab092e3feac2f69dc7b31e94a5 Mon Sep 17 00:00:00 2001 +From: Frediano Ziglio +Date: Mon, 13 Sep 2021 15:12:43 +0100 +Subject: [PATCH] Fix some issues detected by fuzzer + +If we fail to unserialize data we need to reset data to avoid +invalid state. +We can accept data only if we had data (data_len > 0), otherwise +reset it. + +This also fixes https://gitlab.freedesktop.org/spice/usbredir/-/issues/21. + +Signed-off-by: Frediano Ziglio +--- + usbredirparser/usbredirparser.c | 11 +++++++++-- + 1 file changed, 9 insertions(+), 2 deletions(-) + +diff --git a/usbredirparser/usbredirparser.c b/usbredirparser/usbredirparser.c +index a0312fb..f9a95e0 100644 +--- a/usbredirparser/usbredirparser.c ++++ b/usbredirparser/usbredirparser.c +@@ -1748,6 +1748,7 @@ int usbredirparser_unserialize(struct usbredirparser *parser_pub, + if (unserialize_data(parser, &state, &remain, &data, &i, "header")) + return -1; + parser->header_read = i; ++ parser->type_header_len = 0; + + /* Set various length field froms the header (if we've a header) */ + if (parser->header_read == header_len) { +@@ -1782,14 +1783,20 @@ int usbredirparser_unserialize(struct usbredirparser *parser_pub, + } + } + i = parser->data_len; +- if (unserialize_data(parser, &state, &remain, &parser->data, &i, "data")) ++ if (unserialize_data(parser, &state, &remain, &parser->data, &i, "data")) { ++ free(parser->data); ++ parser->data = NULL; ++ parser->data_len = 0; + return -1; ++ } + if (parser->header_read == header_len && +- parser->type_header_read == parser->type_header_len) { ++ parser->type_header_read == parser->type_header_len && ++ parser->data_len > 0) { + parser->data_read = i; + } else if (parser->data != NULL) { + free(parser->data); + parser->data = NULL; ++ parser->data_len = 0; + } + + /* Get the write buffer count and the write buffers */ +-- +1.8.3.1 + diff --git a/usbredir.spec b/usbredir.spec index 98ca8ca..70177b0 100644 --- a/usbredir.spec +++ b/usbredir.spec @@ -1,6 +1,6 @@ Name: usbredir Version: 0.8.0 -Release: 6 +Release: 7 Summary: network protocol libraries for sending USB device traffic License: LGPLv2+ and GPLv2+ URL: https://www.spice-space.org/usbredir.html @@ -14,6 +14,10 @@ Obsoletes: %{name}-server Patch1: 0001-CVE-2021-3700.patch Patch2: 0002-Skip-empty-write-buffers-when-unserializing-parser.patch Patch3: 0003-Update-write-buffer-count-during-deserialization.patch +Patch4: 0004-Release-memory-after-handling-packet.patch +Patch5: 0005-usbredirparser-free-parser-s-data-on-destroy.patch +Patch6: 0006-Avoid-memory-leak-from-ill-formatted-serialization-d.patch +Patch7: 0007-Fix-some-issues-detected-by-fuzzer.patch %description usbredir is the name of a network protocol for sending USB device traffic over @@ -70,6 +74,9 @@ make %{?_smp_mflags} V=1 %changelog +* Wed Dec 08 2021 wuguanghao - 0.8.0-7 +- fix memleak in fuzz-test + * Tue Dec 07 2021 wuguanghao - 0.8.0-6 - fix timeout and abort of fuzz-test