Bugzilla

Updated

•

6 years ago

Group: firefox-core-security → dom-core-security

Component: Untriaged → Security: Process Sandboxing

Keywords: csectype-uaf, testcase-wanted

Product: Firefox → Core

Comment 1

•

6 years ago

Nika, any ideas? It looks like we're somehow trying to send a HangDetails after it was destroyed.

Component: Security: Process Sandboxing → XPCOM

Flags: needinfo?(nika)

Assignee

Comment 2

•

6 years ago

HangStack objects contain char* pointers into a buffer which is also on the object. This doesn't appear to be due to sending a HangDetails after it was destroyed, but rather that one of these HangStack objects somehow got a reference into another HangStack's backing buffer.

I looked through the code and don't know how that could have happened. If we somehow get a more reproducible test case then it will be much easier to figure out what happened.

Flags: needinfo?(nika)

Reporter

Comment 3

•

6 years ago

So far I have not been able to reproduce it with the samples buffer I have at that moment (I use to save the last 50 testcases before a crash). That doesn't means it somehow fixed. It happened twice, but I'm still looking in case I could get a specific testcase. Anyway, a second pair of eyes is appreciated.


> If we
> somehow get a more reproducible test case then it will be much easier to
> figure out what happened.

Updated

•

6 years ago

Comment 4

•

6 years ago

Have you had the chance to review #bug 1426184 stacktrace?

