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)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 - fixed
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 - fixed
firefox58 - fixed

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.
Doh, totally my fault.  Can you CC me to the other bug?
Flags: needinfo?(dkeeler)
Honestly, I'm inclined to blame PR_snprintf and/or snprintf here :)
Looks like RyanVM cc'd you to bug 1368652.
Flags: needinfo?(dkeeler)
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.
(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.
(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.
Group: crypto-core-security → dom-core-security
[Tracking Requested - why for this release]:
Unknown number of potential buffer overflows.
Assignee: nobody → dkeeler
Keywords: meta, sec-critical
Priority: -- → P1
Daniel, did anything change recently here? or it is just time to fix that?
Flags: needinfo?(dveditz)
(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)
Not sure what I can usefully do here. But tracking for 58 anyway. 
keeler, are you doing this audit for 57 or 58?
Flags: needinfo?(dkeeler)
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)
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.
Going to call this fixed based on comment 10.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
AFAICT, relevant bugs have landed on 57/58/ESR52 where deemed appropriate. Marking them as fixed accordingly.
Group: dom-core-security → core-security-release
Whiteboard: [adv-main57-]
Flags: qe-verify-
Whiteboard: [adv-main57-] → [adv-main57-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.