Re: qemu - CVE-2018-19665: bt subsystem mishandles negative length variables
Hi Adrian, > On 1/12/19 5:52 PM, Hugo Lefeuvre wrote: > > the subsystem doesn't seem to be very actively maintained and that the user > > base is quite small, it is maybe better to mark this no-dsa in stretch and > > Please don't forget thet Debian has derivates that do not get summed up in > popcon.d.o. So the user base might be bigger than assumed. Right, but I was actually strictly speaking about the bluetooth subsystem, quoting qemu's upstream[0]. cheers Hugo [0] https://patchwork.kernel.org/patch/10678421/ -- Hugo Lefeuvre (hle)|www.owl.eu.com RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C signature.asc Description: PGP signature
Re: qemu - CVE-2018-19665: bt subsystem mishandles negative length variables
On 1/12/19 5:52 PM, Hugo Lefeuvre wrote: the subsystem doesn't seem to be very actively maintained and that the user base is quite small, it is maybe better to mark this no-dsa in stretch and Please don't forget thet Debian has derivates that do not get summed up in popcon.d.o. So the user base might be bigger than assumed. Regards, Adrian.
Re: qemu - CVE-2018-19665: bt subsystem mishandles negative length variables
> Anyways, given that the patch is quite large (though straightforward), that
> the subsystem doesn't seem to be very actively maintained and that the user
> base is quite small, it is maybe better to mark this no-dsa in stretch and
> jessie.
... but if we manage to trim down upstream's patch to just a few lines,
it could still be worth it.
I have taken upstream's patch and got rid of all type related changes
which don't have any security related impact. In fact they don't solve
the 'negative len' issue, these changes are just equivalent to moving the
size_t cast a few instructions earlier.
These changes might make sense in a refactoring perspective but this is
just noise in our case.
The resulting patch is tiny:
diff --git a/bt-host.c b/bt-host.c
index 2f8f631c25..b73a44d07d 100644
--- a/bt-host.c
+++ b/bt-host.c
@@ -113,6 +113,7 @@ static void vhci_host_send(void *opaque,
static uint8_t buf[4096];
buf[0] = type;
+assert((size_t) len < sizeof(buf));
memcpy(buf + 1, data, len);
while (write(s->fd, buf, len + 1) < 0)
diff --git a/hw/bt/hci-csr.c b/hw/bt/hci-csr.c
index 0341ded50c..26bd516d31 100644
--- a/hw/bt/hci-csr.c
+++ b/hw/bt/hci-csr.c
@@ -320,18 +320,18 @@ static int csrhci_write(struct Chardev *chr,
struct csrhci_s *s = (struct csrhci_s *)chr;
int total = 0;
-if (!s->enable)
+if (!s->enable || len <= 0)
return 0;
for (;;) {
int cnt = MIN(len, s->in_needed - s->in_len);
-if (cnt) {
-memcpy(s->inpkt + s->in_len, buf, cnt);
-s->in_len += cnt;
-buf += cnt;
-len -= cnt;
-total += cnt;
-}
+assert(cnt > 0);
+
+memcpy(s->inpkt + s->in_len, buf, cnt);
+s->in_len += cnt;
+buf += cnt;
+len -= cnt;
+total += cnt;
if (s->in_len < s->in_needed) {
break;
3 lines changed, omitting indentation related diff. Given that this
issue might allow host side DoS/memory corruption I don't think this is
exaggerated.
The only think which is still unclear to me is why the patch is checking
using assert(). If these assert() calls are standard ansi ones, then their
failure would stop the whole qemu process which is not exactly what we
want right?
cheers,
Hugo
--
Hugo Lefeuvre (hle)|www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C
signature.asc
Description: PGP signature
qemu - CVE-2018-19665: bt subsystem mishandles negative length variables
Hi, I had a look at CVE-2018-19665 regarding qemu in oldstable/stable. summary: the bluetooth subsystem uses signed length variables at multiple places. These length variables are used, among others, in memcpy calls. A malicious guest VM could attempt to crash the host by passing negative len values (in fact, huge len values interpreted as negative numbers) to these functions. The suggested patch[0] changes the type of these length variables to size_t (unsigned) and adds a few assert calls to make sure the code is also resilient again large values of len. First, it is not completely clear to me to what extent this length variable is under the control of guest VM users. say, if guest kernel drivers process calls first, then these large/negative values are likely to be rejected before they have even reached the affected qemu code. Under this hypothesis, guest VM users would need to have full control over the guest kernel to exploit this vulnerability (making exploit more difficult in real envs ?). I might be wrong on this point due to my limited knowledge of this code-base. Anyways, given that the patch is quite large (though straightforward), that the subsystem doesn't seem to be very actively maintained and that the user base is quite small, it is maybe better to mark this no-dsa in stretch and jessie. Cheers, Hugo [0] https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03570.html -- Hugo Lefeuvre (hle)|www.owl.eu.com RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C signature.asc Description: PGP signature