(In reply to Nika Layzell [:mystor] from comment #2)

Frederik Braun [:freddy]

Comment 5

•

6 years ago

(In reply to Francisco A. from comment #4)
> Have you had the chance to review #bug 1426184 stacktrace?
> 
> (In reply to Nika Layzell [:mystor] from comment #2)

Can you please take a look? :)

Flags: needinfo?(nika)

Assignee

Comment 6

•

6 years ago

I'm rewriting HangStack to remove all of the unsafe logic which could be causing the crash, and instead use obviously safe code in the hope that that fixes this issue.

Assignee

Comment 7

•

6 years ago

Attached patch Remove the complex custom HangStack class and replace it with a much simpler implementation (obsolete) — Details — Splinter Review

This patch is ~+200 -800, so I figure it's a pretty good one.

I tried to read through the HangStack class to find the problem but couldn't
because it is big and unweildy, so I just re-implemented it in an obviously safe
way. This also let me get around the need to manually write the IPC::ParamTraits
impls, as I could instead just write the data types in ipdl, which also reduced
the complexity of the component significantly.

MozReview-Commit-ID: qlBUnvYams

Attachment #8941643 - Flags: review?(nfroyd)

Assignee

Updated

•

6 years ago

Assignee: nobody → nika

Flags: needinfo?(nika)

Assignee

Comment 8

•

6 years ago

(In reply to Nika Layzell [:mystor] from comment #7)
> Created attachment 8941643 [details] [diff] [review]
> Remove the complex custom HangStack class and replace it with a much simpler
> implementation
> 
> This patch is ~+200 -800, so I figure it's a pretty good one.
> 
> I tried to read through the HangStack class to find the problem but couldn't
> because it is big and unweildy, so I just re-implemented it in an obviously
> safe
> way. This also let me get around the need to manually write the
> IPC::ParamTraits
> impls, as I could instead just write the data types in ipdl, which also
> reduced
> the complexity of the component significantly.
> 
> MozReview-Commit-ID: qlBUnvYams

I should note that the actual commmit message I'll push will probably be something along the lines of

Simplify `HangStack` and `HangDetails` by implementing using ipdl structs and unions, r=froydnj

'cause security bugs.

Reporter

Comment 9

•

6 years ago

code is much clearer, thank you!.


(In reply to Nika Layzell [:mystor] from comment #7)
> Created attachment 8941643 [details] [diff] [review]
> Remove the complex custom HangStack class and replace it with a much simpler
> implementation
> 
> This patch is ~+200 -800, so I figure it's a pretty good one.

Updated

•

6 years ago

Blocks: 1418125

Updated

•

6 years ago

No longer blocks: 1418125

Updated

•

6 years ago

Keywords: sec-high

Updated

•

6 years ago

Status: UNCONFIRMED → ASSIGNED

Ever confirmed: true

Nathan Froyd [:froydnj]

Comment 12

•

6 years ago

Comment on attachment 8941643 [details] [diff] [review]
Remove the complex custom HangStack class and replace it with a much simpler implementation

Review of attachment 8941643 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: "big and unwieldy" in the commit message.

This is way more understandable, thank you.  I should have suggested this in the original review.  Some notes below.

::: toolkit/components/backgroundhangmonitor/HangDetails.h
@@ +10,5 @@
>  #include "ipc/IPCMessageUtils.h"
>  #include "mozilla/ProcessedStack.h"
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/Move.h"
> +// #include "mozilla/HangStack.h"

Nit: just delete this rather than commenting it out?

::: toolkit/components/backgroundhangmonitor/ThreadStackHelper.cpp
@@ +190,5 @@
>    mDesiredStackSize += 1;
>  
>    // Perform the append if we have enough space to do so.
> +  if (mStackToFill->stack().Capacity() > mStackToFill->stack().Length()) {
> +    mStackToFill->stack().AppendElement(mozilla::Move(aFrame));

Last time I looked, IPDL-generated classes did not have move constructors.  Do we care about this inefficiency for purposes of this patch?  (Probably not?)

@@ +272,5 @@
> +    const char* entryLabel = aEntry.label();
> +
> +    // We need to do this slightly sketchy thing in order to put a literal
> +    // string value into the label. We cannot allocate here, so it's important
> +    // that we create a literal DataFlags string.

So, uh, ok.

I think a better comment here would explain why faking this as a literal string is justifiable.  I think, TryAppendFrame will copy the string anyway (see comments above), in which case we might as well do a copy here, since this comment is mis-stating the need to avoid allocation.  If I am wrong, and TryAppendFrame transfers the literalness of this string into storage, then we need an argument here about why it is safe to have a long-lived reference to aEntry.label().

Either way...something needs to change here.

@@ +275,5 @@
> +    // string value into the label. We cannot allocate here, so it's important
> +    // that we create a literal DataFlags string.
> +    nsCString label;
> +    label.AssignLiteral(entryLabel, strlen(entryLabel));
> +    TryAppendFrame(label);

This is not your fault, but I am reminded that the IPDL union constructs are implicit, and therefore this has magic going on, which is unfortunate.  Also, the implicitly-invoked constructor here will copy-construct from `label`, for which see the above objection.

Attachment #8941643 - Flags: review?(nfroyd)

Assignee

Comment 13

•

6 years ago

Attached patch Simplify `HangStack` and `HangDetails` by implementing using ipdl structs and unions — Details — Splinter Review

MozReview-Commit-ID: qlBUnvYams

Attachment #8942310 - Flags: review?(nfroyd)

Assignee

Updated

•

6 years ago

Attachment #8941643 - Attachment is obsolete: true

Reporter

Comment 14

•

6 years ago

Is this fix planned for Tuesday's 2018-01-23 release?

Nathan Froyd [:froydnj]

Comment 15

•

6 years ago

Comment on attachment 8942310 [details] [diff] [review]
Simplify `HangStack` and `HangDetails` by implementing using ipdl structs and unions

Review of attachment 8942310 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me.  Thanks for clarifying the code with a comment.

Attachment #8942310 - Flags: review?(nfroyd) → review+

Nathan Froyd [:froydnj]

Comment 16

•

6 years ago

(In reply to Francisco A. from comment #14)
> Is this fix planned for Tuesday's 2018-01-23 release?

It's unlikely that this would make it into Firefox 58, but it will likely be in Firefox 59 (released on 2018-03-13).

Reporter

Comment 17

•

6 years ago

(In reply to Nathan Froyd [:froydnj] from comment #16)
> It's unlikely that this would make it into Firefox 58, but it will likely be
> in Firefox 59 (released on 2018-03-13).

It's a bit disappointing.. Two months more seems too much to be honest.

Assignee

Comment 18

•

6 years ago

(In reply to Francisco A. from comment #17)
> It's a bit disappointing.. Two months more seems too much to be honest.

This code doesn't run outside of nightly builds of Firefox. This bug cannot affect release or beta users of the software.

Once we land the fix in central there's no point in even trying to uplift.

Assignee

Comment 19

•

6 years ago

In a related note, because of that I'm not sure this is actually sec-high.

Assignee

Comment 20

•

6 years ago

Comment on attachment 8942310 [details] [diff] [review]
Simplify `HangStack` and `HangDetails` by implementing using ipdl structs and unions

NOTE: I am not sure this bug is sec-high. The code being fixed does not run outside of nightly builds of Firefox, so despite it being present in older versions it should never be hit.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch doesn't expose any information about the exploit other than the part of code it might be in. The patch just wholesale replaces a potentially buggy component.

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?
None.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, They are unnecessary.

How likely is this patch to cause regressions; how much testing does it need?
This patch may cause regressions, but they should only impact our BHR telemetry. I imagine regressions are unlikely.

Attachment #8942310 - Flags: sec-approval?

Comment 21

•

6 years ago

(In reply to Nika Layzell [:mystor] from comment #18)
> This code doesn't run outside of nightly builds of Firefox. This bug cannot
> affect release or beta users of the software.
> 
> Once we land the fix in central there's no point in even trying to uplift.

Oh really? That's good to know. What we do in that situation is mark other branches as disabled, but still keep the sec-high rating. If it is Nightly-only, you don't need sec-approval.

Updated

•

6 years ago

status-firefox57: --- → disabled

status-firefox58: --- → disabled

status-firefox59: --- → affected

status-firefox-esr52: --- → unaffected

Whiteboard: [Nightly only]

Updated

•

6 years ago

Attachment #8942310 - Flags: sec-approval?

Assignee

Updated

•

6 years ago

Depends on: 1430850

Assignee

Comment 22

•

6 years ago

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/19c0ea91ed2b

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Reporter

Comment 23

•

6 years ago

(In reply to Nika Layzell [:mystor] from comment #22)
> Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/19c0ea91ed2b

Thank you!

Comment 24

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/19c0ea91ed2b

status-firefox59: affected → fixed

Target Milestone: --- → mozilla59

Updated

•

6 years ago

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

Resolution: --- → FIXED

Al Billings [:abillings - ex-MoCo]

Updated

•

6 years ago

Group: dom-core-security → core-security-release

Updated

•

6 years ago

Flags: sec-bounty? → sec-bounty+