| Message ID | 20190214201939.494-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ccid-card-passthru: check buffer size parameter | expand |
Hi On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > The right side of the comparison is the return value of can_read(): > VSCARD_IN_SIZE - card->vscard_in_pos. > Since the 'size' argument of chardev::read() is bound to > what chardev::can_read() returns, this condition can never happen. I think so too, because vscard_in_pos is unchanged between the 2 callbacks (or set to 0 in break event). > > Add an assertion, which will always fail if card->vscard_in_pos >= > VSCARD_IN_SIZE), since size > 0. If "size > VSCARD_IN_SIZE - card->vscard_in_pos" this is a chardev bug. But which backend does that? Iow, did we ever reach the "no room for data" error? > > This is a quick fix for CVE-2018-18438 "Integer overflow in > ccid_card_vscard_read() allows memory corruption". I have a hard time to find how that memory corruption can happen. It would be a broken chardev (one calling qemu_chr_be_write() with a size bigger than qemu_chr_be_can_write()). It would need to be fixed. But which one does that? > > Fixes: CVE-2018-18438 > Reported-by: Arash Tohidi Chafi <tohidi.arash@gmail.com> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/usb/ccid-card-passthru.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c > index 8bb1314f49..1676b5fc05 100644 > --- a/hw/usb/ccid-card-passthru.c > +++ b/hw/usb/ccid-card-passthru.c > @@ -264,24 +264,12 @@ static void ccid_card_vscard_handle_message(PassthruState *card, > } > } > > -static void ccid_card_vscard_drop_connection(PassthruState *card) > -{ > - qemu_chr_fe_deinit(&card->cs, true); > - card->vscard_in_pos = card->vscard_in_hdr = 0; > -} > - > static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) > { > PassthruState *card = opaque; > VSCMsgHeader *hdr; > > - if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { > - error_report("no room for data: pos %u + size %d > %" PRId64 "." > - " dropping connection.", > - card->vscard_in_pos, size, VSCARD_IN_SIZE); > - ccid_card_vscard_drop_connection(card); > - return; > - } > + assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos); > assert(card->vscard_in_hdr < VSCARD_IN_SIZE); > memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size); > card->vscard_in_pos += size; > -- > 2.20.1 >
On 2/15/19 11:59 AM, Marc-André Lureau wrote: > Hi > > On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé > <philmd@redhat.com> wrote: >> >> The right side of the comparison is the return value of can_read(): >> VSCARD_IN_SIZE - card->vscard_in_pos. >> Since the 'size' argument of chardev::read() is bound to >> what chardev::can_read() returns, this condition can never happen. > > I think so too, because vscard_in_pos is unchanged between the 2 > callbacks (or set to 0 in break event). > >> >> Add an assertion, which will always fail if card->vscard_in_pos >= >> VSCARD_IN_SIZE), since size > 0. > > If "size > VSCARD_IN_SIZE - card->vscard_in_pos" this is a chardev > bug. But which backend does that? > > Iow, did we ever reach the "no room for data" error? > >> >> This is a quick fix for CVE-2018-18438 "Integer overflow in >> ccid_card_vscard_read() allows memory corruption". > > I have a hard time to find how that memory corruption can happen. It > would be a broken chardev (one calling qemu_chr_be_write() with a size > bigger than qemu_chr_be_can_write()). It would need to be fixed. But It will :) > which one does that? Arash or Prasad can you help us here? Do you have a reproducer? >> >> Fixes: CVE-2018-18438 >> Reported-by: Arash Tohidi Chafi <tohidi.arash@gmail.com> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/usb/ccid-card-passthru.c | 14 +------------- >> 1 file changed, 1 insertion(+), 13 deletions(-) >> >> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c >> index 8bb1314f49..1676b5fc05 100644 >> --- a/hw/usb/ccid-card-passthru.c >> +++ b/hw/usb/ccid-card-passthru.c >> @@ -264,24 +264,12 @@ static void ccid_card_vscard_handle_message(PassthruState *card, >> } >> } >> >> -static void ccid_card_vscard_drop_connection(PassthruState *card) >> -{ >> - qemu_chr_fe_deinit(&card->cs, true); >> - card->vscard_in_pos = card->vscard_in_hdr = 0; >> -} >> - >> static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) >> { >> PassthruState *card = opaque; >> VSCMsgHeader *hdr; >> >> - if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { >> - error_report("no room for data: pos %u + size %d > %" PRId64 "." >> - " dropping connection.", >> - card->vscard_in_pos, size, VSCARD_IN_SIZE); >> - ccid_card_vscard_drop_connection(card); >> - return; >> - } >> + assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos); >> assert(card->vscard_in_hdr < VSCARD_IN_SIZE); >> memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size); >> card->vscard_in_pos += size; >> -- >> 2.20.1 >>
+-- On Mon, 18 Feb 2019, Philippe Mathieu-Daudé wrote --+ | On 2/15/19 11:59 AM, Marc-André Lureau wrote: | Arash or Prasad can you help us here? Do you have a reproducer? No, we don't have a reproducer handy I'm afraid. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Hi On Thu, Feb 21, 2019 at 12:06 PM P J P <ppandit@redhat.com> wrote: > > +-- On Mon, 18 Feb 2019, Philippe Mathieu-Daudé wrote --+ > | On 2/15/19 11:59 AM, Marc-André Lureau wrote: > | Arash or Prasad can you help us here? Do you have a reproducer? > > No, we don't have a reproducer handy I'm afraid. Without a reproducer or convincing arguments, do we have a CVE?
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c index 8bb1314f49..1676b5fc05 100644 --- a/hw/usb/ccid-card-passthru.c +++ b/hw/usb/ccid-card-passthru.c @@ -264,24 +264,12 @@ static void ccid_card_vscard_handle_message(PassthruState *card, } } -static void ccid_card_vscard_drop_connection(PassthruState *card) -{ - qemu_chr_fe_deinit(&card->cs, true); - card->vscard_in_pos = card->vscard_in_hdr = 0; -} - static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) { PassthruState *card = opaque; VSCMsgHeader *hdr; - if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { - error_report("no room for data: pos %u + size %d > %" PRId64 "." - " dropping connection.", - card->vscard_in_pos, size, VSCARD_IN_SIZE); - ccid_card_vscard_drop_connection(card); - return; - } + assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos); assert(card->vscard_in_hdr < VSCARD_IN_SIZE); memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size); card->vscard_in_pos += size;
The right side of the comparison is the return value of can_read(): VSCARD_IN_SIZE - card->vscard_in_pos. Since the 'size' argument of chardev::read() is bound to what chardev::can_read() returns, this condition can never happen. Add an assertion, which will always fail if card->vscard_in_pos >= VSCARD_IN_SIZE), since size > 0. This is a quick fix for CVE-2018-18438 "Integer overflow in ccid_card_vscard_read() allows memory corruption". Fixes: CVE-2018-18438 Reported-by: Arash Tohidi Chafi <tohidi.arash@gmail.com> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/usb/ccid-card-passthru.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)