core: make sure there is rdata to process before parsing it

[msekletar]

Fixes #452

CVE-2023-38472

[evverx]

Thanks for the PR!

Could you add a test to https://github.com/lathiat/avahi/blob/master/avahi-client/client-test.c?

[evverx]

I'm not sure why it should reject that. parse_rdata can handle zero-length rdata just fine. It even special-cases it in some places:

        case AVAHI_DNS_TYPE_TXT:
    
            if (rdlength > 0) {
...
            } else
                r->data.txt.string_list = NULL;

Looks like avahi_rdata_parse is broken somewhere.

[msekletar]

Hmm, empty rdata for TXT records is not valid so I think it is fine to refuse them on D-Bus API side.

https://datatracker.ietf.org/doc/html/rfc1035#section-3.3.14

[evverx]

That API is supposed to handle all sorts of resource records. My point was that there is nothing inherently wrong with RRs like that and some parts of avahi are equipped to deal with that.

Given that nobody has ever relied on that because it always crashed I guess it should be fine to start rejecting that instead of crashing and explore the other options later.

(Also looking at (!rdata || size <= 0) I think size == 0 should be enough. I'm not sure why the other conditions are necessary)

[evverx]

Maybe those if conditions could be merged

-        if (avahi_rdata_parse (r, rdata, size) < 0) {
+        if (!rdata || avahi_rdata_parse (r, rdata, size) < 0) {
             avahi_record_unref (r);
             return avahi_dbus_respond_error(c, m, AVAHI_ERR_INVALID_RDATA, NULL);
         }

Either way I think either size == 0 or !rdata should be enough. If rdata isn't NULL and size is 0 something is broken somewhere.

[evverx]

some parts of avahi are equipped to deal with that

I'm going to take that back.

malloc.c:250: avahi_memdup: Assertion `s' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff72b0884 in __pthread_kill_implementation () from /lib64/libc.so.6
#0  0x00007ffff72b0884 in __pthread_kill_implementation ()
   from /lib64/libc.so.6
#1  0x00007ffff725fafe in raise () from /lib64/libc.so.6
#2  0x00007ffff724887f in abort () from /lib64/libc.so.6
#3  0x00007ffff724879b in __assert_fail_base.cold () from /lib64/libc.so.6
#4  0x00007ffff7258187 in __assert_fail () from /lib64/libc.so.6
#5  0x00007ffff7f76315 in avahi_memdup (s=<optimized out>, l=l@entry=0)
    at malloc.c:250
#6  0x00007ffff756bb6d in avahi_record_copy (r=0x604000007b50) at rr.c:469
#7  0x00007ffff75480c6 in make_goodbye_record (r=<optimized out>)
    at announce.c:366
#8  send_goodbye_callback (m=<optimized out>, i=0x60d000000380, 
    userdata=0x608000003e20) at announce.c:416
#9  0x00007ffff7508e10 in avahi_interface_monitor_walk (m=0x606000000b60, 
    interface=interface@entry=-1, protocol=protocol@entry=-1, 
    callback=callback@entry=0x7ffff7547a75 <send_goodbye_callback>, 
    userdata=userdata@entry=0x608000003e20) at iface.c:761
#10 0x00007ffff7548f25 in avahi_goodbye_entry (s=0x616000001580, 
    e=e@entry=0x608000003e20, send_goodbye=send_goodbye@entry=1, 
    remove=remove@entry=1) at announce.c:524
#11 0x00007ffff7524379 in avahi_s_entry_group_free (g=g@entry=0x60b000000510)
    at entry.c:1096
#12 0x000000000043b799 in avahi_dbus_entry_group_free (i=i@entry=0x60600002f240) at dbus-entry-group.c:40
#13 0x0000000000427abb in client_free (c=c@entry=0x60c000000ac0) at dbus-protocol.c:73
#14 0x000000000042a6f9 in msg_signal_filter_impl (c=<optimized out>, m=0x610000000340, userdata=<optimized out>) at dbus-protocol.c:233
#15 0x00007ffff7ed8f79 in dbus_connection_dispatch () from /lib64/libdbus-1.so.3
#16 0x0000000000446c82 in dispatch_timeout_callback (t=<optimized out>, userdata=0x6030000006a0) at ../avahi-common/dbus-watch-glue.c:105
#17 0x00007ffff7f80aac in timeout_callback (t=t@entry=0x606000000680) at simple-watch.c:447
#18 0x00007ffff7f85377 in avahi_simple_poll_dispatch (s=s@entry=0x60e000000040) at simple-watch.c:563
#19 0x00007ffff7f85960 in avahi_simple_poll_iterate (s=0x60e000000040, timeout=timeout@entry=-1) at simple-watch.c:605
#20 0x0000000000410254 in run_server (c=0x4d8b20 <config>) at main.c:1268
#21 main (argc=<optimized out>, argv=<optimized out>) at main.c:1693

so let's reject that to fix the issue at hand and then teach the part responsible for sending packets to copy records with zero-length rdata (and some other things) if necessary.

[msekletar]

@evverx I've merged the two if conditions. PTAL.

[evverx]

LGTM. Thanks!

The coverage went up in all the right places to judge from https://coveralls.io/builds/63438983/source?filename=avahi-daemon%2Fdbus-entry-group.c#L343.

[evverx]

malloc.c:250: avahi_memdup: Assertion `s' failed.

It should be addressed in #492

[evverx]

Looks like avahi_rdata_parse is broken somewhere

It is actually broken. size_t size is silently downgraded to uint16_t when it's passed to parse_rdata there:
https://github.com/lathiat/avahi/blob/b024ae5749f4aeba03478e6391687c3c9c8dee40/avahi-core/dns.c#L856-L868
It should probably be fixed too. It should fail if size > AVAHI_DNS_RDATA_MAX.