Closed Bug 820410 Opened 7 years ago Closed 7 years ago

Audit/Clean up buffer handling in nICEr STUN code


(Core :: WebRTC: Networking, defect)

Not set



Tracking Status
firefox17 --- unaffected
firefox18 - disabled
firefox19 - disabled
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected


(Reporter: abr, Assigned: abr)


(Keywords: sec-audit, Whiteboard: [WebRTC][nICEr][blocking-webrtc+][qa-][adv-main20-])


(1 file, 1 obsolete file)

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.
s/potentially safe/potentially unsafe/
Attached patch Buffer handling safety checks (obsolete) — Splinter Review
Assignee: nobody → adam
Attachment #691144 - Flags: review?(ekr)
Attachment #691144 - Flags: review?(ekr) → review?(rjesup)
Whiteboard: [WebRTC][nICEr]
Whiteboard: [WebRTC][nICEr] → [WebRTC][nICEr][blocking-webrtc+]
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);


::: 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
Attachment #691144 - Flags: review?(rjesup) → review-
Attachment #691144 - Attachment is obsolete: true
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 for the Windows implementation of snprintf used by the nICEr code.
Attachment #691941 - Flags: review?(rjesup)
Attachment #691941 - Flags: review?(rjesup) → review+
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?


> 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?

> 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.
Attachment #691941 - Flags: sec-approval?
Attachment #691941 - Flags: sec-approval? → sec-approval+

Please check this into trunk. Release Management can determine if they really need it for 18 or 19.
Given where we are in the cycle, no need to take fixes of this nature for pref'd off features.
Attachment #691941 - Flags: checkin?(rjesup)
Attachment #691941 - Flags: checkin?(rjesup) → checkin+
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC][nICEr][blocking-webrtc+] → [WebRTC][nICEr][blocking-webrtc+][qa-]
Whiteboard: [WebRTC][nICEr][blocking-webrtc+][qa-] → [WebRTC][nICEr][blocking-webrtc+][qa-][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.