[BZ,21295] getaddrinfo: do not overwrite IPv6 IPs with IPv4 when using AF_UNSPEC
diff mbox

Message ID 20170323132130.GA25347@kmeaw.com
State New
Headers show

Commit Message

kmeaw@kmeaw.com March 23, 2017, 1:21 p.m. UTC
CVE-2016-3706 patch introduces a regression which disrupts connectivity
from IPv6-only to dual-stack hosts. This is caused by
convert_hostent_to_gaih_addrtuple which frees the result opposed to
appending to it (prior to the CVE patch in gaih_inet).

This change replaces free(*result) call with a loop which looks for the
pointer to the end of the linked list (&(*result)->next), so successive
calls append the result to the list instead of overwriting it.

Bugzilla entry #21295 describes a way to reproduce the issue.

---
 ChangeLog                   | 5 +++++
 sysdeps/posix/getaddrinfo.c | 8 ++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

--
2.12.0

Comments

Florian Weimer April 20, 2017, 12:41 p.m. UTC | #1
On 03/23/2017 02:21 PM, kmeaw@kmeaw.com wrote:
> CVE-2016-3706 patch introduces a regression which disrupts connectivity
> from IPv6-only to dual-stack hosts. This is caused by
> convert_hostent_to_gaih_addrtuple which frees the result opposed to
> appending to it (prior to the CVE patch in gaih_inet).
> 
> This change replaces free(*result) call with a loop which looks for the
> pointer to the end of the linked list (&(*result)->next), so successive
> calls append the result to the list instead of overwriting it.
> 
> Bugzilla entry #21295 describes a way to reproduce the issue.

I'm trying to write a test for this, but I haven't been successful so 
far.  What's the exact container setup that shows this?  What are its 
network interfaces and sysctl settings for IPv6?

I don't think the bug can happen with nss_dns and the upstream sources. 
We either use AF_UNSPEC and the name4 lookup function, or we have just a 
AF_INET or AF_INET6 lookup, so the current overriding behavior does not 
matter.  This means that in order to reproduce the bug, we'd need a 
custom NSS module which does not implement the name4 lookup function.

It's puzzling that you see a problem on Ubuntu.

Thanks,
Florian
Florian Weimer April 20, 2017, 12:58 p.m. UTC | #2
On 04/20/2017 02:41 PM, Florian Weimer wrote:
> On 03/23/2017 02:21 PM, kmeaw@kmeaw.com wrote:
>> CVE-2016-3706 patch introduces a regression which disrupts connectivity
>> from IPv6-only to dual-stack hosts. This is caused by
>> convert_hostent_to_gaih_addrtuple which frees the result opposed to
>> appending to it (prior to the CVE patch in gaih_inet).
>>
>> This change replaces free(*result) call with a loop which looks for the
>> pointer to the end of the linked list (&(*result)->next), so successive
>> calls append the result to the list instead of overwriting it.
>>
>> Bugzilla entry #21295 describes a way to reproduce the issue.
> 
> I'm trying to write a test for this, but I haven't been successful so 
> far.  What's the exact container setup that shows this?  What are its 
> network interfaces and sysctl settings for IPv6?
> 
> I don't think the bug can happen with nss_dns and the upstream sources. 
> We either use AF_UNSPEC and the name4 lookup function, or we have just a 
> AF_INET or AF_INET6 lookup, so the current overriding behavior does not 
> matter.  This means that in order to reproduce the bug, we'd need a 
> custom NSS module which does not implement the name4 lookup function.

I think I found another corner case: AF_INET6 with AI_ALL and 
AI_V4MAPPED as flags.  This is independent of the host IPv4/IPv6 support 
level.

> It's puzzling that you see a problem on Ubuntu.

This still mystifies me.

Thanks,
Florian
kmeaw@kmeaw.com April 20, 2017, 1:44 p.m. UTC | #3
On Thu, Apr 20, 2017 at 02:41:26PM +0200, Florian Weimer wrote:
> On 03/23/2017 02:21 PM, kmeaw@kmeaw.com wrote:
> >CVE-2016-3706 patch introduces a regression which disrupts connectivity
> >from IPv6-only to dual-stack hosts. This is caused by
> >convert_hostent_to_gaih_addrtuple which frees the result opposed to
> >appending to it (prior to the CVE patch in gaih_inet).
> >
> >This change replaces free(*result) call with a loop which looks for the
> >pointer to the end of the linked list (&(*result)->next), so successive
> >calls append the result to the list instead of overwriting it.
> >
> >Bugzilla entry #21295 describes a way to reproduce the issue.
> 
> I'm trying to write a test for this, but I haven't been successful
> so far.  What's the exact container setup that shows this?  What are
> its network interfaces and sysctl settings for IPv6?

I have a basic configuration required for Docker to have IPv6-enabled
containers:

# this is a real(hardware) host:

kmeaw@yaws-5477-05:~$ cat /etc/docker/daemon.json 
{
    "fixed-cidr-v6": "2a02:6b8:b010:702c:3027::/80",
    "ipv6": true
}

kmeaw@yaws-5477-05:~$ docker network inspect bridge
[
    {
        "Name": "bridge",
        "Id": "ec8176b9886736996f1a8f6854df2716d84c7ea2dae6054a7278f182585c65b8",
        "Scope": "local",
        "Driver": "bridge",
        "EnableIPv6": true,
        "IPAM": {
            "Driver": "default",
            "Options": null,
            "Config": [
                {
                    "Subnet": "172.17.0.0/16",
                    "Gateway": "172.17.0.1"
                },
                {
                    "Subnet": "2a02:6b8:b010:702c:3027::/80",
                    "Gateway": "2a02:6b8:b010:702c:3027::1"
                }
            ]
        },
        "Internal": false,
        "Containers": {
            "9a1313723f65962d86131e59f296fd32d878464b55202d6ede1f66ce3ff26d03": {
                "Name": "suspicious_mestorf",
                "EndpointID": "0d5591769edc79b19f7112fa95a9247d44f4de890062823d7ce12ccb83333dcb",
                "MacAddress": "02:42:ac:11:00:03",
                "IPv4Address": "172.17.0.3/16",
                "IPv6Address": "2a02:6b8:b010:702c:3027:242:ac11:3/80"
            }
        },
        "Options": {
            "com.docker.network.bridge.default_bridge": "true",
            "com.docker.network.bridge.enable_icc": "true",
            "com.docker.network.bridge.enable_ip_masquerade": "true",
            "com.docker.network.bridge.host_binding_ipv4": "0.0.0.0",
            "com.docker.network.bridge.name": "docker0",
            "com.docker.network.driver.mtu": "1500"
        },
        "Labels": {}
    }
]

kmeaw@yaws-5477-05:~$ cat /etc/sysctl.conf  | grep -v '^#' | grep ipv6
net.ipv6.conf.eth0.autoconf = 2
net.ipv6.conf.eth0.acecpt_ra = 1

# container:

root@9a1313723f65:~# ip -6 a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 state UNKNOWN qlen 1
    inet6 ::1/128 scope host
       valid_lft forever preferred_lft forever
154: eth0@if155: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP
    inet6 2a02:6b8:b010:702c:3027:242:ac11:3/80 scope global nodad
       valid_lft forever preferred_lft forever
    inet6 fe80::42:acff:fe11:3/64 scope link
       valid_lft forever preferred_lft forever
root@9a1313723f65:~# ip -6 ro
2a02:6b8:b010:702c:3027::/80 dev eth0  proto kernel  metric 256
fe80::/64 dev eth0  proto kernel  metric 256
default via 2a02:6b8:b010:702c:3027::1 dev eth0  metric 1024
root@9a1313723f65:~# ip -4 a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
154: eth0@if155: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP
    inet 172.17.0.3/16 scope global eth0
       valid_lft forever preferred_lft forever
root@9a1313723f65:~# ip -4 ro
default via 172.17.0.1 dev eth0
172.17.0.0/16 dev eth0  proto kernel  scope link  src 172.17.0.3
root@9a1313723f65:~# sysctl -a 2> /dev/null | grep disable_ipv6   
net.ipv6.conf.all.disable_ipv6 = 0
net.ipv6.conf.default.disable_ipv6 = 0
net.ipv6.conf.eth0.disable_ipv6 = 0
net.ipv6.conf.lo.disable_ipv6 = 0

To reproduce the problem, you need global scope (RFC1918 is fine) for
both IPv4 and IPv6. If you don't want to properly setup IPv6 networking
in docker, you can try something like this:

  kmeaw-pc# docker ps
  CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
  6e6d4b17326a        ubuntu:12.04        "bash"              3 minutes ago       Up 3 minutes                            vibrant_rosalind
  kmeaw-pc# docker inspect --format '{{.State.Pid}}' 6e6d4b17326a
  23348
  kmeaw-pc# nsenter --net=/proc/23348/ns/net                     
  kmeaw-pc# echo 0 > /proc/sys/net/ipv6/conf/eth0/disable_ipv6
  kmeaw-pc# ip -6 a add 2001:db8::1/64 dev eth0

> I don't think the bug can happen with nss_dns and the upstream
> sources. We either use AF_UNSPEC and the name4 lookup function, or
> we have just a AF_INET or AF_INET6 lookup, so the current overriding
> behavior does not matter.  This means that in order to reproduce the
> bug, we'd need a custom NSS module which does not implement the
> name4 lookup function.

That's right. Latest upstream libnss_dns has name4:

[kmeaw@fill ~]$ strings /lib/libnss_dns.so|grep name4
_nss_dns_gethostbyname4_r
[kmeaw@fill ~]$ /lib/libc.so.6 | head -n1
GNU C Library (GNU libc) stable release version 2.25, by Roland McGrath et al.

