Bugzilla

Updated

•

11 years ago

Component: DOM → Document Navigation

Comment 1

•

11 years ago

Couldn't reproduce on a non-Asan build.

Comment 2

•

11 years ago

Now I managed to reproduce.

Assignee: nobody → bugs

Comment 3

•

11 years ago

But I'll let Trevor to fix this and other cases where array[i] used to be safe but isn't anymore.

Assignee: bugs → nobody

Blocks: 841434

Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]

Comment 4

•

11 years ago

Note, VoidArray::operator[] is safe, nsTArray::operator[] isn't.

Updated

•

11 years ago

Flags: sec-bounty?

Boris Zbarsky [:bzbarsky]

Assignee

Comment 5

•

11 years ago

(In reply to Olli Pettay [:smaug] from comment #2)
> Now I managed to reproduce.

interesting, I still can't (by typing the path to test.html in the url bar several times)

(In reply to Olli Pettay [:smaug] from comment #3)
> But I'll let Trevor to fix this and other cases where array[i] used to be
> safe but isn't anymore.

So, the cases we need to fix are the ones where we can mutate the array while iterating it, however I think its pretty infesable to tell which places do this just by code inspection.  I still can't see how Stop() does it, but I suspect its calling Cancel() on mLoadGroup.

(In reply to Olli Pettay [:smaug] from comment #4)
> Note, VoidArray::operator[] is safe, nsTArray::operator[] isn't.

I really doubt we want to play wack a mole with security bugs to find all the places we do this, but I'm not sure we really want to keep using SafeElementAt() everywhere, or using nsTObserverArray and iterating over it everywhere either.  I guess we could use SafeElementAt() remove the null checks and play wack a mole with non exploitable crashes, but that seems worse than just using nsTObserverArray iterators everywhere.

Assignee: nobody → trev.saunders

Updated

•

11 years ago

tracking-firefox21: --- → ?

Updated

•

11 years ago

Keywords: crash, csec-bounds, sec-critical, testcase

bhavana bajaj [:bajaj]

Updated

•

11 years ago

status-firefox21: --- → affected

tracking-firefox21: ? → +

Comment 6

•

11 years ago

So, this is now in Aurora. I think we should just back out bug 841434, and fix that bug
in some other way in m-c

Andrew McCreight [:mccr8]

Assignee

Comment 7

•

11 years ago

(In reply to Olli Pettay [:smaug] from comment #6)
> So, this is now in Aurora. I think we should just back out bug 841434, and
> fix that bug
> in some other way in m-c

So, I tend to think it would be easier to just decide how we want to fix the issue and do that.  For example if we don't really care about fixing the issue of mutation we could just make ChildAt() into SafeChildAt() and be done with it.

Updated

•

11 years ago

status-firefox20: --- → unaffected

Keywords: regression

Andrew McCreight [:mccr8]

Updated

•

11 years ago

status-firefox22: --- → affected

Assignee

Comment 8

•

11 years ago

Attached patch bug 842078 - properly handle mutations to mChildList while iterating over it (obsolete) — Details — Splinter Review

of course you should look over the remaining callers of ChildAt() and make sure I'm right that we can leave them all as is.

Attachment #716220 - Flags: review?(bzbarsky)

Al Billings [:abillings - ex-MoCo]

Updated

•

11 years ago

status-b2g18: --- → unaffected

status-firefox-esr17: --- → unaffected

tracking-firefox22: --- → +

Updated

•

11 years ago

status-firefox19: --- → unaffected

Boris Zbarsky [:bzbarsky]

Comment 9

•

11 years ago

Comment on attachment 716220 [details] [diff] [review]
bug 842078 - properly handle mutations to mChildList while iterating over it

Olli, do you have the bandwidth to review this properly?  If not, punt back to me, but it'll be sometime next week...

Attachment #716220 - Flags: review?(bzbarsky) → review?(bugs)

Comment 10

•

11 years ago

Comment on attachment 716220 [details] [diff] [review]
bug 842078 - properly handle mutations to mChildList while iterating over it

>@@ -104,21 +104,21 @@ protected:
>     virtual nsresult SetDocLoaderParent(nsDocLoader * aLoader);
> 
>     bool IsBusy();
> 
>     void Destroy();
>     virtual void DestroyChildren();
> 
>     nsIDocumentLoader* ChildAt(int32_t i) {
>-        return mChildList[i];
>+        return mChildList.ElementAt(i);
>     }
So, this used to be safe when we had voidarray member variable.
Have you gone through *all* the cases when ChildAt is called?
Actually, I think we should keep ChildAt as safe, as it was before the regression and
remove SafeChildAt.



>@@ -252,17 +252,17 @@ protected:
>     nsCOMPtr<nsIRequest>       mDocumentRequest;       // [OWNER] ???compare with document
> 
>     nsDocLoader*               mParent;                // [WEAK]
> 
>     nsVoidArray                mListenerInfoList;
> 
>     nsCOMPtr<nsILoadGroup>        mLoadGroup;
>     // We hold weak refs to all our kids
>-    nsTArray<nsDocLoader*>                   mChildList;
>+    nsTObserverArray<nsDocLoader*>                   mChildList;
You could remove some spaces before mChildList.

Attachment #716220 - Flags: review?(bugs) → review-

Assignee

Comment 11

•

11 years ago

(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 716220 [details] [diff] [review]
> bug 842078 - properly handle mutations to mChildList while iterating over it
> 
> >@@ -104,21 +104,21 @@ protected:
> >     virtual nsresult SetDocLoaderParent(nsDocLoader * aLoader);
> > 
> >     bool IsBusy();
> > 
> >     void Destroy();
> >     virtual void DestroyChildren();
> > 
> >     nsIDocumentLoader* ChildAt(int32_t i) {
> >-        return mChildList[i];
> >+        return mChildList.ElementAt(i);
> >     }
> So, this used to be safe when we had voidarray member variable.
> Have you gone through *all* the cases when ChildAt is called?

all the ones in nsDocLoader / nsDocShell yes, though I suppose renaming it might have been good to be 110% sure

> Actually, I think we should keep ChildAt as safe, as it was before the
> regression and
> remove SafeChildAt.

if we really don't care about extra checks sure that's fine with me.

Comment 12

•

11 years ago

We have had the checks there since the beginning of time, and this isn't very hot code, so
I'd prefer keeping ChildAt safe.
(But still move to use observer array)

Assignee

Comment 13

•

11 years ago

(In reply to Olli Pettay [:smaug] from comment #12)
> We have had the checks there since the beginning of time, and this isn't
> very hot code, so
> I'd prefer keeping ChildAt safe.
> (But still move to use observer array)

ok

Assignee

Comment 14

•

11 years ago

Attached patch bug 842078 - properly handle mutations to mChildList while iterating over it — Details — Splinter Review

Attachment #716220 - Attachment is obsolete: true

Attachment #717252 - Flags: review?(bugs)

Updated

•

11 years ago

Attachment #717252 - Flags: review?(bugs) → review+

Assignee

Comment 15

•

11 years ago

Comment on attachment 717252 [details] [diff] [review]
bug 842078 - properly handle mutations to mChildList while iterating over it

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

once you figure out the next question you need to create an exploit based on us calling methods on a deleted object

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

it's probably fairly obvious the rason for the change is to protect against mutation while iterating over an array combined with the fact is associated with a security bug that probably makes it obvious that its possible to create a out of bounds read.

Which older supported branches are affected by this flaw?

only aurora

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

bug 841434

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

shouldn't be more risky than this patch, and should apply with no trouble. backing out the causing bug should also be easy

How likely is this patch to cause regressions; how much testing does it need?

it's very unlikely but possible that it could introduce a null deref, and this fixes behaviour in the case of mutation while iterating which could also theoretically break something.

Attachment #717252 - Flags: sec-approval?

Assignee

Comment 16

•

11 years ago

Attached patch backout for aurora — Details — Splinter Review

the right fix scares me a little mostly just because this is docshell stuff, and the real bug has probably existed since 99 so given we know how to fix it lets just back out for aurora

Al Billings [:abillings - ex-MoCo]

Assignee

Comment 17

•

11 years ago

Comment on attachment 718012 [details] [diff] [review]
backout for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Attachment #718012 - Flags: approval-mozilla-aurora?

Updated

•

11 years ago

Attachment #717252 - Flags: sec-approval? → sec-approval+

Assignee

Comment 18

•

11 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> Comment on attachment 718012 [details] [diff] [review]
> backout for aurora
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined:  exploitable bug could cause crashes
> Testing completed (on m-c, etc.):  none (branch only backout)
> Risk to taking this patch (and alternatives if risky): basically none, it brings us back to the behaviour we had forever.   we could take the patch to be landed on m-c that actually fixes the issue, but that seems more risky
> String or UUID changes made by this patch: none

bhavana bajaj [:bajaj]

Comment 19

•

11 years ago

Comment on attachment 718012 [details] [diff] [review]
backout for aurora

Approving backout of 841434, fixing a sec-crit bug.

Attachment #718012 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Ryan VanderMeulen [:RyanVM]

Assignee

Comment 20

•

11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1444bada530a

Comment 21

•

11 years ago

https://hg.mozilla.org/mozilla-central/rev/1444bada530a

Can we land a test for this?

Status: NEW → RESOLVED

Closed: 11 years ago

status-firefox22: affected → fixed

Flags: in-testsuite?

Resolution: --- → FIXED

Target Milestone: --- → mozilla22

Ryan VanderMeulen [:RyanVM]

Comment 22

•

11 years ago

Aurora backout:
https://hg.mozilla.org/releases/mozilla-aurora/rev/954fbeed220c

status-firefox21: affected → fixed

Assignee

Comment 23

•

11 years ago

> Can we land a test for this?

in theory certainly we should be able to test at least the Stop() change, but the test case is not great, and its really not clear what exactly causes us  to mutate the children list while we're iterating over it so it seems hard :/

Updated

•

11 years ago

Flags: sec-bounty? → sec-bounty+