1624098 - Console::mGroupStack is modified and read from workers on both console and main threads

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P2)

Description

[Karl Tomlinson (:karlt)]

Console::mGroupStack is an nsTArray<nsString>.

PopulateConsoleNotificationInTheTargetScope() calls ComposeAndStoreGroupName() from both ProcessCallData() main thread and NotifyHandler() and RetrieveConsoleEvents() console thread.

I assume that both the ProcessCallData() and NotifyHandler() paths can be triggered from content with strings controlled by content.

Comment 1

[Karl Tomlinson (:karlt)]

This seems to be a regression from
https://hg.mozilla.org/mozilla-central/rev/71cabfed10139f2a7280834673326a55040da658#l7.102

Comment 2

[Karl Tomlinson (:karlt)]

An exploit would involve a race, but content has a fair bit of control of the timing. I'm not on the security team but still looks sec-high.

Comment 3

[Karl Tomlinson (:karlt)]

"dom.worker.console.dispatch_events_to_main_thread" could be toggled to defeat an exploit, but would disable console messages until bug 1592584 is fixed.

Comment 4

[Karl Tomlinson (:karlt)]

There was another issue where mCallDataStorage was cleared on "memory-pressure" notification without moving the elements to mCallDataStoragePending, which presumably could lead to problems similar to that of bug 1244995 due to lack of tracing of JS::Heap<> members.

Both these issues were incidentally fixed on 76 in this subseries of patches for bug 1492011:
https://hg.mozilla.org/mozilla-central/log?rev=ancestors(59bebd739346)-ancestors(14b59d4adc95)

The issues remain on ESR68. While it may be possible to produce a somewhat smaller fix for ESR68, I suspect it would be less risky to fix if we can apply the same changes as for m-c. I'll need to investigate how easily these can be rebased.

Comment 5

[Jens Stutte [:jstutte]]

Hi Karl, do you expect to have a solution for ESR68 soon? Thank you!

Comment 6

[Karl Tomlinson (:karlt)]

My intention was to first let the fix for 76 ride to Release, then prepare a patch for ESR68. This would avoid making public any patch associated with this bug before Release channel users have a fixed version. Does that make sense?

Comment 7

[Jens Stutte [:jstutte]]

I think so, yes. Thanks!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

That doesn't sound like it'd work well with our usual bug disclosure policies. Dan should weigh in on that plan before we move forward with that.

Comment 9

[Daniel Veditz [:dveditz]]

I think Karl is saying that this vulnerability is fully fixed by bug 1492011. I'm marking this bug to match--please let me know if that's wrong and there's something more to do here!

This would avoid making public any patch associated with this bug before Release channel users have a fixed version. Does that make sense?

I may be missing something. Are Release users more at risk than ESR users? Is it a risky change such that Beta testing isn't good enough? Is that testing even relevant if you have to craft a very different ESR patch from bits taken from a 14-part mozilla-central patch? I don't see any benefit to waiting until the next release.

Since this will require a different ESR patch is there any chance to get more-than-usual ESR QA testing? We can hold the patch until we have QA lined up to test a build which will probably be later in the cycle. But waiting for the next cycle doesn't help anything.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Note that next week is the final week of the cycle prior to RC week.

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: uplift changes 97e12e488248..59bebd739346 from bug 1492011

consolidate StoreProfileData and StoreConsoleData into a single method with arguments parameter r=baku
store raw JS arguments on Console separately from ConsoleCallData r=baku
Remove Console::mCallDataStoragePending r=baku
Remove now-unused Console::mStatus r=baku
change ShouldIncludeStackTrace from instance to class method r=baku
replace ArgumentsToValueList instance method with nsTArray::AppendElements() r=baku
provide group stack parameter to PopulateConsoleNotificationInTheTargetScope() r=baku
change ProcessArguments from instance method to nonmember function with internal linkage r=baku
change CreateCounterOrResetCounterValue from instance method to nonmember function with internal linkage r=baku
change CreateStartTimerValue and CreateLogOrEndTimerValue from instance to class method r=baku
provide ID and Prefix on ConsoleCallData r=baku
introduce a separate class to hold main-thread data associated with each Console r=baku

Mechanical conflict resolution was required to address conflicts with these changes not on esr68:
https://hg.mozilla.org/mozilla-central/rev/11cda2e9615b
https://hg.mozilla.org/mozilla-central/rev/9919570a9843
https://hg.mozilla.org/mozilla-central/rev/c9c7f800aae6
https://hg.mozilla.org/mozilla-central/diff/30100f2c612832dc74831e4b1b36bec636d5b396/dom/console/Console.cpp

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 9141138 [details]
uplift changes 97e12e488248..59bebd739346 from bug 1492011

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Some understanding of the surrounding code would be required.

