buglwIP - A Lightweight TCP/IP stack - Bugs: bug #60569, lwip_gethostbyname(_r) does not...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #60569: lwip_gethostbyname(_r) does not handle IPv4/IPv6 correctly

Submitted by:  Patrik Lantto <patriklantto>
Submitted on:  Mon 10 May 2021 07:27:17 AM UTC  
 
Category:  DNS Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  git head

Wed 26 May 2021 06:45:02 AM UTC, comment #5: 

As there has not been any further response on this topic, I can only assume there is some hesitation on applying the patch without the support for IPv6 (even though the previous version hardcoded the h_addrtype to AF_INET).

I have now updated the patch so that lwip_gethostbyname and lwip_gethostbyname_r supports AF_INET and AF_INET6, and correctly sets h_addrtype, h_length and the h_addr_list entry accordingly (i.e. AF_INET/struct in_addr and AF_INET6/struct in6_addr respectively).

In addition I have also added two routines lwip_gethostbyname2 and lwip_gethostbyname2_r corresponding to the glibc2 routines gethostbyname2 and gethostbyname2_r. There routines permits to specify the address family. For a caller that do not expect gethostbyname to support AF_INET6, the compatibility layer can now choose to define gethostbyname as lwip_gethostbyname2 and specify AF_INET.

While updating the implementation, I made one common “helper” routine that the four other routines are calling, and additionally corrected a somewhat strange usage of the memory alignment that could cause lwip_gethostbyname_r to return ERANGE even if the buffer was large enough.

New patch is attached.

(file #51481)

Patrik Lantto <patriklantto>
Thu 13 May 2021 06:49:05 PM UTC, comment #4: 

While lwip_gethostbyname and lwip_gethostbyname_r may of course return whatever they like in the list of addresses (e.g. an ip_addr_t), the issue is that LWIP_COMPAT_SOCKETS maps the corresponding POSIX routines, and thus it should be be expected that they behave the same way.

For the POSIX gethostbyname/gethostbyname_r, hostent returns either a IPv4 address or a IPv6 address. For an IPv4 address, h_addrtype is set to AF_INET, the address pointers are struct in_addr and h_length would then be sizeof (struct in_addr).

For IPv6, it is instead AF_INET6, struct in6_addr and sizeof (struct in6_addr) respectively.

A caller that expects a IPv4 address could copy the address using 'memcpy(dest, hostent->h_addr_list[0], hostent->h_length);'

The way lwip_gethostbyname currently does is that it always sets h_addrtype to AF_INET (disregarding if it is IPv4 or IPv6), and additionally the length is set to sizeof (ip_addr_t) which is even longer that in6_addr. Thus any application expecting to use h_length for the memcpy would corrupt the memory event if the destination is an in6_addr.

So I would state the main issue here is that it is not compatible with the POSIX gethostbyname/gethostbyname_r.

But an additional sub-issue is that it sets h_addrtype to AF_INET even if the address would be an IPv6 address (which it may be since netconn_gethostbyname is used which could return an IPv6 address if IPv4 fails).

The provided patch will still only return AF_INET, but at least it will be compatible with gethostbyname, and additionally ensure an IPv6 address is never returned.

Patrik Lantto <patriklantto>
Thu 13 May 2021 11:21:53 AM UTC, comment #3: 

Maybe one case where it could be a problem is a v4 only build resolving a hostname only return a v6 address? Not sure if that is supported.

Erik Ekman <yarrick>
Project Member
Thu 13 May 2021 11:20:17 AM UTC, comment #2: 

ip_addr_t is sized to fit any IP address supported in the build - so there should not be any out of bounds writes at least. Did you see memory corruption happen?

Switching from ipaddr_ntoa to ip4addr_ntoa seems like a step away from v4/v6 compatibility as the former handles both.

Erik Ekman <yarrick>
Project Member
Tue 11 May 2021 07:46:33 AM UTC, comment #1: 

Patch to correct behaviour of gethostbyname() and gethostbyname_r() for AF_INET has been attached.

(file #51417)

Patrik Lantto <patriklantto>
Mon 10 May 2021 07:27:17 AM UTC, original submission:  

lwip_gethostbyname and lwip_gethostbyname_r are currently always setting the h_addrtype field of struct hostent to AF_INET disregarding if the result is IPv4 or IPv6. Moreover, the h_length field is set to sizeof (ip_addr_t) instead of sizeof (struct in_addr) or sizeof (struct in6_addr) respectively. An application that uses the h_length field to copy the adress would potentially corrupt memory; the amount of corrupted bytes depends on whether lwIP is compiled with IPv6 support or not.

Since h_addrtype is hardcoded to AF_INET, a quick solution (or an intermediate step) is to use netconn_gethostbyname_addrtype instead of netconn_gethostbyname, and specify NETCONN_DNS_IPV4 to ensure only an IPv4 address is returned, and set h_length to sizeof (struct in_addr). A long term solution is to look at the address type and set the fields of hostent (including h_addrtype and h_length dynamically).

Patrik Lantto <patriklantto>

 

Attached Files
file #51481:  60569-gethostbyname2.patch added by patriklantto (14KiB - application/octet-stream)
file #51417:  60569-gethostbyname.patch added by patriklantto (3KiB - application/octet-stream - Patch to make gethostbyname behave correctly for AF_INET)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by yarrick (Posted a comment)
  • -email is unavailable- added by patriklantto (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-05-26 patriklantto Attached File- => Added 60569-gethostbyname2.patch, #51481
    2021-05-11 patriklantto Attached File- => Added 60569-gethostbyname.patch, #51417

    Back to the top


    Powered by Savane 3.7