Closed
Bug 1237831
Opened 9 years ago
Closed 9 years ago
nsContentUtils::LogMessageToConsole() shouldn't take a format string as argument
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: arthur, Assigned: arthur)
References
Details
(Keywords: sec-moderate, Whiteboard: [tor][adv-main45-][post-critsmash-triage]lurking (but currently inert in Firefox) exploitable bug)
Attachments
(4 files, 2 obsolete files)
1.91 KB,
patch
|
jst
:
review+
dveditz
:
approval-mozilla-aurora+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
6.17 KB,
patch
|
dveditz
:
approval-mozilla-aurora+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
6.48 KB,
patch
|
Details | Diff | Splinter Review | |
2.69 KB,
patch
|
Details | Diff | Splinter Review |
The use of nsContentUtils::LogMessageToConsole here https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/security/sandbox/chromium-shim/sandbox/win/loggingCallbacks.h#107 looks like a possible format string vulnerability. Here is the original Tor Browser bug comment where this was discovered: https://trac.torproject.org/projects/tor/ticket/17931#comment:8
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Also, the name and lack of documentation of nsContentUtils::LogMessageToConsole does not providing any warning that a format string is expected in the first argument, and therefore should be a string literal. In Tor Browser, we used the attached patches (above) to change the signature of nsContentUtils::LogMessageToConsole to a plain, non-format, string. And we changed the usage of that function in GonkGPSGeolocationProvider.cpp to use logging more similar to other B2G files.
Assignee | ||
Updated•9 years ago
|
Component: General → DOM: Core & HTML
Assignee | ||
Updated•9 years ago
|
Attachment #8705401 -
Flags: review?(jst)
Assignee | ||
Updated•9 years ago
|
Attachment #8705403 -
Flags: review?(jst)
Updated•9 years ago
|
Attachment #8705401 -
Flags: review?(jst) → review+
Updated•9 years ago
|
Attachment #8705403 -
Flags: review?(jst) → review+
Comment 4•9 years ago
|
||
Thanks for the fixes, seems like a foot-gun worthy of removing.
Assignee | ||
Comment 5•9 years ago
|
||
Adding a couple of casts to get the B2G build working on the try server. Here are the try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8157731799b4&selectedJob=15232294
Attachment #8705401 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 6•9 years ago
|
||
This needs a security rating and possible sec-approval before it can land. https://wiki.mozilla.org/Security/Bug_Approval_Process
Keywords: checkin-needed
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8705403 [details] [diff] [review] 0002-Use-a-non-format-argument-in-LogMessageToConsole.patch [Security approval request comment] (This comment applies to both patches in this ticket.) > How easily could an exploit be constructed based on the patch? I'm not sure. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? > Probably not. > Which older supported branches are affected by this flaw? > If not all supported branches, which bug introduced the flaw? The potentially exploitable patch was introduced in Firefox 38. https://hg.mozilla.org/mozilla-central/rev/be47e1718018 > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Shouldn't be difficult to backport if desired. > How likely is this patch to cause regressions; how much testing does it need? I don't expect regressions. Please see try results.
Attachment #8705403 -
Flags: sec-approval?
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8705834 [details] [diff] [review] 0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.patch [Security approval request comment] (Please see comment for other patch attached to this ticket.)
Attachment #8705834 -
Flags: sec-approval?
Comment 9•9 years ago
|
||
This needs a security rating. What are the implications of this flaw? What can someone do with it? Based on https://trac.torproject.org/projects/tor/ticket/17931#comment:8 I assume this is an exploitable crash? How easy is it to trigger by a third party?
status-firefox43:
--- → wontfix
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox-esr38:
--- → affected
Comment 10•9 years ago
|
||
My understanding of this is that we are not currently using this function incorrectly in Firefox. Instead, this is a dangerous API that is used incorrectly in the Tor browser. The patches here make the API safer to use. Is that correct Arthur? In that case, this can be rated sec-want or something. Thanks for the patches!
Flags: needinfo?(arthuredelstein)
Comment 11•9 years ago
|
||
I'm not entirely sure whether aContext couldn't contain characters that looked like format specifiers. However the sandbox logging is off by default, it can be turned on via a pref or environment variable, but I guess if something is able to do that it is already game over. As everyone else has pointed out LogMessageToConsole is clearly dangerous in it's current form.
Comment 12•9 years ago
|
||
I'll mark this as sec-other if this is off by default.
Component: DOM: Core & HTML → DOM
Keywords: sec-other
Summary: Possible format string vulnerability → nsContentUtils::LogMessageToConsole() shouldn't take a format string as argument
Updated•9 years ago
|
Flags: needinfo?(arthuredelstein)
Updated•9 years ago
|
Assignee: nobody → arthuredelstein
Updated•9 years ago
|
Group: core-security → dom-core-security
Updated•9 years ago
|
tracking-firefox45:
--- → +
tracking-firefox46:
--- → +
tracking-firefox-esr38:
--- → 45+
Keywords: sec-other → sec-moderate
Whiteboard: lurking (but currently inert in Firefox) exploitable bug
Comment 13•9 years ago
|
||
Comment on attachment 8705403 [details] [diff] [review] 0002-Use-a-non-format-argument-in-LogMessageToConsole.patch sec-approval+ [Triage Comment] a=dveditz for aurora so we don't have to worry about this in the next ESR (45).
Attachment #8705403 -
Flags: sec-approval?
Attachment #8705403 -
Flags: sec-approval+
Attachment #8705403 -
Flags: approval-mozilla-aurora+
Comment 14•9 years ago
|
||
Comment on attachment 8705834 [details] [diff] [review] 0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.patch [Triage Comment]
Attachment #8705834 -
Flags: sec-approval?
Attachment #8705834 -
Flags: sec-approval+
Attachment #8705834 -
Flags: approval-mozilla-aurora+
Comment 15•9 years ago
|
||
problems during aurora-checkin - Tomcats-MacBook-Pro-2:mozilla-aurora Tomcat$ hg qimport /sheriffs/patch/1237831-1.patch && hg qpush && hg qrefresh -e adding 1237831-1.patch to series file applying 1237831-1.patch patching file dom/system/gonk/GonkGPSGeolocationProvider.cpp Hunk #3 FAILED at 205 1 out of 10 hunks FAILED -- saving rejects to file dom/system/gonk/GonkGPSGeolocationProvider.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh 1237831-1.patch can you take a look, thanks!
Flags: needinfo?(arthuredelstein)
Comment 16•9 years ago
|
||
Also Arthur - are you ready for this to be checked-in now? I can do that, if you are. Thanks for taking the time to upstream this. :)
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #16) > Also Arthur - are you ready for this to be checked-in now? > I can do that, if you are. > > Thanks for taking the time to upstream this. :) Hi Bob, Sorry for the delay. I think it's ready to be checked in. Thanks for your help.
Flags: needinfo?(arthuredelstein)
Keywords: checkin-needed
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #15) > problems during aurora-checkin - Tomcats-MacBook-Pro-2:mozilla-aurora > Tomcat$ hg qimport /sheriffs/patch/1237831-1.patch && hg qpush && hg > qrefresh -e > adding 1237831-1.patch to series file > applying 1237831-1.patch > patching file dom/system/gonk/GonkGPSGeolocationProvider.cpp > Hunk #3 FAILED at 205 > 1 out of 10 hunks FAILED -- saving rejects to file > dom/system/gonk/GonkGPSGeolocationProvider.cpp.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working directory > errors during apply, please fix and qrefresh 1237831-1.patch > > can you take a look, thanks! Hi Carsten, Here's a version that applies to Firefox 45.
Attachment #8712839 -
Flags: review?(cbook)
Comment 19•9 years ago
|
||
Comment on attachment 8712839 [details] [diff] [review] 0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.v44.patch landed on beta in https://hg.mozilla.org/releases/mozilla-beta/rev/81db231f08d8
Attachment #8712839 -
Flags: review?(cbook)
Updated•9 years ago
|
Comment 20•9 years ago
|
||
landed on mozilla-inbound as remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/42ee56af794c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e6d241e103
Updated•9 years ago
|
Keywords: checkin-needed
Comment 21•9 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=20659974&repo=mozilla-inbound
Flags: needinfo?(arthuredelstein)
Assignee | ||
Comment 22•9 years ago
|
||
Fixing the bitrot in my patch.
Attachment #8705403 -
Attachment is obsolete: true
Flags: needinfo?(arthuredelstein)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
status-firefox47:
--- → affected
Comment 23•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7cfbfa3139b4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2abc599b0464
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7cfbfa3139b4 https://hg.mozilla.org/mozilla-central/rev/2abc599b0464
Updated•9 years ago
|
Group: dom-core-security → core-security-release
Comment 25•9 years ago
|
||
I see that a fix for Bug 1245033 has just landed on m-i. We should get Aurora and Beta approval for that, tomcat was just about to uplift this to Aurora. We should wait and do them together. Note that the GonkGPSGeolocationProvider patch has already hit Beta (due to some confusing use of flags), so that compile issue for B2G will still be there until we uplift. The 0002 LogMessageToConsole patch has not hit Beta yet.
Flags: needinfo?(ferjmoreno)
Comment 26•9 years ago
|
||
I just asked for Aurora and Beta approval on Bug 1245033
Flags: needinfo?(ferjmoreno)
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8716095 -
Flags: checkin?(cbook)
Assignee | ||
Comment 28•9 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #19) > Comment on attachment 8712839 [details] [diff] [review] > 0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.v44.patch > > landed on beta in > https://hg.mozilla.org/releases/mozilla-beta/rev/81db231f08d8 It was pointed out to me that this landing included only one of two patches needed for mozilla-beta. I just posted the second attachment (for Firefox 45) in comment 27.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 29•9 years ago
|
||
Comment on attachment 8716095 [details] [diff] [review] 0001-Bug-17931-Use-a-non-format-argument-in-LogMessageToC.v45.patch This patch is not the same as the previous one, which my well apply to Beta as it stands ... just about to test that. It just removes a #include. Also this bug needs to be uplifted with bug 1245033 because a previous fix including some casts, was incorrectly merge into the wrong patch and never uploaded.
Attachment #8716095 -
Attachment is obsolete: true
Attachment #8716095 -
Flags: checkin?(cbook)
Comment 30•9 years ago
|
||
OK, here's what we need for the uplift to Aurora (this includes bug 1245033): https://bugzilla.mozilla.org/attachment.cgi?id=8705834 https://bugzilla.mozilla.org/attachment.cgi?id=8713321 https://bugzilla.mozilla.org/attachment.cgi?id=8714702 The comments for the first two need updating to add/fix the bug numbers. I'll comment separately about Beta. (Also setting this back to FIXED as it has landed on m-c and correcting flag for Beta as it hasn't all landed there.)
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 31•9 years ago
|
||
Comment on attachment 8705403 [details] [diff] [review] 0002-Use-a-non-format-argument-in-LogMessageToConsole.patch For Beta the first patch is already there, so we need the original version of the second patch for this bug (which I have just de-obsoleted) and the patch for bug 1245033: https://bugzilla.mozilla.org/attachment.cgi?id=8705403 https://bugzilla.mozilla.org/attachment.cgi?id=8714702 Again the comment for the first one needs updating to add the bug number and of course reviewer. (Note that when this was approved Aurora was Fx45, which is now on Beta.)
Attachment #8705403 -
Attachment is obsolete: false
Comment 32•9 years ago
|
||
Spoke to tomcat on IRC and he asked me to uplift these, I'll wait for Aurora to complete before pushing the rest to Beta.
Comment 33•9 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/da55b9d9191d remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/309b219937b8 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/f19a31a82ae3 This includes the push for bug 1245033.
Comment 34•9 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/4dbe3b781c32 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/ffb9c10f484b Again this includes the push for bug 1245033.
Comment 35•9 years ago
|
||
More of a "for info" than a "need info". If we decide to merge this to ESR45, which I guess we will. Here are all the Beta changesets again as it happened in two lots: https://hg.mozilla.org/releases/mozilla-beta/rev/81db231f08d8 https://hg.mozilla.org/releases/mozilla-beta/rev/4dbe3b781c32 https://hg.mozilla.org/releases/mozilla-beta/rev/ffb9c10f484b This includes the B2G compile fix from bug 1245033.
Flags: needinfo?(sledru)
Comment 36•9 years ago
|
||
esr45 has not branched yet, so, it will have this patch! Thanks!
Flags: needinfo?(sledru)
Bob, could you please nominate a patch for uplift to esr38? We still have two more ESR38 releases to ship before it is EOL'd. Thanks!
Flags: needinfo?(bobowen.code)
Comment 38•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #37) > Bob, could you please nominate a patch for uplift to esr38? We still have > two more ESR38 releases to ship before it is EOL'd. Thanks! I can do, but I thought from comment 13 (and just before) that this had already been rejected for ESR38.
Flags: needinfo?(bobowen.code)
(In reply to Bob Owen (:bobowen) from comment #38) > (In reply to Ritu Kothari (:ritu) from comment #37) > > Bob, could you please nominate a patch for uplift to esr38? We still have > > two more ESR38 releases to ship before it is EOL'd. Thanks! > > I can do, but I thought from comment 13 (and just before) that this had > already been rejected for ESR38. Bob, ESR45 will be branched off sometime this week. But we still need to do two esr releases, namely esr38.7, esr38.8 from esr38 branch. This fix landed on Fx45 and esr45 will be branched off of that. But we still have to uplift it to esr38 branch. Please nominate a patch for uplift to esr38. Thanks!
Flags: needinfo?(bobowen.code)
I discussed this one with Al and he pointed out that since this is sec-moderate issue, we don't need it in esr38.
Flags: needinfo?(bobowen.code)
Updated•9 years ago
|
Whiteboard: lurking (but currently inert in Firefox) exploitable bug → [post-critsmash-triage]lurking (but currently inert in Firefox) exploitable bug
Updated•9 years ago
|
tracking-firefox-esr38:
45+ → ---
Whiteboard: [post-critsmash-triage]lurking (but currently inert in Firefox) exploitable bug → [adv-main45-][post-critsmash-triage]lurking (but currently inert in Firefox) exploitable bug
Assignee | ||
Updated•8 years ago
|
Whiteboard: [adv-main45-][post-critsmash-triage]lurking (but currently inert in Firefox) exploitable bug → [tor][adv-main45-][post-critsmash-triage]lurking (but currently inert in Firefox) exploitable bug
Updated•8 years ago
|
Group: core-security-release
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•