821003 - There are 258 snprintfs in signalling that should be replaced

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 2

[Adam Roach [:abr]]

Attachment: Fix snprintf macro for WIN32

Comment 3

[Adam Roach [:abr]]

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 4

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

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.

Comment 5

[Adam Roach [:abr]]

Doing a quick build check before asking for checkin: https://tbpl.mozilla.org/?tree=Try&rev=50fe6acbb832

Comment 6

[Adam Roach [:abr]]

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.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

What is the specific concern with snprintf?
Are some native snprintf implementations silly?
What should we do with gecko snprintf usage outside signaling code?

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Sorry I missed comment 1 when commenting above. My bad.

Comment 9

[Adam Roach [:abr]]

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

Comment 10

[Adam Roach [:abr]]

Sorry, those return values should have been 5 and 3 respectively -- but you get the idea.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 13

[Adam Roach [:abr]]

Attachment: Replace snprintf macro with static function

Comment 14

[Adam Roach [:abr]]

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.

Comment 15

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

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

Comment 16

[Adam Roach [:abr]]

Attachment: Replace snprintf macro with function

Comment 17

[Adam Roach [:abr]]

Try push (win32 build opt & debug, no tests): https://tbpl.mozilla.org/?tree=Try&rev=837350c4abe6

Comment 18

[Adam Roach [:abr]]

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.

Comment 19

[Adam Roach [:abr]]

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

Comment 20

[Adam Roach [:abr]]

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.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe65eeaa2b4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Jason Smith [:jsmith]]

in-testsuite- - not worthwhile to chase after for an automated test