820410 - Audit/Clean up buffer handling in nICEr STUN code

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Adam Roach [:abr]]

The nICEr STUN implementation has been identified as having potentially safe buffer handling. It should, in general, be audited for common errors such as potential buffer overflows and string null-termination hygiene.

Comment 1

[Adam Roach [:abr]]

s/potentially safe/potentially unsafe/

Comment 2

[Adam Roach [:abr]]

Attachment: Buffer handling safety checks

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 691144 [details] [diff] [review]
Buffer handling safety checks

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

r- for the snprintf() thing and the strlcpy args

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +221,5 @@
>          goto next;
>        if(cand->stun_server != foundation->stun_server)
>          goto next;
>  
> +      snprintf(fnd,sizeof(fnd),"%d",i);

snprintf doesn't guarantee null-termination.  Need to use an alternative that does (see signaling and other places, PR_snprintf, etc), or insert a null at the end of the buffer as a backstop, or be certain the string can't be as large as the buffer (which makes strnxxx irrelevant basically), but may need an assertion or kickout.

@@ +238,5 @@
>      foundation->type=cand->type;
>      foundation->stun_server=cand->stun_server;
>      STAILQ_INSERT_TAIL(&ctx->foundations,foundation,entry);
>  
> +    snprintf(fnd,sizeof(fnd),"%d",i);

ditto

::: media/mtransport/third_party/nICEr/src/stun/addrs.c
@@ +151,5 @@
>      struct rt_addrinfo info;
>      struct sockaddr_in *sin;
>  
>      ifr.ifr_addr.sa_family = AF_INET;
> +    strlcpy(ifr.ifr_name, name, sizeof ifr.ifr_name);

()'s on sizeof

@@ +300,5 @@
>  #ifdef _UNICODE
>      mbstowcs_s(&converted_chars, adapter_GUID_tchar, strlen(adapter_GUID)+1,
>                 adapter_GUID, _TRUNCATE);
>  #else
> +    strlcpy(adapter_GUID_tchar, _NR_MAX_NAME_LENGTH, adapter_GUID);

This was wrong before, and still is - length comes last

::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c
@@ +391,5 @@
>      }
>  
>      STAILQ_FOREACH(clnt, &ctx->clients, entry) {
> +        if (!strncmp(clnt->username, attr->u.username,
> +                     sizeof(attr->u.username)))

I assume u.username is null-terminated, so it will kick out there even if clnt->username is much longer, so this isn't *strictly) needed, but it's not a problem.

::: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
@@ +115,5 @@
>              ++(ctx->phase);
> +            if (ctx->phase > NUMBER_OF_STUN_CTX) {
> +                ABORT(R_INTERNAL);
> +            }
> +            

trailing spaces

Comment 4

[Adam Roach [:abr]]

Attachment: Buffer handling safety checks

Comment 5

[Adam Roach [:abr]]

New patch fixes parameter ordering issue for windows, cosmetic flaws.

Summary of IRC conversation with jesup: Windows does not provide an snprintf function, and the similarly-named Windows function _snprintf does not guarantee null termination. Within the nICEr (ice) code, snprintf for Windows is defined in terms of Microsoft's vsnprintf_s function, which *does* provide null termination. See http://mxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/util.c#555 for the Windows implementation of snprintf used by the nICEr code.

Comment 6

[Adam Roach [:abr]]

Comment on attachment 691941 [details] [diff] [review]
Buffer handling safety checks

[Security approval request comment]
> How easily can the security issue be deduced from the patch?

Figuring out how to overflow the particular buffers in question would be a very difficult task. I'm not certain any of these are actually exploitable; for the most part, this patch replaces potentially unsafe constructs with safe variants.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

> Which older supported branches are affected by this flaw?

FF 18 and 19 contain this code. However, this code is only executed as part of WebRTC handling. WebRTC is preffed off by default in these builds, so exposure in 18 and 19 is very limited.

> If not all supported branches, which bug introduced the flaw?

https://bugzilla.mozilla.org/show_bug.cgi?id=790517

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The source file in question does not appear to have been changed since its introduction. The patch should apply cleanly to all branches.

> How likely is this patch to cause regressions; how much testing does it need?

The chance of these changes causing regressions is quite low: the changes consist primarily of replacing potentially unsafe functions with equivalent, but safer, functions.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

sec-approval+.

Please check this into trunk. Release Management can determine if they really need it for 18 or 19.

Comment 8

[Alex Keybl [:akeybl]]

Given where we are in the cycle, no need to take fixes of this nature for pref'd off features.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7963fe83e7a2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7963fe83e7a2

Comment 11

[Adam Roach [:abr]]

Landed on resiprocate trunk r9911: http://svn.resiprocate.org/viewsvn/resiprocate?diff_format=l&view=revision&revision=9911