1589990 - STUN: stun_getifaddrs() can return uninitialised bits which are subsequently used

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Julian Seward [:jseward]]

media/mtransport/third_party/nICEr/src/stun/addrs.c:

stun_getifaddrs() is used on all non-WIN32 targets. It extracts from the
kernel an array describing network connections, or addresses, or something
like that (doesn't really matter what). These are written into its
out-parameter nr_local_addr addrs[] and the number of entries is written to
int *count.

There is a path through the main loop in stun_getifaddrs() which can cause a
nr_local_addr record to be returned with its .interface.type field being
uninitialized, but which nevertheless is later used. It also looks to me as
if the .interface.estimated_speed field is not initialised.

The path is simple: first this ioctl fails (returns nonzero):
e = ioctl(s, SIOCETHTOOL, &ifr); and then
e = ioctl(s, SIOCGIWRATE, &wrq); also fails.

The uninitialised .interface.type is subsequently used in
nr_local_addr_fmt_info_string():

    int addr_type = addr->interface.type;
    const char *vpn = (addr_type & NR_INTERFACE_TYPE_VPN) ? "VPN on " : "";

    const char *type = (addr_type & NR_INTERFACE_TYPE_WIRED) ? "wired" :
                       (addr_type & NR_INTERFACE_TYPE_WIFI) ? "wifi" :
                       (addr_type & NR_INTERFACE_TYPE_MOBILE) ? "mobile" :
                       "unknown";

Comment 1

[Julian Seward [:jseward]]

Attachment: complaints from valgrind

Comment 2

[Julian Seward [:jseward]]

Attachment: badness2.c

Standalone test program extracted from addrs.c, that shows the problem.

I suspect it is for a loopback mount, that both ioctls fail. I confirmed that
they both can fail by adding debug printing.

An obvious fix is to zero out the record we might be about to write to, at
the top of the main loop:

  while (if_addr && *count < maxaddrs) {
    memset(&addrs[*count], 0, sizeof(addrs[*count]));   <-----------

I don't know if 'zero' is a safe fill value for those otherwise-uninitialised
fields, but at least doing this would make it fail repeatably (if it would
have failed anyway), rather than non-deterministically.

A quick scan of stun_get_win32_addrs suggests it doesn't suffer from
the same problem.

Comment 3

[Julian Seward [:jseward]]

Removing needinfo. Not sure why I added it. Sorry for the noise.

Comment 4

[Julian Seward [:jseward]]

Attachment: patch for contemplation

Would a fix along these lines be appropriate?

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 9102919 [details] [diff] [review]
patch for contemplation

Review of attachment 9102919 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/mtransport/third_party/nICEr/src/stun/addrs.c
@@ +202,5 @@
>            hex_hashed_ifname))
>            ABORT(r);
>  
>          for (u = tmpAddress->FirstUnicastAddress; u != 0; u = u->Next) {
> +          // FIXME should we do this as a defensive measure?

My inclination would be to do a single memset over the entire addrs array at the beginning of the function.

@@ +271,5 @@
>  
>    if_addr = if_addrs_head;
>  
>    while (if_addr && *count < maxaddrs) {
> +    // Ensure output records are always fully defined.  See bug 1589990.

Same comment here.

Comment 6

[Alastor Wu [:alwu]]

As it's an ongoing task and I assume that might be landed in this cycle, so mark it as P2.

Comment 7

[Julian Seward [:jseward]]

Attachment: Bug 1589990 - STUN: stun_getifaddrs() can return uninitialised bits which are subsequently used. r=bwc.

In media/mtransport/third_party/nICEr/src/stun/addrs.c:

stun_getifaddrs() is used on all non-WIN32 targets. It extracts from the
kernel an array describing network interfaces (I think). These are written
into its out-parameter nr_local_addr addrs[] and the number of entries is
written to int *count.

There is a path through the main loop in stun_getifaddrs() which can cause a
nr_local_addr record to be returned with its .interface.type field being
uninitialized, but which nevertheless is later used. It also looks as if the
.interface.estimated_speed field is not initialised.

This commit zero-initialises the entire output array before writing anything
into it, to avoid such problems, on all targets.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f33d915fcf5
STUN: stun_getifaddrs() can return uninitialised bits which are subsequently used.  r=bwc.

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6f33d915fcf5