Bug 9666 - Broken filtering of link-local addresses
Broken filtering of link-local addresses
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: Other
4.0.3
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-18 03:05 UTC by Landon Fuller
Modified: 2013-03-04 09:26 UTC (History)
4 users (show)

See Also:


Attachments
Simple test of getnameinfo() and flags (4.04 KB, application/octet-stream)
2013-02-18 05:55 UTC, Timur Bakeyev
no flags Details
Patch on interfaces.c (57 bytes, text/plain)
2013-02-18 07:12 UTC, Timur Bakeyev
no flags Details
Patch on interfaces.c (1.50 KB, text/plain)
2013-02-18 07:14 UTC, Timur Bakeyev
no flags Details
git-am fix for mater. (2.15 KB, patch)
2013-02-28 00:28 UTC, Jeremy Allison
no flags Details
Updated git-am fix for master. (2.07 KB, patch)
2013-02-28 00:48 UTC, Jeremy Allison
rsharpe: review+
Details
Patch for samba_dnsupdate (898 bytes, patch)
2013-03-02 05:17 UTC, Landon Fuller
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Landon Fuller 2013-02-18 03:05:02 UTC
On FreeBSD, I noticed that link-local IPv6 addresses were being enumerated as valid IPs by Samba, as well as published to DNS. This results in replication breaking if DCs are not on the same LAN, or if they do not accept IPv6 traffic. On Linux, the addresses are also being enumerated, but are filtered out in the samba_dnsupdate script.

Looking into the issue, I found two bugs:

1) There is code in get_interfaces() to filter out link-local IPv6 addresses, but it is short-circuited by an earlier check for the IFF_BROADCAST flag:

http://gitweb.samba.org/?p=samba.git;a=blob;f=lib/socket/interfaces.c;h=74c642372a24323befae23534d1f525991a96fab;hb=HEAD#l180

This appears to have been added in a merge earlier by abartlett; I'm not sure whether the correct/intended behavior is to filter out link-local addresses, or if resolving this issue would break other users of get_interfaces().

2) There is code in samba_dnsupdate to filter out link-local IPv6 addresses prior to publishing them, but it does not examine the 10-bit address prefix to determine whether an address is valid; rather, it is reliant on the particular format Linux returns from getifaddrs() in which the iface name is appended, (eg, "fe80::225:90ff:fe7b:ff54%eth0"):

http://gitweb.samba.org/?p=samba.git;a=blob;f=source4/scripting/bin/samba_dnsupdate;h=a5cece1333659b680e731a979153bf6fdb45cfac;hb=HEAD#l98

The result is that IPv6 link-local AAAA records are filtered out on Linux by samba_dnsupdate itself, but are published on FreeBSD.

I'm happy to take a look at this when I next have time, in which case some direction would be appreciated. I can modify the get_interfaces() code to filter IPv6 link-local addresses as seems to have originally been intended, and/or I can modify samba_dnsupdate to check for link-local addresses by looking at the actual address prefix.
Comment 1 Landon Fuller 2013-02-18 03:37:16 UTC
I believe something like this ought to be able to resolve item #2 in samba_dnsupdate:

import array
import sys

def in6_is_addr_linklocal (addr):
    # Extract the first two octets
    idx = addr.find(':')
    if idx == -1:
        return False

    # Check for link-local IPv6. Note that
    # this comprises the first 10 bits (not 16)
    # See also RFC 2373
    octets = array.array('B', addr[:idx].decode("hex"))

    if len(octets) != 2:
        return False

    if octets[0] != 0xfe:
        return False

    if (octets[1] & 0xc0) != 0x80:
        return False;

    return True;
Comment 2 Timur Bakeyev 2013-02-18 05:55:46 UTC
Created attachment 8566 [details]
Simple test of getnameinfo() and flags
Comment 3 Timur Bakeyev 2013-02-18 06:06:39 UTC
If also decided to jump this train and did a bit of testing. Here I attach simple test program that queries the interfaces, get's their names and flags. In the next comments I'll show some resulting output, but the base line is that Landon is correct - we are testing flags of the interfaces, not IP addresses, so we can't rely on the interface flag to assume does it have broadcast address or not. A bit more digging around AF family and typ of the address has to be done.

The second statement isn't correct though - this code SUPPOSED to calculate broadcast address, not filter out Link Local addresses.

There is another difference between Linux and FreeBSD exposed also - the RFC 4007 compliant LL addresses in FreeBSD are not returned automagically by getnameinfo(). From the man page:

     This implementation allows numeric IPv6 address notation with scope iden-
     tifier, as documented in chapter 11 of RFC 4007.  IPv6 link-local address
     will appear as a string like ``fe80::1%ne0''.  Refer to getaddrinfo(3)
     for more information.

And from getaddrinfo():

    This implementation of getaddrinfo() allows numeric IPv6 address notation
     with scope identifier, as documented in chapter 11 of draft-ietf-
     ipv6-scoping-arch-02.txt.  By appending the percent character and scope
     identifier to addresses, one can fill the sin6_scope_id field for
     addresses.  This would make management of scoped addresses easier and
     allows cut-and-paste input of scoped addresses.

That brings the need to fill 'sin6_scope_id' filed of sockaddr_in6 structure(manually). Luckily, it's easy to do and test shows, how.

So, based on that test case this part of the _get_interfaces() has to be reworked. As a result we'll get LL addresses with %iface scope id, that can be filtered by current code.
Comment 4 Timur Bakeyev 2013-02-18 06:09:18 UTC
FreeBSD9 with LL IPv6 only:

em0  address family: 18 (AF_LINK)
em0  address family: 2 (AF_INET)
        address: <10.0.0.236>
        Flags: IFF_UP IFF_BROADCAST IFF_DRV_RUNNING  IFF_SIMPLEX  IFF_MULTICAST
em0  address family: 28 (AF_INET6)
        address: <fe80:1::20c:29ff:fe28:c72b%em0>
        Flags: IFF_UP IFF_BROADCAST IFF_DRV_RUNNING  IFF_SIMPLEX  IFF_MULTICAST
plip0  address family: 18 (AF_LINK)
lo0  address family: 18 (AF_LINK)
lo0  address family: 28 (AF_INET6)
        address: <::1>
        Flags: IFF_UP IFF_LOOPBACK  IFF_DRV_RUNNING  IFF_MULTICAST
lo0  address family: 28 (AF_INET6)
        address: <fe80:3::1%lo0>
        Flags: IFF_UP IFF_LOOPBACK  IFF_DRV_RUNNING  IFF_MULTICAST
lo0  address family: 2 (AF_INET)
        address: <127.0.0.1>
        Flags: IFF_UP IFF_LOOPBACK  IFF_DRV_RUNNING  IFF_MULTICAST


Debina6 Linux with LL IPv6 only:

lo  address family: 17 (AF_PACKET)
eth0  address family: 17 (AF_PACKET)
eth1  address family: 17 (AF_PACKET)
eth2  address family: 17 (AF_PACKET)
eth3  address family: 17 (AF_PACKET)
eth4  address family: 17 (AF_PACKET)
eth5  address family: 17 (AF_PACKET)
eth6  address family: 17 (AF_PACKET)
eth7  address family: 17 (AF_PACKET)
bond0  address family: 17 (AF_PACKET)
bond1  address family: 17 (AF_PACKET)
bond2  address family: 17 (AF_PACKET)
bond3  address family: 17 (AF_PACKET)
lo  address family: 2 (AF_INET)
        address: <127.0.0.1>
        Flags: IFF_UP IFF_LOOPBACK
eth3  address family: 2 (AF_INET)
        address: <10.0.2.254>
        Flags: IFF_UP IFF_BROADCAST IFF_MULTICAST
eth3:0  address family: 2 (AF_INET)
        address: <192.168.2.1>
        Flags: IFF_UP IFF_BROADCAST IFF_MULTICAST
bond0  address family: 2 (AF_INET)
        address: <95.211.181.20>
        Flags: IFF_UP IFF_BROADCAST IFF_MULTICAST
bond1  address family: 2 (AF_INET)
        address: <10.0.0.201>
        Flags: IFF_UP IFF_BROADCAST IFF_MULTICAST
lo  address family: 10 (AF_INET6)
        address: <::1>
        Flags: IFF_UP IFF_LOOPBACK
eth3  address family: 10 (AF_INET6)
        address: <fe80::7ae3:b5ff:fe18:89f2%eth3>
        Flags: IFF_UP IFF_BROADCAST IFF_MULTICAST
bond0  address family: 10 (AF_INET6)
        address: <fe80::7ae3:b5ff:fe18:89e8%bond0>
        Flags: IFF_UP IFF_BROADCAST IFF_MULTICAST
bond1  address family: 10 (AF_INET6)
        address: <fe80::7ae3:b5ff:fe18:89ea%bond1>
        Flags: IFF_UP IFF_BROADCAST IFF_MULTICAST


FreeBSD 6.2(sic!) with real IPv6:

fwe0  address family: 18 (AF_LINK)
fwe0  address family: 28 (AF_INET6)
        address: <fe80:1::1:b7ff:fe01:b9b%fwe0>
        Flags: IFF_UP IFF_BROADCAST IFF_DRV_RUNNING  IFF_PROMISC  IFF_SIMPLEX  IFF_MULTICAST
xl0  address family: 18 (AF_LINK)
xl0  address family: 28 (AF_INET6)
        address: <fe80:2::2e0:81ff:fe21:37a6%xl0>
        Flags: IFF_UP IFF_BROADCAST IFF_DRV_RUNNING  IFF_SIMPLEX  IFF_MULTICAST
xl0  address family: 2 (AF_INET)
        address: <192.168.0.4>
        Flags: IFF_UP IFF_BROADCAST IFF_DRV_RUNNING  IFF_SIMPLEX  IFF_MULTICAST
xl0  address family: 28 (AF_INET6)
        address: <2001:888:1d8f:0:2e0:81ff:fe21:37a6>
        Flags: IFF_UP IFF_BROADCAST IFF_DRV_RUNNING  IFF_SIMPLEX  IFF_MULTICAST
plip0  address family: 18 (AF_LINK)
pflog0  address family: 18 (AF_LINK)
pfsync0  address family: 18 (AF_LINK)
lo0  address family: 18 (AF_LINK)
lo0  address family: 28 (AF_INET6)
        address: <::1>
        Flags: IFF_UP IFF_LOOPBACK  IFF_DRV_RUNNING  IFF_MULTICAST
lo0  address family: 28 (AF_INET6)
        address: <fe80:6::1%lo0>
        Flags: IFF_UP IFF_LOOPBACK  IFF_DRV_RUNNING  IFF_MULTICAST
lo0  address family: 2 (AF_INET)
        address: <127.0.0.1>
        Flags: IFF_UP IFF_LOOPBACK  IFF_DRV_RUNNING  IFF_MULTICAST
lo0  address family: 2 (AF_INET)
        address: <10.10.10.10>
        Flags: IFF_UP IFF_LOOPBACK  IFF_DRV_RUNNING  IFF_MULTICAST
Comment 5 Timur Bakeyev 2013-02-18 07:12:17 UTC
Created attachment 8567 [details]
Patch on interfaces.c

This patch should address the problem with Link-Local addresses on FreeBSD and, hopefully on Linux.
Comment 6 Timur Bakeyev 2013-02-18 07:14:18 UTC
Created attachment 8568 [details]
Patch on interfaces.c

URL link, seems does smth. else...

This patch should address the problem with Link Local addresses on FreeBSD and Linux(?).
Comment 7 Timur Bakeyev 2013-02-18 07:16:35 UTC
A test for <net/if.h> and presence of if_nametoindex() is possibly necessary.
Comment 8 Landon Fuller 2013-02-18 12:50:00 UTC
(In reply to comment #3)
> The second statement isn't correct though - this code SUPPOSED to calculate
> broadcast address, not filter out Link Local addresses.

If you take a look at:

http://gitweb.samba.org/?p=samba.git;a=blob;f=lib/socket/interfaces.c;h=74c642372a24323befae23534d1f525991a96fab;hb=HEAD#l198

The code in question would filter out link-local addresses, except for the fact that the IFF_BROADCAST flag preempts it. The intended behavior of the code seems to be to filter out link-local addresses, but according to git blame, it hasn't done so for some time :)

> So, based on that test case this part of the _get_interfaces() has to be
> reworked. As a result we'll get LL addresses with %iface scope id, that can be
> filtered by current code.

I believe it's also still necessary to modify any filtering code (eg, samba_dnsupdate) to check the address prefix rather than relying on finding an RFC4007 site scope; the use of % scopes appears to be optional, rather than required, when representing link-local addresses:

https://tools.ietf.org/html/rfc4007#section-11.6
Comment 9 Timur Bakeyev 2013-02-18 13:03:29 UTC
(In reply to comment #8)
> (In reply to comment #3)
> > The second statement isn't correct though - this code SUPPOSED to calculate
> > broadcast address, not filter out Link Local addresses.
> 
> If you take a look at:
> 
> http://gitweb.samba.org/?p=samba.git;a=blob;f=lib/socket/interfaces.c;h=74c642372a24323befae23534d1f525991a96fab;hb=HEAD#l198
> 
> The code in question would filter out link-local addresses, except for the fact
> that the IFF_BROADCAST flag preempts it. The intended behavior of the code
> seems to be to filter out link-local addresses, but according to git blame, it
> hasn't done so for some time :)

If you read the whole function, you'll notice that it tries to build the triplet
of ip, netmask and broadcast addresses. In general, it almost works, after rearranging the order of the code it fully does(Well, I have some doubts about 
IN6_IS_ADDR_V4COMPAT there, but that can be discussed).

> > So, based on that test case this part of the _get_interfaces() has to be
> > reworked. As a result we'll get LL addresses with %iface scope id, that can be
> > filtered by current code.
> 
> I believe it's also still necessary to modify any filtering code (eg,
> samba_dnsupdate) to check the address prefix rather than relying on finding an
> RFC4007 site scope; the use of % scopes appears to be optional, rather than
> required, when representing link-local addresses:
> 
> https://tools.ietf.org/html/rfc4007#section-11.6

Well, I agree that relying on the '%' syntax may be not strict enough. Still, this works both on FreeBSD and Linux now.

There is at least one more place, where LL addresses have to be excluded, check 
provision/__init.py__

Did you test my patch by any chance?
Comment 10 Landon Fuller 2013-02-18 13:27:09 UTC
(In reply to comment #9)
> 
> If you read the whole function, you'll notice that it tries to build the
> triplet
> of ip, netmask and broadcast addresses. In general, it almost works, after
> rearranging the order of the code it fully does(Well, I have some doubts about 
> IN6_IS_ADDR_V4COMPAT there, but that can be discussed).

I'm specifically referring to this code:

 205    if (IN6_IS_ADDR_LINKLOCAL(in6) || IN6_IS_ADDR_V4COMPAT(in6)) {
 206        continue;
 207    }

> Did you test my patch by any chance?

Not yet!
Comment 11 Richard Sharpe 2013-02-19 05:15:36 UTC
If we can get agreement that this works for FreeBSD and Linux, I can try to shepherd this into git (following the correct rules, however), unless Andrew takes it up.

I don't currently have any IPv6 setups, and will have to come up to speed, so will be relying on others doing the testing.
Comment 12 Timur Bakeyev 2013-02-19 10:29:28 UTC
(In reply to comment #11)
> If we can get agreement that this works for FreeBSD and Linux, I can try to
> shepherd this into git (following the correct rules, however), unless Andrew
> takes it up.

It should work for both platforms, but as Landon stated above it's not clear what was intended policy for Link-Local and V4inV6 addresses.

Shall they be dropped entirely from the list of IP addresses or they should be filtered later on application level?

Also, in case we have only lo with 127.0.0.1 - what would be the behaviour of this function? It overrides array element with the index 'total', but in such a case there will be nothing to override with.

> I don't currently have any IPv6 setups, and will have to come up to speed, so
> will be relying on others doing the testing.

Well, as soon as you have IPv6 enabled kernel(which is the default) - you already have linked-local addresses, what is the part of the problem - they have no real use, but present in the list of valid addresses.
Comment 13 Landon Fuller 2013-02-27 23:49:54 UTC
The bug has grown long in the tooth, and I think we're at the point where we need guidance from someone higher up the code chain, so here's my attempt at summarizing the issue for review:

Problem:

Samba 4 on FreeBSD publishes link-local IPv6 addresses in DNS. This does not happen on Linux.
Replication breaks when clients attempt to connect via the IPv6 link-local address.

Cause:

There are two issues:

- get_interfaces() is used to fetch the list of local addresses, and it has code to filter out link-local addresses. Due to a logic error, that code is never run.

- samba_dnsupdate has code to filter out link-local IPv6 addresses, but it relies on there being a %-encoded scope identifier in the address. This succeeds on Linux, and fails on FreeBSD, which is why this issue has not appeared on Linux.

Potential fixes:

There are also two, either of which would resolve this issue:

- get_interfaces() can be modified to not return link-local IPv6 addresses. This seems to be what was originally intended, but I'm not sure whether this is the now-desired behavior.

- samba_dnsupdate can be modified to filter based on the link-local IPv6 address prefix, rather than based on the display formatting of the address.

I'm happy to do both/either, depending on direction from someone responsible for the code in question.
Comment 14 Richard Sharpe 2013-02-28 00:03:55 UTC
(In reply to comment #13)
> The bug has grown long in the tooth, and I think we're at the point where we
> need guidance from someone higher up the code chain, so here's my attempt at
> summarizing the issue for review:
> 
> Problem:
> 
> Samba 4 on FreeBSD publishes link-local IPv6 addresses in DNS. This does not
> happen on Linux.
> Replication breaks when clients attempt to connect via the IPv6 link-local
> address.
> 
> Cause:
> 
> There are two issues:
> 
> - get_interfaces() is used to fetch the list of local addresses, and it has
> code to filter out link-local addresses. Due to a logic error, that code is
> never run.
> 
> - samba_dnsupdate has code to filter out link-local IPv6 addresses, but it
> relies on there being a %-encoded scope identifier in the address. This
> succeeds on Linux, and fails on FreeBSD, which is why this issue has not
> appeared on Linux.
> 
> Potential fixes:
> 
> There are also two, either of which would resolve this issue:
> 
> - get_interfaces() can be modified to not return link-local IPv6 addresses.
> This seems to be what was originally intended, but I'm not sure whether this is
> the now-desired behavior.
> 
> - samba_dnsupdate can be modified to filter based on the link-local IPv6
> address prefix, rather than based on the display formatting of the address.
> 
> I'm happy to do both/either, depending on direction from someone responsible
> for the code in question.

Off the top of my head I would prefer that samba_dnsupdate filter based on the link-local address prefix, as that has the highest likelihood of working on other platforms without us having to put in platform specific stuff all the time.
Comment 15 Jeremy Allison 2013-02-28 00:08:16 UTC
get_interfaces() is intended to filter out all link-local addresses. I'd prefer a fix that restores this behaviour.

Jeremy.
Comment 16 Richard Sharpe 2013-02-28 00:21:42 UTC
(In reply to comment #15)
> get_interfaces() is intended to filter out all link-local addresses. I'd prefer
> a fix that restores this behaviour.

In that case, it seems that the filtering done in samba_dnsupdate should be removed so we do not have two places doing the same thing.
Comment 17 Jeremy Allison 2013-02-28 00:28:06 UTC
Created attachment 8593 [details]
git-am fix for mater.

This is Timurs patch as a git-am attachment with attribution and a review. Richard, if you can review then I'll push to master and we'll get this into 4.0.next and 3.6.next.

Jeremy.
Comment 18 Landon Fuller 2013-02-28 00:34:15 UTC
(In reply to comment #15)
> get_interfaces() is intended to filter out all link-local addresses. I'd prefer
> a fix that restores this behaviour.

[snip]

(In reply to comment #17)
> Created attachment 8593 [details]
> git-am fix for mater.
> 
> This is Timurs patch as a git-am attachment with attribution and a review.
> Richard, if you can review then I'll push to master and we'll get this into
> 4.0.next and 3.6.next.

Note that Timur's patch modifies the check to add the sin6_scope_id, rather than filtering the link-local address. This allows the check for '%' in samba_dnsupdate to succeed, as the address will be then be display-formatted with the % scope id.

Wasn't sure if that was the intention.
Comment 19 Jeremy Allison 2013-02-28 00:46:00 UTC
Ah. good point and I missed that. That change to

sin6->sin6_scope_id = if_nametoindex(ifptr->ifa_name); 

is actually irrelevant, as the next line is a 'continue' which means that address won't be returned anyway. I'll fix that up and re-attach the patch.

Jeremy.
Comment 20 Jeremy Allison 2013-02-28 00:48:47 UTC
Created attachment 8594 [details]
Updated git-am fix for master.

Removed unused element in previous patch.
Comment 21 Richard Sharpe 2013-02-28 04:12:08 UTC
Comment on attachment 8594 [details]
Updated git-am fix for master.

This looks good.
Comment 22 Richard Sharpe 2013-02-28 04:13:15 UTC
Now if only we can get rid of the special case filtering done by samba_dnsupdate.
Comment 23 Jeremy Allison 2013-02-28 20:01:43 UTC
Patch applies cleanly to both 3.6.next and 4.0.next. Re-assigning to Karolin for inclusion.

Jeremy.
Comment 24 Karolin Seeger 2013-03-01 15:40:29 UTC
Pushed to v3-6-test and autobuild-v4-0-test.
Comment 25 Landon Fuller 2013-03-01 15:47:15 UTC
I can confirm it resolves (pun intended) replication issues on FreeBSD (although I also had to manually delete the previously created AAAA records).
Comment 26 Landon Fuller 2013-03-02 05:17:46 UTC
Created attachment 8598 [details]
Patch for samba_dnsupdate

An additional patch to remove the now-obsolete link-local check from samba_dnsupdate, as per Richard's comment.
Comment 27 Richard Sharpe 2013-03-02 05:21:28 UTC
(In reply to comment #26)
> Created attachment 8598 [details]
> Patch for samba_dnsupdate
> 
> An additional patch to remove the now-obsolete link-local check from
> samba_dnsupdate, as per Richard's comment.

Hi Landon,

Thank you for this patch, but one problem per bug and this bug is close to being closed.

Can you create a new bug and attach the patch, and I will shepherd it through.
Comment 28 Richard Sharpe 2013-03-02 05:22:39 UTC
Hi again Landon.

Can you include a Signed-Off-By line in that patch as well?
Comment 29 Landon Fuller 2013-03-02 05:32:29 UTC
(In reply to comment #28)
> Hi again Landon.
> 
> Can you include a Signed-Off-By line in that patch as well?

Sure. Follow-up filed as https://bugzilla.samba.org/show_bug.cgi?id=9696
Comment 30 Karolin Seeger 2013-03-04 09:26:44 UTC
Pushed to v4-0-test.
Closing out bug report.

Thanks!