Closed
Bug 821003
Opened 12 years ago
Closed 12 years ago
There are 258 snprintfs in signalling that should be replaced
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: jib, Assigned: abr)
Details
(Keywords: sec-audit, Whiteboard: [WebRTC], [blocking-webrtc+][qa-][adv-main20-])
Attachments
(1 file, 2 obsolete files)
1.42 KB,
patch
|
jesup
:
review+
abillings
:
sec-approval+
jesup
:
checkin+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → adam
Assignee | ||
Updated•12 years ago
|
Assignee: adam → nobody
Assignee | ||
Comment 3•12 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 4•12 years ago
|
||
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•12 years ago
|
Assignee: nobody → adam
Whiteboard: [WebRTC], [blocking-webrtc+]
Assignee | ||
Comment 5•12 years ago
|
||
Doing a quick build check before asking for checkin: https://tbpl.mozilla.org/?tree=Try&rev=50fe6acbb832
Assignee | ||
Comment 6•12 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.
Comment 7•12 years ago
|
||
What is the specific concern with snprintf? Are some native snprintf implementations silly? What should we do with gecko snprintf usage outside signaling code?
Assignee | ||
Comment 9•12 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•12 years ago
|
||
Sorry, those return values should have been 5 and 3 respectively -- but you get the idea.
Reporter | ||
Comment 11•12 years ago
|
||
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.
Reporter | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #692420 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 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•12 years ago
|
Attachment #694113 -
Flags: review?(rjesup)
Comment 15•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #694113 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
Try push (win32 build opt & debug, no tests): https://tbpl.mozilla.org/?tree=Try&rev=837350c4abe6
Assignee | ||
Comment 18•12 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•12 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•12 years ago
|
Status: NEW → ASSIGNED
Updated•12 years ago
|
Attachment #694966 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 20•12 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•12 years ago
|
Attachment #694966 -
Attachment description: Replace snprintf macro with static function → Replace snprintf macro with function
Updated•12 years ago
|
Attachment #694966 -
Flags: sec-approval? → sec-approval+
Updated•12 years ago
|
status-firefox18:
--- → disabled
status-firefox19:
--- → disabled
status-firefox20:
--- → affected
tracking-firefox20:
--- → +
Assignee | ||
Updated•12 years ago
|
Attachment #694966 -
Flags: checkin?(rjesup)
Comment 21•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe65eeaa2b4
Target Milestone: --- → mozilla20
Updated•12 years ago
|
Attachment #694966 -
Flags: checkin?(rjesup) → checkin+
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6fe65eeaa2b4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+][qa-]
Comment 23•12 years ago
|
||
in-testsuite- - not worthwhile to chase after for an automated test
Flags: in-testsuite? → in-testsuite-
Updated•11 years ago
|
status-b2g18:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Updated•11 years ago
|
Updated•11 years ago
|
Whiteboard: [WebRTC], [blocking-webrtc+][qa-] → [WebRTC], [blocking-webrtc+][qa-][adv-main20-]
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•