A reader would not normally suspect that there is a security issue, except for the patch being labelled with a security bug number.

• 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?: ESR68. ESR60 is also affected - does Thunderbird expose workers?
• If not all supported branches, which bug introduced the flaw?: Bug 1357473
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This patch is for ESR68. Comments identify the reasons for differences.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, as this has been on m-c for some time.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Daniel Veditz [:dveditz] from comment #9)

I think Karl is saying that this vulnerability is fully fixed by bug 1492011.

That's correct.

This would avoid making public any patch associated with this bug before Release channel users have a fixed version. Does that make sense?

I may be missing something. Are Release users more at risk than ESR users?

No. There is no known difference in severity between Release and ESR.

Is it a risky change such that Beta testing isn't good enough?

No. Beta testing is sufficient.

Is that testing even relevant if you have to craft a very different ESR patch from bits taken from a 14-part mozilla-central patch? I don't see any benefit to waiting until the next release.

Testing is not the issue, but the ESR patch I've posted is not very different.

The issue is merely that making an ESR patch public now would add risk to a proportion of users that do not need to be exposed to that risk.

That can be weighed against the risk of delaying by one month the ESR fix for a 3-year-old bug. I'm assuming there is no additional risk to ESR users from the publicity of changes for bug 1492011 because no security issue is identified there and the motivations described relate to a feature that is disabled on ESR.

Is there another reason to justify adding risk to Release channel users?

Is there a need for "disclosure" of this bug just because bug 1492011 was fixed?

But waiting for the next cycle doesn't help anything.

It does help Release channel users. I assume that is not under question, but it is just the relative value of that benefit that is under question?

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Karl Tomlinson (:karlt) from comment #12)

• How easily could an exploit be constructed based on the patch?: Some understanding of the surrounding code would be required.
  A reader would not normally suspect that there is a security issue, except for the patch being labelled with a security bug number.

Why would we label with this bug number? We already have a perfect cover bug so change it to reference bug 1492011 instead. Or "relevant parts of bug 1492011".

• Which older supported branches are affected by this flaw?: ESR68. ESR60 is also affected - does Thunderbird expose workers?

ESR60 is unsupported. Both Thunderbird and Fennec are on the ESR68 branch (so is Tor Browser).

If we check in an ESR fix without mentioning this bug at all (as you didn't on trunk) it should not add meaningful risk to Release users.

Comment 15

[Karl Tomlinson (:karlt)]

I won't change the "Bugzilla Bug ID" on https://phabricator.services.mozilla.com/D71252 because I'm not confident that phabricator will keep the history hidden.

I can submit a separate revision, with the same contents, to bug 1492011.
Is that what you'd like me to do, or would you like me to attach a plain patch to this bug, without using phabricator?

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Daniel Veditz [:dveditz] from comment #14)

Why would we label with this bug number? We already have a perfect cover bug so change it to reference bug 1492011 instead.

I thought any ESR uplift would be suspicious, especially one that is not clearly a null-offset deref., but I can go ahead with whatever you prefer.
I need to know how and where to submit the patch, where to request sec-approval, and where to request uplift.

Comment 17

[Daniel Veditz [:dveditz]]

(In reply to Karl Tomlinson (:karlt) from comment #15)

I won't change the "Bugzilla Bug ID" on https://phabricator.services.mozilla.com/D71252 because I'm not confident that phabricator will keep the history hidden.

You're absolutely right -- we can't change the number associated with the D71252 revision itself. I was hoping we could just change it in the commit message. Unfortunately, it looks like even on the ESR branch (which I didn't think were autolanded) we still normally have the phabricator links, so either that will give someone "access denied" (defeating the purpose of using a buglink that doesn't) or it will be suspiciously missing.

I can submit a separate revision, with the same contents, to bug 1492011.
Is that what you'd like me to do, or would you like me to attach a plain patch to this bug, without using phabricator?

Looks like we need a separate revision with the same contents in bug 1492011 and request uplift there ("stability fix" I guess). sec-approval+ for that, but don't actually request sec-approval in bug 1492011 -- just normal branch uplift.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 9141138 [details]
uplift changes 97e12e488248..59bebd739346 from bug 1492011

Sec-approval+ to land on ESR, but not from this phabricator link. Add this as a revision to bug 1492011 and we'll handle it like a stability fix.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/e388563552d694596b996433cc90c6f5ab7d8154