But Ubuntu 12.04 does not:

root@6e6d4b17326a:/# dpkg-query -s libc6 | grep Version:
Version: 2.15-0ubuntu10.17
root@6e6d4b17326a:/# strings /lib/x86_64-linux-gnu/libnss_dns.so.2|grep name4
root@6e6d4b17326a:/# strings /lib/x86_64-linux-gnu/libnss_dns.so.2|grep name 
__ns_name_unpack
__ns_name_ntop
_nss_dns_gethostbyname2_r
_nss_dns_gethostbyname_r
_nss_dns_gethostbyname3_r
__dn_skipname
_nss_dns_getnetbyname_r
_nss_dns_getcanonname_r
root@6e6d4b17326a:/# 

> It's puzzling that you see a problem on Ubuntu.

At some point in time upstream libc have implemented gethostbyname4
interface in libnss_dns, then CVE-2016-3706 was fixed.
That fix introduces a bug (regression) which is actually never seen if
you are using upstream version of libc, because it touches only
gethostbyname[23] code paths.
However, you can hide gethostbyname4 implementation from the dynamic
linker:

[kmeaw@fill tmp]$ ./foo | uniq
2a02:6b8::2:242
87.250.250.242
[kmeaw@fill tmp]$ sed -e 's/gethostbyname4/gethostbyname5/g' -i.bak ./libnss_dns.so
[kmeaw@fill tmp]$ ./foo | uniq
87.250.250.242
[kmeaw@fill tmp]$ 

Ubuntu have backported the CVE-2016-3706 fix but have not backported new
gethostbyname4 interface implementation for libnss_dns. That's why this
issue is observable in Ubuntu 12.04.
Florian Weimer April 20, 2017, 2:05 p.m. UTC | #4
On 04/20/2017 03:44 PM, kmeaw@kmeaw.com wrote:

> But Ubuntu 12.04 does not:
> 
> root@6e6d4b17326a:/# dpkg-query -s libc6 | grep Version:
> Version: 2.15-0ubuntu10.17
> root@6e6d4b17326a:/# strings /lib/x86_64-linux-gnu/libnss_dns.so.2|grep name4
> root@6e6d4b17326a:/# strings /lib/x86_64-linux-gnu/libnss_dns.so.2|grep name
> __ns_name_unpack
> __ns_name_ntop
> _nss_dns_gethostbyname2_r
> _nss_dns_gethostbyname_r
> _nss_dns_gethostbyname3_r
> __dn_skipname
> _nss_dns_getnetbyname_r
> _nss_dns_getcanonname_r
> root@6e6d4b17326a:/#

Very odd.  This is due to the patch 
debian/patches/all/fedora-nss_dns-gethostbyname4-disable.diff:

# Description: disable _nss_dns_gethostbyname4_r for the moment, as it
#  causes problems for IPv6 users; patch from Fedora by Jakub Jelinek
# Ubuntu: https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/313218
# Upstream: https://bugzilla.redhat.com/show_bug.cgi?id=459756

So your reproducer is indeed very Ubuntu-specific at this point.  (Red 
Hat Enterprise Linux 6 backported the single-request resolver option 
instead, which can be used to work around issues with the parallel lookup.)

We still need to fix this upstream because it's also a bug with AI_ALL 
and AI_V4MAPPED (see my other message).

Thanks,
Florian

Patch
diff mbox

diff --git a/ChangeLog b/ChangeLog
index 7809c3dc2b..56179d6164 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@ 
+2017-03-23  Dmitry Bilunov  <kmeaw@kmeaw.com>
+
+       * sysdeps/posix/getaddrinfo.c (onvert_hostent_to_gaih_addrtuple):
+       do not overwrite list of IPv6 addresses with IPv4; merge them instead.
+
 2017-03-22  Zack Weinberg  <zackw@panix.com>

        * stdio-common/bug25.c: Include stdlib.h.
diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index eed7264850..cf1d99b2e2 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -190,16 +190,16 @@  gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,

 /* Convert struct hostent to a list of struct gaih_addrtuple objects.
    h_name is not copied, and the struct hostent object must not be
-   deallocated prematurely.  *RESULT must be NULL or a pointer to an
-   object allocated using malloc, which is freed.  */
+   deallocated prematurely.  *RESULT must be NULL or a pointer to a
+   linked-list, which is scanned to the end.  */
 static bool
 convert_hostent_to_gaih_addrtuple (const struct addrinfo *req,
                                   int family,
                                   struct hostent *h,
                                   struct gaih_addrtuple **result)
 {
-  free (*result);
-  *result = NULL;
+  while (*result)
+    result = &(*result)->next;

   /* Count the number of addresses in h->h_addr_list.  */
   size_t count = 0;