search

libimobiledevice / libusbmuxd

A client library to multiplex connections from and to iOS devices
https://libimobiledevice.org
GNU Lesser General Public License v2.1
589 stars 270 forks source link

unsafe string operations leading to possible memory corruption #50

Closed d-hat closed 5 years ago

d-hat commented 7 years ago

It seems a pretty unlikely scenario to trigger this, but the following code in device_info_from_device_record() and get_next_event() can leave udid unterminated:

        memset(devinfo->udid, '\0', sizeof(devinfo->udid));
        memcpy(devinfo->udid, dev->serial_number, sizeof(devinfo->udid));

Later in usbmuxd_get_device_by_udid(), udid is strcpy()d which could cause all sorts of chaos.

I believe a well-formed serial number should be 40 chars, so the fix (in both locations) is simple:

         memset(devinfo->udid, '\0', sizeof(devinfo->udid));
-        memcpy(devinfo->udid, dev->serial_number, sizeof(devinfo->udid));
+        memcpy(devinfo->udid, dev->serial_number, sizeof(devinfo->udid) - 1);

This might be a security issue if usbmuxd forwards notifications from potentially malicious devices. I believe any such device would need to be physically connected to USB, so exposure is limited.

nikias commented 5 years ago

This is fixed in latest code, by using stpncpy and making sure it is terminated.