usbmuxd required in main for libiphone

[Sebastien Bacher]

Binary package hint: usbmuxd

https://wiki.ubuntu.com/MainInclusionReportUsbmuxd

[Loïc Minier (lool)]

usbmuxd runs as a separate user and has udev integration; I don't quite know whether there is a central package to add support for devices over udev, but the approach looks good enough.

Has .symbols file, looks overall good. Apparently no testsuite.

[Loïc Minier (lool)]

There are a couple of bad warnings; looks like a missing pthread include:
/build/buildd/usbmuxd-1.0.0/libusbmuxd/libusbmuxd.c:298: warning: implicit declaration of function 'pthread_create'

[Alexander Sack (asac)]

Packaing:
looks good.

QA:
Please subscribe desktop team to bug reports if not needed

Upstream code:
Source code wise i checked how the input data from device is processed and found a nit:

in device_data_input the buffer bounds are not properly/explicitly checked in daemon.c, like

in connection_device_input:
                memcpy(conn->ib_buf + conn->ib_size, payload, payload_length);
should only happen if payload_length < CONN_INBUF_SIZE - conn->ib_size

in device_data_input:
                memcpy(dev->pktbuf + dev->pktlen, buffer, length);
should only happen if length < DEV_PKTBUF_SIZE - dev->pkglen

                memcpy(dev->pktbuf, buffer, length);
should only happen if length < DEV_PKTBUF_SIZE

in client.c:
                memcpy(client->ob_buf + client->ob_size + sizeof(hdr), payload, payload_length);

has the same issue ... though that doesnt process stuff coming from the device - so isnt that critical imo.

[Alexander Sack (asac)]

right. the implicit declaration warnings should be fixed too

[Alexander Sack (asac)]

also maybe to consider: drop_privileges should be the default when running as root ... feels safer.

[Hector Martin (marcan42)]

The first memcpy() is already guarded by a check right before it.
The second memcpy is potentially exploitable, I fixed that.
The third one isn't, because the length is always < USB_MRU which is < the buffer size (and the buffer is empty in this case), though I added a paranoia check anyways in case the USB implementation changes in the future.
The fourth memcpy is also guarded by a check right before it, not to mention that it's for data going _to_ the client, not coming from it, and we always generate that data ourselves.
The data from the client is not memcpy()ed, it's just recv()ed straight into the buffer, and the amount is also checked against the buffer size.

So it looks like I only had one exploitable bug, not four.

It's also worth noting that integer overflow bugs are not possible, neither on the device input nor on the client input, because packet lengths are bounded by the USB MRU on the device side (and we check header lengths against it) and we check header length (no math, and this is unsigned) against the client buffer size. I changed the buffer/packet sizes to unsigned for good hygiene anyway.

I also added underflow checks to device_data_input, to make sure packets are large enough for the required headers. This isn't exploitable either (buffers are always large enough, all it could do is result in garbage headers), but again, it's good hygiene.

The new version is in Git, let me know if it's acceptable and I'll make a release.

[Alexander Sack (asac)]

On Sat, Jan 23, 2010 at 11:22:02PM -0000, Hector Martin wrote:
>
> The new version is in Git, let me know if it's acceptable and I'll make
> a release.
>

Thanks a lot for checking and fixing these.

Any chance to also fix the compiler warnings in
https://bugs.edge.launchpad.net/ubuntu/+source/usbmuxd/+bug/494549/comments/2 ?

 - Alexander

[Hector Martin (marcan42)]

Those warnings were already fixed together with the security issue.

[Martin Pitt (pitti)]

Looks like the issues got all addressed now, thanks! I promoted the package now.