There are 258 snprintfs in signalling that should be replaced

RESOLVED FIXED in Firefox 20

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jib, Assigned: abr)

Tracking

({sec-audit})

unspecified
mozilla20
x86
Mac OS X
sec-audit
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 disabled, firefox19 disabled, firefox20+ fixed, firefox-esr17 unaffected, b2g18 disabled)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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
(Assignee)

Comment 2

5 years ago
Created attachment 692420 [details] [diff] [review]
Fix snprintf macro for WIN32
(Assignee)

Updated

5 years ago
Assignee: nobody → adam
(Assignee)

Updated

5 years ago
Assignee: adam → nobody
(Assignee)

Comment 3

5 years ago
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+

Updated

5 years ago
Assignee: nobody → adam
Whiteboard: [WebRTC], [blocking-webrtc+]
(Assignee)

Comment 5

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

Comment 6

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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...
(Assignee)

Comment 10

5 years ago
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)
(Assignee)

Comment 13

5 years ago
Created attachment 694113 [details] [diff] [review]
Replace snprintf macro with static function
(Assignee)

Updated

5 years ago
Attachment #692420 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 16

5 years ago
Created attachment 694966 [details] [diff] [review]
Replace snprintf macro with function
(Assignee)

Updated

5 years ago
Attachment #694113 - Attachment is obsolete: true
(Assignee)

Comment 17

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

Comment 18

5 years ago
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)
(Assignee)

Comment 19

5 years ago
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...
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Updated

5 years ago
Attachment #694966 - Flags: review?(rjesup) → review+
(Assignee)

Comment 20

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #694966 - Attachment description: Replace snprintf macro with static function → Replace snprintf macro with function
Attachment #694966 - Flags: sec-approval? → sec-approval+
status-firefox18: --- → disabled
status-firefox19: --- → disabled
status-firefox20: --- → affected
tracking-firefox20: --- → +
(Assignee)

Updated

5 years ago
Attachment #694966 - Flags: checkin?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe65eeaa2b4
Target Milestone: --- → mozilla20

Updated

5 years ago
Attachment #694966 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/6fe65eeaa2b4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox20: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+][qa-]
in-testsuite- - not worthwhile to chase after for an automated test
Flags: in-testsuite? → in-testsuite-
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected
status-b2g18: unaffected → disabled
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.