packages/usbredir
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix memleak in fuzz-test

Browse Source
This commit is contained in:
wguanghao 2021-12-08 15:44:29 +08:00 committed by markeryang
parent 33317f65da
commit 557c99b566
5 changed files with 282 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

113
0004-Release-memory-after-handling-packet.patch Normal file
View File

@ -0,0 +1,113 @@
From f66f7d35b84a06629ef4d5d13b0f9e06076191d4 Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <public@hansmi.ch>
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 <public@hansmi.ch>
---
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 <assert.h>
#include <stddef.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
@@ -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

30
0005-usbredirparser-free-parser-s-data-on-destroy.patch Normal file
View File

@ -0,0 +1,30 @@
From 6d12abee7aadb757565a26f8243e46b1718fcb7e Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <public@hansmi.ch>
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 <public@hansmi.ch>
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
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

76
0006-Avoid-memory-leak-from-ill-formatted-serialization-d.patch Normal file
View File

@ -0,0 +1,76 @@
From babd82e4f83d479e9fb418cd2dd55be6a95a1fe7 Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <public@hansmi.ch>
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 <public@hansmi.ch>
---
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

55
0007-Fix-some-issues-detected-by-fuzzer.patch Normal file
View File

@ -0,0 +1,55 @@
From f2d7dab7119615ab092e3feac2f69dc7b31e94a5 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <freddy77@gmail.com>
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 <freddy77@gmail.com>
---
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

9
usbredir.spec
View File

@ -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 <wuguanghao3@hauwei.com> - 0.8.0-7
- fix memleak in fuzz-test
* Tue Dec 07 2021 wuguanghao <wuguanghao3@hauwei.com> - 0.8.0-6
- fix timeout and abort of fuzz-test