1237831 - nsContentUtils::LogMessageToConsole() shouldn't take a format string as argument

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Arthur Edelstein [:arthur]]

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

Comment 1

[Arthur Edelstein [:arthur]]

Attachment: 0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.patch

Comment 2

[Arthur Edelstein [:arthur]]

Attachment: 0002-Use-a-non-format-argument-in-LogMessageToConsole.patch

Comment 3

[Arthur Edelstein [:arthur]]

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.

Comment 4

[Johnny Stenback (:jst)]

Thanks for the fixes, seems like a foot-gun worthy of removing.

Comment 5

[Arthur Edelstein [:arthur]]

Attachment: 0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.patch

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

This needs a security rating and possible sec-approval before it can land.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 7

[Arthur Edelstein [:arthur]]

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.

Comment 8

[Arthur Edelstein [:arthur]]

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.)

Comment 9

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 10

[Andrew McCreight [:mccr8]]

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!

Comment 11

[Bob Owen (:bobowen)]

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

[Andrew McCreight [:mccr8]]

I'll mark this as sec-other if this is off by default.

Comment 13

[Daniel Veditz [:dveditz]]

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).

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 8705834 [details] [diff] [review]
0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.patch

[Triage Comment]

Comment 15

[Carsten Book [:Tomcat]]

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!

Comment 16

[Bob Owen (:bobowen)]

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. :)

Comment 17

[Arthur Edelstein [:arthur]]

(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.

Comment 18

[Arthur Edelstein [:arthur]]

Attachment: 0001-Update-GonkGPSGeolocationProvider.cpp-to-use-B2G-sty.v44.patch

(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.

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=20659974&repo=mozilla-inbound

Comment 22

[Arthur Edelstein [:arthur]]

Attachment: 0002-Bug-17931-Use-a-non-format-argument-in-LogMessageToC.patch

Fixing the bitrot in my patch.

Comment 23

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7cfbfa3139b4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2abc599b0464

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7cfbfa3139b4
https://hg.mozilla.org/mozilla-central/rev/2abc599b0464

Comment 25

[Bob Owen (:bobowen)]

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.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

I just asked for Aurora and Beta approval on Bug 1245033

Comment 27

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-17931-Use-a-non-format-argument-in-LogMessageToC.v45.patch

Comment 28

[Arthur Edelstein [:arthur]]

(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.

Comment 29

[Bob Owen (:bobowen)]

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.

Comment 30

[Bob Owen (:bobowen)]

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.)

Comment 31

[Bob Owen (:bobowen)]

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.)

Comment 32

[Bob Owen (:bobowen)]

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

[Bob Owen (:bobowen)]

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

[Bob Owen (:bobowen)]

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

[Bob Owen (:bobowen)]

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.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

esr45 has not branched yet, so, it will have this patch! Thanks!

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 38

[Bob Owen (:bobowen)]

(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.

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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!

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I discussed this one with Al and he pointed out that since this is sec-moderate issue, we don't need it in esr38.