New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
ext



Sign in to add a comment
link

Issue 169765: Security: Integer overflow in libusb_alloc_transfer causes Heap-Buffer-Overflow in chrome.usb.isochronousTransfer

Reported by mea...@chromium.org, Jan 14 2013 Project Member

Issue description

There are two problems with libusb_alloc_transfer (src/third_party/libusb/src/libusb/io.c):

struct libusb_transfer * LIBUSB_CALL libusb_alloc_transfer(int iso_packets) {
    ...
    size_t alloc_size = sizeof(struct usbi_transfer)
        + sizeof(struct libusb_transfer)
        + (sizeof(struct libusb_iso_packet_descriptor) * iso_packets)
        + os_alloc_size;
    struct usbi_transfer *itransfer = malloc(alloc_size);
    ...
}

1) It takes a single signed int parameter "iso_packets" but it doesn't check the number being negative. A negative iso_packets causes alloc_size to wrap and cause a crash in Chrome:

        var transferInfo = {
          "direction": "in",
          "endpoint": 0,
          "data": new ArrayBuffer(1),
          "length": 1
        }
        var isoTransferInfo = {
          "packetLength": 0,
          "transferInfo": transferInfo,
          "packets": -100, // passed to libusb_alloc_transfer as iso_packets
        };
        chrome.usb.isochronousTransfer(device, isoTransferInfo, function(){}); // malloc fails, crash on null ptr 

2) On 32 bit, it's possible to overflow alloc_size in libusb_alloc_transfer. libusb_alloc_transfer is called from UsbDevice::IsochronousTransfer as follows:

    struct libusb_transfer* const transfer = libusb_alloc_transfer(packets);
    const uint8 new_endpoint = ConvertTransferDirection(direction) | endpoint;
    libusb_fill_iso_transfer(...);
    libusb_set_iso_packet_lengths(transfer, packet_length);

Tested with Asan, passing a large enough "packets" parameter will overflow alloc_size, and subsequently cause an invalid write in libusb_set_iso_packet_lengths:

        var transferInfo = {
          "direction": "in",
          "endpoint": 0,
          "data": new ArrayBuffer(1),
          "length": 1
        }
        var isoTransferInfo = {
          "packetLength": 0, // defeat the check in UsbDevice::IsochronousTransfer
          "transferInfo": transferInfo,
          "packets": 360000000  // passed to libusb_alloc_transfer as iso_packets
        };
        chrome.usb.isochronousTransfer(device, isoTransferInfo, function(){}); // heap buffer overflow

Assigning low severity since libusb is only called from extension APIs. I've attached a modified version of the sample usb app for reproduction of #2.

Affecting M25 Stable, #1 on all platforms, #2 on 32 bit Windows and Mac
 
usb-knob-app.zip
6.6 KB Download

Comment 1 by jsc...@chromium.org, Jan 14 2013

Labels: -SecSeverity-Low SecSeverity-Medium Feature-Apps-APIs
It seems like this would be medium, since it could allow an extension to break out of the sandbox.

Comment 2 by infe...@chromium.org, Jan 14 2013

Labels: -Pri-0 -Mstone-25 Pri-1 Mstone-24 SecImpacts-Stable
Status: Available
This comment "Affecting M25 Stable, #1 on all platforms, #2 on 32 bit Windows and Mac" is wrong. Stable is m24, so fixing flags.

Comment 3 by infe...@chromium.org, Jan 18 2013

Cc: -gdk@chromium.org
Owner: gdk@chromium.org
Status: Assigned
Gdk@, can you take a look or help to find an owner.

Comment 4 by mea...@chromium.org, Jan 18 2013

Cc: gdk@chromium.org
Owner: mea...@chromium.org
Status: Started
I'm looking into this.

Comment 5 by mea...@chromium.org, Jan 28 2013

I've submitted a patch to libusb about a week ago but haven't heard back yet. Patch for chrome side: https://codereview.chromium.org/12096024/

Comment 6 by infe...@chromium.org, Feb 11 2013

