fix use-after-free in local printing if dumping after flushing interface.

[tinselcity]

What

• fix use-after-free in local socket printing if flushing interface by flushing associated neighbours in flush_interface function.

Steps to reproduce

start babeld (can run with asan enabled -see diff)

diff --git a/Makefile b/Makefile
index 16d8591..2eb1d0e 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 PREFIX = /usr/local
 MANDIR = $(PREFIX)/share/man
 
-CDEBUGFLAGS = -Os -g -Wall
+CDEBUGFLAGS = -O0 -ggdb -fsanitize=address -Wall
 
 DEFINES = $(PLATFORM_DEFINES)

running

>sudo ./babeld -C 'skip-kernel-setup true' -C 'interface noop hello-interval 500 type wired' -C 'local-port-readwrite 5832' -C 'debug 0' -C 'redistribute proto 250 allow' -C 'redistribute local deny' -C 'export-table 72' -C 'import-table 72'

add interface

>echo 'interface br-91aa1c89f2e5 hello-interval 4 type wireless' | nc -q1 ::1 5832
BABEL 1.0
version unknown
host rmorrison-5570-ubuntu
my-id 66:49:7d:ff:fe:91:d9:f3
ok

wait for neighbor babeld to peer

babeld monitor output

change interface br-91aa1c89f2e5 up true ipv6 fe80::42:13ff:fe8f:38c4 ipv4 172.21.0.1
add neighbour 513000000040 address fe80::42:acff:fe15:2 if br-91aa1c89f2e5 reach 0000 ureach 0000 rxcost 65535 txcost 65535 cost 65535
change neighbour 513000000040 address fe80::42:acff:fe15:2 if br-91aa1c89f2e5 reach 8000 ureach 0000 rxcost 1023 txcost 65535 cost 65535
change neighbour 513000000040 address fe80::42:acff:fe15:2 if br-91aa1c89f2e5 reach 8000 ureach 0000 rxcost 1023 txcost 1023 cost 4088
add route 507000000090 prefix 172.29.0.3/32 from 0.0.0.0/0 installed no id 38:1e:20:3a:fb:ac:be:0f metric 4344 refmetric 256 via fe80::42:acff:fe15:2 if br-91aa1c89f2e5
change route 507000000090 prefix 172.29.0.3/32 from 0.0.0.0/0 installed yes id 38:1e:20:3a:fb:ac:be:0f metric 4344 refmetric 256 via fe80::42:acff:fe15:2 if br-91aa1c89f2e5
add route 507000000100 prefix 172.29.0.1/32 from 0.0.0.0/0 installed no id e0:96:72:0b:3b:cd:53:44 metric 4088 refmetric 0 via fe80::42:acff:fe15:2 if br-91aa1c89f2e5
...

flush interface and dump state

