Closed
Bug 1368870
Opened 7 years ago
Closed 7 years ago
the changes made by the bugs tracked by bug 1197205 may have introduced a number of buffer overflows
Categories
(Core :: Security, defect, P1)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: keeler, Assigned: keeler)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [adv-main57-][post-critsmash-triage])
See bug 1368652. Bug 1197205 tracked a number of bugs around replacing PR_snprintf (and similar) calls, often with snprintf. Unfortunately, PR_snprintf and snprintf have different semantics. PR_snprintf will return the number of bytes of the given format string it wrote (which may be less than the total length the format string turned out to be). snprintf will return the number of bytes the format string turned out to be, which may be more than the total number of bytes it actually wrote. For example: example.c: #include <stdio.h> #include <prprf.h> int main(int argc, char* argv[]) { char buf[10]; int written = snprintf(buf, sizeof(buf), "01234567890123456789"); printf("snprintf returned %d for a buffer of size %lu\n", written, sizeof(buf)); written = PR_snprintf(buf, sizeof(buf), "01234567890123456789"); printf("PR_snprintf returned %d for a buffer of size %lu\n", written, sizeof(buf)); return 0; } clang -I /usr/include/nspr4 -lnspr4 -o example example.c ./example > snprintf returned 20 for a buffer of size 10 > PR_snprintf returned 9 for a buffer of size 10 Any code that repeatedly called PR_snprintf to append to a buffer that was updated to use snprintf and relies on the return value is potentially vulnerable. Looking at the bugs tracked by that meta bug, some of these instances may be in content, which would make them sec-critical. This is just a meta bug to track the work to fix this. My intention is to open bugs for each bug tracked by the meta bug so we can audit the changes made in each one.
Updated•7 years ago
|
status-firefox53:
--- → wontfix
status-firefox54:
--- → affected
status-firefox55:
--- → affected
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → affected
Comment 1•7 years ago
|
||
Doh, totally my fault. Can you CC me to the other bug?
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 2•7 years ago
|
||
Honestly, I'm inclined to blame PR_snprintf and/or snprintf here :) Looks like RyanVM cc'd you to bug 1368652.
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 3•7 years ago
|
||
I went through the bugs tracked by bug 1197205 and here's what I found: bug 1197306 - no return values from snprintf used => not vulnerable bug 1197307 - '' bug 1197309 - intl/unicharutil/nsSaveAsCharset.cpp: nsSaveAsCharset::DoConversionFallBack used the return value, but this was removed in 45 by bug 1214619 => current versions not vulnerable bug 1197311: - dom/plugins/base/nsPluginHost.cpp: nsPluginHost::ParsePostBufferToFixHeaders uses the return value => check needs to be updated from '==' to '>=' (it's probably not vulnerable in practice, since it doesn't look like the format could ever actually be larger than the buffer allocated, but still) - dom/system/gonk/NetworkUtils.cpp: NetworkUtils::removeDefaultRoute has a call to snprintf that is properly checked, but it also has calls to SprintfLiteral that aren't, although, again, this may not be able to overflow in practice. Further, this was removed in 55 by bug 1357323 (also, this is gonk, so I'm not sure when we last had a product that actually used this). bug 1197313 - netwerk/protocol/http/nsHttpHandler.cpp: PrepareAcceptLanguages uses snprintf without properly checking it => needs fixing bug 1197314 - being addressed in bug 1368652 bug 1197315 - gfx/layers/composite/FPSCounter.cpp: - FPSCounter::WriteFrameTimeStamps uses SprintfLiteral without properly checking it, although in practice it shouldn't overflow => needs fixing (unless this is performance-critical?) - FPSCounter::PrintHistogram uses snprintf with an assertion and I have no idea how it's not failing in the general case, unless I'm misreading it. In any case, this should be fixed and/or clarified. bug 1197316 - xpcom/threads/ThreadStackHelper.cpp: - ThreadStackHelper::AppendJSEntry uses SprintfLiteral, but it looks like it's properly checked - ThreadStackHelper::GetNativeStack used to use snprintf_literal, but it was removed in 43 by bug 1196381 => not vulnerable bug 1197328 - no return values from snprintf used => not vulnerable bug 1197331 - '' bug 1302163: - security/sandbox/linux/SandboxUtil.cpp: UnshareUserNamespace uses SprintfLiteral, does have assertions, but they're debug-only. Even though they're unlikely to fail, we should probably handle them properly. - tools/profiler/core/platform.cpp: profiler_log used VsprintfLiteral but had proper checking and in any case was removed in 55 by bug 1346132 - xpcom/base/nsSystemInfo.cpp: nsSystemInfo::Init uses SprintfLiteral without a sufficient check (again, unlikely to overflow, but still) - xpcom/glue/nsStringAPI.cpp: nsA[C]String::AppendInt used SprintfLiteral without a check, but it would only be a problem if int were 8 bytes (I'm not aware of any implementation where it actually is) and anyway this was removed in 54 by bug 1332639. So it looks like we only need to be concerned with bug 1197311, bug 1197313, bug 1197314 (fixing bug already filed), bug 1197315, and bug 1302163. I'll file those bugs tomorrow.
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3) > bug 1197311: > - dom/plugins/base/nsPluginHost.cpp: > nsPluginHost::ParsePostBufferToFixHeaders uses the return value => check > needs to be updated from '==' to '>=' (it's probably not vulnerable in > practice, since it doesn't look like the format could ever actually be > larger than the buffer allocated, but still) > - dom/system/gonk/NetworkUtils.cpp: NetworkUtils::removeDefaultRoute has a Whoops - that's NetworkUtils::setInterfaceDns, rather. In any case, filed bug 1369543. > bug 1197313 - netwerk/protocol/http/nsHttpHandler.cpp: > PrepareAcceptLanguages uses snprintf without properly checking it => needs > fixing Filed bug 1369545. > bug 1197315 - gfx/layers/composite/FPSCounter.cpp: > - FPSCounter::WriteFrameTimeStamps uses SprintfLiteral without properly > checking it, although in practice it shouldn't overflow => needs fixing > (unless this is performance-critical?) > - FPSCounter::PrintHistogram uses snprintf with an assertion and I have no > idea how it's not failing in the general case, unless I'm misreading it. In > any case, this should be fixed and/or clarified. Turns out, this will indeed fail: [Parent 2304] ###!!! ASSERTION: Buffer overrun while printing FPS histogram.: 'length >= kBufferLength', file /home/keeler/mozilla-unified/gfx/layers/composite/FPSCounter.cpp, line 314 Filed bug 1369560. > bug 1302163: > - security/sandbox/linux/SandboxUtil.cpp: UnshareUserNamespace uses > SprintfLiteral, does have assertions, but they're debug-only. Even though > they're unlikely to fail, we should probably handle them properly. > - tools/profiler/core/platform.cpp: profiler_log used VsprintfLiteral but > had proper checking and in any case was removed in 55 by bug 1346132 > - xpcom/base/nsSystemInfo.cpp: nsSystemInfo::Init uses SprintfLiteral > without a sufficient check (again, unlikely to overflow, but still) > - xpcom/glue/nsStringAPI.cpp: nsA[C]String::AppendInt used SprintfLiteral > without a check, but it would only be a problem if int were 8 bytes (I'm not > aware of any implementation where it actually is) and anyway this was > removed in 54 by bug 1332639. Filed bug 1369561.
Updated•7 years ago
|
Comment 5•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3) > - security/sandbox/linux/SandboxUtil.cpp: UnshareUserNamespace uses > SprintfLiteral, does have assertions, but they're debug-only. Even though > they're unlikely to fail, we should probably handle them properly. rs=me to change those to MOZ_RELEASE_ASSERT, if you want. (We're going to crash anyway if this fails, and it should be mathematically impossible for the buffer to be too short, so there's no point in trying to recover. And, after bug 1151624, what's left of this function will probably return void.) Incidentally, SafeSPrintf from security/sandbox/chromium is another thing with snprintf's return value behavior. It's currently used in SandboxBrokerClient, which checks for and handles truncation, and in the SANDBOX_LOG_ERROR macro, which discards the return value and strlen()s the maybe-truncated string instead.
Updated•7 years ago
|
Group: crypto-core-security → dom-core-security
Comment 6•7 years ago
|
||
[Tracking Requested - why for this release]: Unknown number of potential buffer overflows.
Assignee: nobody → dkeeler
status-firefox57:
--- → affected
tracking-firefox57:
--- → ?
Keywords: meta,
sec-critical
Priority: -- → P1
Comment 7•7 years ago
|
||
Daniel, did anything change recently here? or it is just time to fix that?
status-firefox56:
--- → wontfix
tracking-firefox-esr52:
--- → ?
Updated•7 years ago
|
Flags: needinfo?(dveditz)
Comment 8•7 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #7) > Daniel, did anything change recently here? or it is just time to fix that? We started landing fixes for some of the dependent bugs, which potentially reveal the fact that we weren't using these APIs safely and could trigger a search for others that would be exploitable.
Flags: needinfo?(dveditz)
Updated•7 years ago
|
Updated•7 years ago
|
Keywords: csectype-bounds
Comment 9•7 years ago
|
||
Not sure what I can usefully do here. But tracking for 58 anyway. keeler, are you doing this audit for 57 or 58?
Assignee | ||
Comment 10•7 years ago
|
||
Comment 4 is the audit - the bugs this bug depends on are all the ones I found. I'm taking care of bug 1369561 (it'll get checked in on the 9th). I suppose from one point of view this bug can be closed - the regressions have been identified. It's just a matter of fixing them now.
Flags: needinfo?(dkeeler)
Comment 11•7 years ago
|
||
OK, thanks, I missed that! Looks like everything in comment 4 got triaged appropriately. Anyhing over sec-moderate is fixed already in 58/esr, and if not sec-high or sec-critical, we aren't tracking it.
Comment 12•7 years ago
|
||
Going to call this fixed based on comment 10.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 13•7 years ago
|
||
AFAICT, relevant bugs have landed on 57/58/ESR52 where deemed appropriate. Marking them as fixed accordingly.
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Updated•7 years ago
|
Whiteboard: [adv-main57-]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main57-] → [adv-main57-][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•