Closed Bug 821003 Opened 7 years ago Closed 7 years ago

There are 258 snprintfs in signalling that should be replaced

Categories

(Core :: WebRTC: Signaling, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 --- disabled
firefox19 --- disabled
firefox20 + fixed
firefox-esr17 --- unaffected
b2g18 --- disabled

People

(Reporter: jib, Assigned: abr)

Details

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

Attachments

(1 file, 2 obsolete files)

There are 258 uses of snprintf in the signalling code:
http://mxr.mozilla.org/mozilla-central/search?find=%2Fmedia%2Fwebrtc%2Fsignaling%2Fsrc%2F&string=snprintf

These could be find/replaced with PR_snprintf or maybe csf_sprintf from common/csf_common.h
Ones that are in Mac and Linux code are safe, but those in WIN32 specific code are not safe, as snprintf is pound defined as _snprintf. See http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_stdio.h#12
Attached patch Fix snprintf macro for WIN32 (obsolete) — Splinter Review
Assignee: nobody → adam
Assignee: adam → nobody
I've attached an untested patch that would provide a quick & easy replacement for that macro. Feel free to use it as the basis for a fix, or to ignore it.
Comment on attachment 692420 [details] [diff] [review]
Fix snprintf macro for WIN32

_snprintf_s() is safe, but has different semantics (if it truncates), so we'll do it this way for now.
Attachment #692420 - Flags: review+
Assignee: nobody → adam
Whiteboard: [WebRTC], [blocking-webrtc+]
Doing a quick build check before asking for checkin: https://tbpl.mozilla.org/?tree=Try&rev=50fe6acbb832
Okay, this patch doesn't work -- it doesn't provide a return value, which hoses the compile in several places. Will respin the patch to use PR_snprintf in a bit.
What is the specific concern with snprintf?
Are some native snprintf implementations silly?
What should we do with gecko snprintf usage outside signaling code?
Keywords: sec-audit
Sorry I missed comment 1 when commenting above. My bad.
Argh. A simple replacement from snprintf to PR_snprintf does not work because of subtle difference in return code semantics.

snprintf is defined: "return the number of characters that would have been printed if the n were unlimited... not including the final '\0'"

PR_snprintf is defined: "Returns the length of the written output not including the NULL terminator."

In other words:

snprintf(buffer, 4, "%s", "world") ==> 5
PR_snprintf(buffer, 4, "%s", "world") ==> 4

This difference causes the code to misbehave. With 250+ places to hand-check, this would be a very arduous task. So we're back to something more like the original patch...
Sorry, those return values should have been 5 and 3 respectively -- but you get the idea.
Note that you'll get macro side-effects. Even if we protect the args:

  #define snprintf(str, size, format, ...) do { \
    _snprintf((str), (size), (format), ##__VA_ARGS__); \
    if ((size)>0) str[(size)-1]=0; \
  } while (0)

it would still do the wrong thing for sprintf(*dstarray++, sizeof(*dstarray), bar); if bar is bigger than the buffer.

I tried making it an inline function but had trouble getting it to compile and didn't come back to it. I can upload my attempt as a patch if you want.
Missed one arg:
  #define snprintf(str, size, format, ...) do { \
    _snprintf((str), (size), (format), ##__VA_ARGS__); \
    if ((size)>0) (str)[(size)-1]=0; \
  } while (0)
Attachment #692420 - Attachment is obsolete: true
I've concluded that there's no amount of macro magic that will make this work smoothly. The new patch uses a static function instead, and has been copied in from nrappkit.
Attachment #694113 - Flags: review?(rjesup)
Comment on attachment 694113 [details] [diff] [review]
Replace snprintf macro with static function

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

r-

r+ with either inline or a #define to a single impl, not hundreds of local impls.

Please push a Try before commiting

::: media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_stdio.h
@@ +17,5 @@
> +  ret = _vscprintf(format, argp);
> +  vsnprintf_s(buffer, n, _TRUNCATE, format, argp);
> +  va_end(argp);
> +  return ret;
> +}

This code will get inserted into a zillion files that include cpr_win_stdio.h.  This isn't as horrible as it sounds, as effectively this is just two function calls.  However, I'm concerned that some compilers might complain if a file includes this but doesn't call snprintf ("unused static function: snprintf").  Using inline would help with that (at a small cost if it's used multiple times in the file - and the compiler might have auto-inlined it anyways.)

The alternative is to define it once in a c file as cpr_snprintf(), and then have cpr_win_stdio.h #define snprintf cpr_snprintf
Attachment #694113 - Flags: review?(rjesup) → review-
Attachment #694113 - Attachment is obsolete: true
Try push (win32 build opt & debug, no tests): https://tbpl.mozilla.org/?tree=Try&rev=837350c4abe6
Comment on attachment 694966 [details] [diff] [review]
Replace snprintf macro with function

Try build is still running, but I don't see why we can't get a review done in the meanwhile. I've taken the suggestion to move this off to a single function with a macro redirect from snprintf to the new function.
Attachment #694966 - Flags: review?(rjesup)
Comment on attachment 694966 [details] [diff] [review]
Replace snprintf macro with function

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

Try build is still running, but I figure we can get review in parallel...
Status: NEW → ASSIGNED
Attachment #694966 - Flags: review?(rjesup) → review+
Comment on attachment 694966 [details] [diff] [review]
Replace snprintf macro with function

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

It would require a very careful analysis of each snprintf to determine whether it is possible to reach the extent of the buffer. Even so, the worst that could be achieved would be a string that is not properly null-terminated. There is no way that attacker-crafted bytes could be written beyond the extent of a buffer so as to cause an overflow.

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

Firefox 18 and 19, both of which have this code preffed off by default.

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

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

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

The patch should apply to firefox 18 and 19 cleanly.

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

The chance for regressions is relatively low. This patch is a one-for-one replacement of snprintf with a function that implements a safe variation on Windows.
Attachment #694966 - Flags: sec-approval?
Attachment #694966 - Attachment description: Replace snprintf macro with static function → Replace snprintf macro with function
Attachment #694966 - Flags: sec-approval? → sec-approval+
Attachment #694966 - Flags: checkin?(rjesup)
Attachment #694966 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/6fe65eeaa2b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+][qa-]
in-testsuite- - not worthwhile to chase after for an automated test
Flags: in-testsuite? → in-testsuite-
Whiteboard: [WebRTC], [blocking-webrtc+][qa-] → [WebRTC], [blocking-webrtc+][qa-][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.