[ppp] sys-solaris: Overflow in the sockaddr struct

[raminfp]

Hi @paulusmack

In the function get_hw_addr_dlpi - obtain the hardware address using DLPI, There is bug buffer overflow when pass adrlen = reply.prim.info_ack.dl_addr_length; if greater is 14, We have overflow in memcpy(hwaddr->sa_data, adrp, adrlen); because length buffer hwaddr->sa_data is 14,

    struct sockaddr {
               sa_family_t sa_family;
               char        sa_data[14];
           }

https://github.com/paulusmack/ppp/blob/master/pppd/sys-solaris.c#L2354

    adrlen = reply.prim.info_ack.dl_addr_length; // length of the dl address.
    adrp = (unsigned char *)&reply + reply.prim.info_ack.dl_addr_offset;

#if DL_CURRENT_VERSION >= 2
    if (reply.prim.info_ack.dl_sap_length < 0)
	adrlen += reply.prim.info_ack.dl_sap_length;
    else
	adrp += reply.prim.info_ack.dl_sap_length;
#endif

    hwaddr->sa_family = AF_UNSPEC;
    memcpy(hwaddr->sa_data, adrp, adrlen);

Thanks, Ramin

[paulusmack]

There does seem to be a potential buffer overflow, and more defensive coding would be appropriate.

How was this found? Just by inspection, or was some incorrect behaviour observed in some circumstance?

Given that the contents of 'reply' are supplied by Solaris kernel software, I don't see any exploitable vulnerability here.

[Neustradamus]

@paulusmack: Have you found a solution?

[Neustradamus]

@carlsonj: It is for you, what do you think?

[carlsonj]

All of the code in this module assumes that the MAC address length is in fact exactly 6. There are even hard-coded constants involved. With the callers of this static function, you'll get bad results if it's less than 6 or greater than 14.

This exactly-6 assumption was true as of Solaris 9. I know the InfiniBand guys ran into all sorts of problems with their overly-long QP addresses during Solaris 10 development, and it's possible that there may be real devices in the field like that now. I don't know for certain, though. All of the hardware that I ran into back in my Sun days was either plain Ethernet or emulated Ethernet as the path of least resistance. Or was so absurdly expensive that nobody ever used it.

So, maybe an overrun, if someone has some really exotic and unusual hardware or a custom DLPIv2 driver of some sort, but quite unlikely in my opinion.

If you were concerned and wanted to fix it cleanly, I'd suggest something like this:

-    memcpy(hwaddr->sa_data, adrp, adrlen);
+    if (adrlen < (int)sizeof(hwaddr->sa_data))
+      memset(hwaddr->sa_data + adrlen, 0, sizeof(hwaddr->sa_data) - adrlen);
+    if (adrlen > (int)sizeof(hwaddr->sa_data))
+      adrlen = (int)sizeof(hwaddr->sa_data);
+    memcpy(hwaddr->sa_data, adrp, adrlen);

But if you just wanted to assert the conditions rather than working around excursions, I'd be happy with that as well, as I don't think this actually occurs anywhere.

[paulusmack]

This doesn't seem like it is any more than a theoretical problem, really.