>echo -e 'flush interface br-91aa1c89f2e5\ndump\n' | nc -q 1 -v ::1 5832
Connection to ::1 5832 port [tcp/*] succeeded!
BABEL 1.0
version unknown
host rmorrison-5570-ubuntu
my-id 66:49:7d:ff:fe:91:d9:f3
ok
ok
add interface noop up false

Got SIGABRT (asan backtrace)

babeld>sudo ./babeld -C 'skip-kernel-setup true' -C 'interface noop hello-interval 500 type wired' -C 'local-port-readwrite 5832' -C 'debug 0' -C 'redistribute proto 250 allow' -C 'redistribute local deny' -C 'export-table 72' -C 'import-table 72'
=================================================================
==476376==ERROR: AddressSanitizer: heap-use-after-free on address 0x5120000001d4 at pc 0x5fd8b99b492e bp 0x7fff1dc48840 sp 0x7fff1dc48830
READ of size 2 at 0x5120000001d4 thread T0
    #0 0x5fd8b99b492d in if_up /home/rmorrison/sources/babeld/interface.h:172
    #1 0x5fd8b99b6866 in neighbour_cost /home/rmorrison/sources/babeld/neighbour.c:342
    #2 0x5fd8b99d861a in local_notify_neighbour_1 /home/rmorrison/sources/babeld/local.c:156
    #3 0x5fd8b99d94f6 in local_notify_all_1 /home/rmorrison/sources/babeld/local.c:291
    #4 0x5fd8b99d9bd4 in local_read /home/rmorrison/sources/babeld/local.c:356
    #5 0x5fd8b99a53c9 in babel_main /home/rmorrison/sources/babeld/babeld.c:685
    #6 0x5fd8b99a35f3 in main /home/rmorrison/sources/babeld/babeld.c:385
    #7 0x720f4e22a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #8 0x720f4e22a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #9 0x5fd8b99a1da4 in _start (/home/rmorrison/sources/babeld/babeld+0xbda4) (BuildId: 79879a8aa0a0b934f8f0e3844d524dd73c461931)

0x5120000001d4 is located 20 bytes inside of 304-byte region [0x5120000001c0,0x5120000002f0)
freed by thread T0 here:
    #0 0x720f4e6fc4d8 in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0x5fd8b99affbd in flush_interface /home/rmorrison/sources/babeld/interface.c:133
    #2 0x5fd8b99d5f82 in parse_config_line /home/rmorrison/sources/babeld/configuration.c:1230
    #3 0x5fd8b99d6bd9 in parse_config_from_string /home/rmorrison/sources/babeld/configuration.c:1346
    #4 0x5fd8b99d9b06 in local_read /home/rmorrison/sources/babeld/local.c:347
    #5 0x5fd8b99a53c9 in babel_main /home/rmorrison/sources/babeld/babeld.c:685
    #6 0x5fd8b99a35f3 in main /home/rmorrison/sources/babeld/babeld.c:385
    #7 0x720f4e22a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #8 0x720f4e22a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #9 0x5fd8b99a1da4 in _start (/home/rmorrison/sources/babeld/babeld+0xbda4) (BuildId: 79879a8aa0a0b934f8f0e3844d524dd73c461931)

previously allocated by thread T0 here:
    #0 0x720f4e6fd340 in calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
    #1 0x5fd8b99afc37 in add_interface /home/rmorrison/sources/babeld/interface.c:84
    #2 0x5fd8b99d34fc in add_ifconf /home/rmorrison/sources/babeld/configuration.c:943
    #3 0x5fd8b99d5bb3 in parse_config_line /home/rmorrison/sources/babeld/configuration.c:1199
    #4 0x5fd8b99d6bd9 in parse_config_from_string /home/rmorrison/sources/babeld/configuration.c:1346
    #5 0x5fd8b99d9b06 in local_read /home/rmorrison/sources/babeld/local.c:347
    #6 0x5fd8b99a53c9 in babel_main /home/rmorrison/sources/babeld/babeld.c:685
    #7 0x5fd8b99a35f3 in main /home/rmorrison/sources/babeld/babeld.c:385
    #8 0x720f4e22a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #9 0x720f4e22a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #10 0x5fd8b99a1da4 in _start (/home/rmorrison/sources/babeld/babeld+0xbda4) (BuildId: 79879a8aa0a0b934f8f0e3844d524dd73c461931)

SUMMARY: AddressSanitizer: heap-use-after-free /home/rmorrison/sources/babeld/interface.h:172 in if_up
Shadow bytes around the buggy address:
  0x511fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x511fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x512000000000: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x512000000080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x512000000100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa fa
=>0x512000000180: fa fa fa fa fa fa fa fa fd fd[fd]fd fd fd fd fd
  0x512000000200: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x512000000280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
  0x512000000300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x512000000380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x512000000400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==476376==ABORTING

Patch prevents use-after-free by:

• flushing any neighbours associated with flushed (free'd) interface.

[jech]

Good catch, thanks.

I don't think we should be setting neigh->ifp to NULL, though, we should be flushing all neighbours that appear over that interface, i.e. we should be calling flush_neighbour on all such neighbours. See the functions flush_neighbour and flush_neighbour_routes, we already do that when flushing a neighbour.

I also don't think it should happen in if_updown, it should happen in flush_interface.

[tinselcity]

Good catch, thanks.

I don't think we should be setting neigh->ifp to NULL, though, we should be flushing all neighbours that appear over that interface, i.e. we should be calling flush_neighbour on all such neighbours. See the functions flush_neighbour and flush_neighbour_routes, we already do that when flushing a neighbour.

I also don't think it should happen in if_updown, it should happen in flush_interface.

Thanks for the suggestions! I've updated the flush_interface function to flush associated neighbours. Let me know if that looks better.

By the way, are you interested in a PR to add support for IPv4 multicast (in the protocol_group configuration)? There was an older issue where someone was asking if babeld had intended to support:

• https://github.com/jech/babeld/issues/78

The PR is a bit chunky though since it required changes in many places to use IPv4/IPv6 agnostic storage (eg sockaddr_storage)

Can folks let me know if they're interested and I'll put up the PR for review?

Here's a brief overview of the work. """

• adds global protocol_group_type field alongside existing protocol_group address:

  • AF_INET for IPv4 or AF_INET6 for IPv6 -defaulting to AF_INET6 (IPv6)
  • type is detected from the user configurable protocol_group config (multicast address to use)
  • create separate babel_socket_ipv4 to create an IPv4 listening socket (instead of IPv6 -default from original babel_socket).

• interface changes

  • interface buffer object stores interface IP as an IP agnostic sockadd_storage object instead of a sockaddr_in6

    • this change resulted in the most code churn since necessary to add checks for IP address type where used.

  • If IPv4 multicast, each interface creates it's own socket for sending UDP messages.

    • Necessary since w/ IPv4 can't specify an interface to use to send like IPv6 with the sin6_scope_id field (interface ID). With IPv6 and this field can reuse the same socket for sending out of multiple interfaces.

"""

[jech]

Looks good. Could you please squash your commits?

[tinselcity]

Done -Rebased into a single commit.

Any interest in the IPv4 multicast support?

• protocol_group supporting an IPv4 address like 224.0.0.111

[jech]

Any interest in the IPv4 multicast support?

• protocol_group supporting an IPv4 address like 224.0.0.111

No, we're not going to implement that.

Babel was originally defined to run over either IPv6 or IPv4. It was later decided that in order to avoid fragmenting the community, it is better to run Babel over IPv6 only. All the more so since running over IPv6 has a number of advantages, such as stable neighbour identifiers (IPv6 link-locals are more stable than IPv4 addresses), and v4-via-v6.

[tinselcity]

Any interest in the IPv4 multicast support?

• protocol_group supporting an IPv4 address like 224.0.0.111

No, we're not going to implement that.

Babel was originally defined to run over either IPv6 or IPv4. It was later decided that in order to avoid fragmenting the community, it is better to run Babel over IPv6 only. All the more so since running over IPv6 has a number of advantages, such as stable neighbour identifiers (IPv6 link-locals are more stable than IPv4 addresses), and v4-via-v6.

Got it thanks! Appreciate the review!