Labels: OS-All

Comment 7 by bugdroid1@chromium.org, Feb 13 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=182128

------------------------------------------------------------------------
r182128 | meacer@chromium.org | 2013-02-13T06:06:04.226007Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/usb/usb_api.cc?r1=182128&r2=182127&pathrev=182128
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/usb/invalid_length_transfer/test.js?r1=182128&r2=182127&pathrev=182128
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/usb/usb_device.cc?r1=182128&r2=182127&pathrev=182128

Add validation to length, packets and packet_length parameters
of the Usb Api. Parameter limits are determined from the following:

packets: 4MB from http://msdn.microsoft.com/en-us/library/windows/hardware/ff538112(v=vs.85).aspx
packet_length: 65k since wMaxPacketSize is 16bit integer: http://msdn.microsoft.com/en-us/library/ee437177.aspx

BUG= 169765 
R=gdk@chromium.org,miket@chromium.org

Review URL: https://chromiumcodereview.appspot.com/12096024
------------------------------------------------------------------------

Comment 8 by infe...@chromium.org, Feb 13 2013

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: Fixed

Comment 9 by scarybea...@gmail.com, Feb 16 2013

M26 @ r182893

Comment 10 by bugdroid1@chromium.org, Feb 16 2013

Project Member
Labels: -Merge-Approved merge-merged-1410
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=182893

------------------------------------------------------------------------
r182893 | cevans@chromium.org | 2013-02-16T02:13:18.907218Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/extensions/api/usb/usb_api.cc?r1=182893&r2=182892&pathrev=182893
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/test/data/extensions/api_test/usb/invalid_length_transfer/test.js?r1=182893&r2=182892&pathrev=182893
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/usb/usb_device.cc?r1=182893&r2=182892&pathrev=182893

Merge 182128
> Add validation to length, packets and packet_length parameters
> of the Usb Api. Parameter limits are determined from the following:
> 
> packets: 4MB from http://msdn.microsoft.com/en-us/library/windows/hardware/ff538112(v=vs.85).aspx
> packet_length: 65k since wMaxPacketSize is 16bit integer: http://msdn.microsoft.com/en-us/library/ee437177.aspx
> 
> BUG= 169765 
> R=gdk@chromium.org,miket@chromium.org
> 
> Review URL: https://chromiumcodereview.appspot.com/12096024

TBR=meacer@chromium.org
Review URL: https://codereview.chromium.org/12283027
------------------------------------------------------------------------

Comment 11 by scarybea...@gmail.com, Feb 20 2013

Labels: Merge-Approved

Comment 12 by scarybea...@gmail.com, Feb 20 2013

Labels: -Mstone-24 -Merge-Approved Mstone-26 Merge-Merged Release-0
Non-trivial merge to M25. I think we can just live with it in M26.

Comment 13 by bugdroid1@chromium.org, Mar 10 2013

Project Member
Labels: -Type-Security -Area-Internals -SecSeverity-Medium -SecImpacts-Beta -Mstone-26 -Feature-Apps-APIs -SecImpacts-Stable Cr-Platform-Apps-API M-26 Security-Impact-Beta Security-Severity-Medium Cr-Internals Security-Impact-Stable Type-Bug-Security

Comment 14 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Impact-Stable Security_Impact-Stable

Comment 15 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Severity-Medium Security_Severity-Medium

Comment 16 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Impact-Beta Security_Impact-Beta

Comment 17 by scarybea...@gmail.com, Mar 23 2013

Labels: CVE-2013-0923

Comment 18 by jsc...@chromium.org, Nov 18 2013

Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Comment 19 by tkonch...@chromium.org, Jun 20 2014

Labels: hasTestcase

Comment 20 by sheriffbot@chromium.org, Jun 14 2016

Project Member
Labels: -security_impact-beta

Comment 21 by sheriffbot@chromium.org, Oct 1 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 22 by sheriffbot@chromium.org, Oct 2 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 23 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Comment 24 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment