From: libusb T. <tr...@li...> - 2013-02-08 07:56:14
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ----------------------+------------------------- Reporter: meacer | Owner: Type: defect | Status: new Milestone: | Component: libusb-1.0 Keywords: security | Blocked By: Blocks: | ----------------------+------------------------- It's possible to overflow alloc_size in libusb_alloc_transfer by setting iso_packets large enough on 32 bit (iso_packets >= pow(2,32)/12). Once the wrong number of bytes are allocated, it becomes possible to cause invalid memory reads/writes in an executable. libusb_alloc_transfer also accepts a signed integer which is not checked for negative cases, causing os_alloc_size and alloc_size to wrap. Please see the attached patch. It checks for possible overflows/underflows and fails the allocation in advance. -- Ticket URL: <https://libusb.org/ticket/162> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-02-08 08:32:44
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ---------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: ---------------------+------------------------ Comment (by xiaofan): I remember that last time Daniel Drake's idea about this kind of patch was that the library will not check all kind of boundary conditions and it is the user's responsibility. -- Ticket URL: <https://libusb.org/ticket/162#comment:1> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-03-06 23:20:31
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ---------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: ---------------------+------------------------ Comment (by stuge): Replying to [comment:1 xiaofan]: > I remember that last time Daniel Drake's idea about this kind of patch was that the library will not check all kind of boundary conditions and it is the user's responsibility. Well, only things that the user can actually know about. The other parts of the patch look fine. Thanks! -- Ticket URL: <https://libusb.org/ticket/162#comment:2> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-03-06 23:35:30
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ---------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: ---------------------+------------------------ Comment (by meacer): Thanks! We patched a few such cases in Chromium resulting from libusb_alloc_transfer some time ago, but it'll be great if this gets into libusb as well. -- Ticket URL: <https://libusb.org/ticket/162#comment:3> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-03-07 03:34:10
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ---------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: ---------------------+------------------------ Comment (by xiaofan): Where do you store your git tree for Chromium libusb? I do not see any git tree named libusb under http://git.chromium.org/gitweb/ . -- Ticket URL: <https://libusb.org/ticket/162#comment:4> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-03-27 21:18:52
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ---------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: ---------------------+------------------------ Comment (by meacer): Sorry again for the late response, somehow I'm failing to receive the emails. I'm not the original author of the USB API for Chromium, so I'm not sure why there isn't a libusb git tree. You can find the code in trunk here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libusb/src/ Note that we didn't patch libusb in Chromium, but we patched the client code -- albeit somehow arbitrarily by limiting transfer lengths. One of the patches is here: http://src.chromium.org/viewvc/chrome?view=rev&revision=182128 -- Ticket URL: <https://libusb.org/ticket/162#comment:5> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-04-25 22:37:45
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ---------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: ---------------------+------------------------ Comment (by meacer): Ping :) Is this patched planned to be included? Please let me know if there is anything I can do. -- Ticket URL: <https://libusb.org/ticket/162#comment:6> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-05-08 15:01:34
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow ---------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: 1.0.16 | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: ---------------------+------------------------ Changes (by hjelmn): * milestone: => 1.0.16 Comment: I don't see a problem with including this patch. Can you apply this patch to either libusb.git or libusb-darwin.git (clone https://github.com/hjelmn /libusb-darwin.git) and post a patch generated with git format-patch? -- Ticket URL: <https://libusb.org/ticket/162#comment:7> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-05-24 18:59:18
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow -----------------------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: libusb/libusbx 1.2.0 | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: -----------------------------------+------------------------ Comment (by meacer): Please see the new patch generated with git format-patch: http://www.libusb.org/attachment/ticket/162/0001-Fix-integer-flows-in- libusb_alloc_transfer.patch -- Ticket URL: <https://libusb.org/ticket/162#comment:8> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-06-19 13:52:28
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow -----------------------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: libusb/libusbx 1.2.0 | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: -----------------------------------+------------------------ Comment (by hansdegoede): As mentioned in one of the first comment, I'm not sure this really belongs in libusb, libusb is not meant to be exported to a possible hostile party without an additional sanitizing layer added on top. Looking at the complexity of the patch, and combining that with the estimate that many many more such patches will be necessary to avoid libusb crashing to due it being fed invalid input, I don't really think we want to go there. libusb simply was never designed to deal with unsanitized input. -- Ticket URL: <https://libusb.org/ticket/162#comment:9> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: libusb T. <tr...@li...> - 2013-06-20 03:31:34
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow -----------------------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: new Milestone: libusb/libusbx 1.2.0 | Component: libusb-1.0 Resolution: | Keywords: security Blocked By: | Blocks: -----------------------------------+------------------------ Comment (by meacer): I understand that libusb doesn't do any sanitization. However, it's also not possible for the user to sanitize input to libusb_alloc_transfer. The maximum safe allocation size is determined by implementation details such as sizeof(struct libusb_iso_packet_descriptor). It's not possible for the client code to know which value of iso_packets will cause an overflow. This will lead to the client adding arbitrary limits on iso_packets. In the meanwhile, the actual limit can still change when the implementation of libusb_alloc_transfer changes. -- Ticket URL: <https://libusb.org/ticket/162#comment:10> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |
From: Hans P. S. <hp...@bi...> - 2013-06-20 06:35:45
|
On 06/20/13 05:31, libusb Trac wrote: > #162: libusb_alloc_transfer vulnerable to integer overflow/underflow > -----------------------------------+------------------------ > Reporter: meacer | Owner: > Type: defect | Status: new > Milestone: libusb/libusbx 1.2.0 | Component: libusb-1.0 > Resolution: | Keywords: security > Blocked By: | Blocks: > -----------------------------------+------------------------ > > Comment (by meacer): > > I understand that libusb doesn't do any sanitization. However, it's also > not possible for the user to sanitize input to libusb_alloc_transfer. > The maximum safe allocation size is determined by implementation details > such as sizeof(struct libusb_iso_packet_descriptor). It's not possible for > the client code to know which value of iso_packets will cause an overflow. > This will lead to the client adding arbitrary limits on iso_packets. In > the meanwhile, the actual limit can still change when the implementation > of libusb_alloc_transfer changes. > Maybe libusb should define some limits then as macros? In FreeBSD's libusb we have limits on how many isochronous packets can be allocated, and that is not only due to software reasons. --HPS |
From: libusb T. <tr...@li...> - 2013-06-20 08:15:58
|
#162: libusb_alloc_transfer vulnerable to integer overflow/underflow -----------------------------------+------------------------ Reporter: meacer | Owner: Type: defect | Status: closed Milestone: libusb/libusbx 1.2.0 | Component: libusb-1.0 Resolution: wontfix | Keywords: security Blocked By: | Blocks: -----------------------------------+------------------------ Changes (by hansdegoede): * status: new => closed * resolution: => wontfix Comment: Replying to [comment:10 meacer]: > I understand that libusb doesn't do any sanitization. However, it's also not possible for the user to sanitize input to libusb_alloc_transfer. > The maximum safe allocation size is determined by implementation details such as sizeof(struct libusb_iso_packet_descriptor). It's not possible for the client code to know which value of iso_packets will cause an overflow. This will lead to the client adding arbitrary limits on iso_packets. In the meanwhile, the actual limit can still change when the implementation of libusb_alloc_transfer changes. The maximum safe allocation size is an order of magnitude higher then the amount of iso-packets any real world application will ever use. The EHCI controller / driver cannot schedule (and thus does not allow submitting) isoc packets which will execute further into the future then aprox. 1 second (1024 1 ms schedule slots minus some slack). Given that the maximum isoc packet rate is 8000 / sec, this means a maximum value of 8000 for iso_packets, or submission will be guaranteed to fail. Not only is this 8000 orders of magnitude lower then the numbers needed to hit the integer overflow, it is also way higher then any apps will ever use, since isoc is used for streaming media where you want low latency, and using one gargantuan packet which takes a second to complete does not fit well with low latency. ---- I think it is very cool that chromium is using libusb to enable accessing usb devices from the browser, but libusb was never designed to deal with getting "hostile" input from the application side (it should be robust against unsanitized input from a usb device), and I really don't want to start adding code to libusb to sanitize input from the application. I can understand that chromium as part of its sanitizing will want to impose limits in certain cases and if you need input / insight from the libusb side on what a reasonable amount for a limit will be don't hesitate to ask (the libusb-devel mailing-list would be the proper place for this). IE I don't know what you're currenty using as a limit for iso_packets, but I believe that 1024 would be a very reasonable limit (and that no app will ever hit that). As for enforcing the 1024 limit inside libusb, even though that would avoid the harry calculations in the suggested patch, it still feels wrong to me, people may come up with crazy reasons to use more. As hardware evolves such an arbitrary limit may become an issue, which I would rather evolve then having to fix later. OTOH for chromium's use-case such a limit makes perfect sense. ---- With this all said I'm going to close this ticket. Please don't take this as a "we don't care about the chromium use case" message. I do care and will certainly take other patches, or help debug stuff. I just believe that the application data sanitizing does not belong in libusb, they may be a few exceptions where it is way easier to do in libusb then at a higher level, but I don't believe that is the case here. -- Ticket URL: <https://libusb.org/ticket/162#comment:11> libusb <https://libusb.org/> C library for writing portable USB drivers in userspace |