Closed Bug 1409951 Opened 2 years ago Closed 2 years ago

crash and potential UAF in [@ nsPlainTextSerializer::ScanElementForPreformat]

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ verified
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 + verified
firefox59 + verified

People

(Reporter: tsmith, Assigned: hsivonen)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main58+][adv-esr52.6+])

Crash Data

Attachments

(4 files, 2 obsolete files)

Attached file test_case.html
There are other reports from the wild that look like UAFs.
https://crash-stats.mozilla.com/report/index/2ffaf27e-2dc9-4688-9819-69dce0171018

==8881==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f61f1a1b01d bp 0x7fff82146030 sp 0x7fff82145ff0 T0)
==8881==The signal is caused by a READ memory access.
==8881==Hint: address points to the zero page.
    #0 0x7f61f1a1b01c in construct<bool, bool> src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/ext/new_allocator.h:120:4
    #1 0x7f61f1a1b01c in _M_push_back_aux<bool> src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/deque.tcc:456
    #2 0x7f61f1a1b01c in emplace_back<bool> src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/deque.tcc:142
    #3 0x7f61f1a1b01c in push_back src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/stl_deque.h:1413
    #4 0x7f61f1a1b01c in push src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/stl_stack.h:195
    #5 0x7f61f1a1b01c in nsPlainTextSerializer::ScanElementForPreformat(mozilla::dom::Element*) src/dom/base/nsPlainTextSerializer.cpp:384
    #6 0x7f61f191cc5e in nsDocumentEncoder::SerializeNodeStart(nsINode*, int, int, nsTSubstring<char16_t>&, nsINode*) src/dom/base/nsDocumentEncoder.cpp:367:18
    #7 0x7f61f191f4bc in nsDocumentEncoder::SerializeRangeNodes(nsRange*, nsINode*, nsTSubstring<char16_t>&, int) src/dom/base/nsDocumentEncoder.cpp:816:14
    #8 0x7f61f191f777 in nsDocumentEncoder::SerializeRangeNodes(nsRange*, nsINode*, nsTSubstring<char16_t>&, int) src/dom/base/nsDocumentEncoder.cpp:853:16
    #9 0x7f61f192134e in nsDocumentEncoder::SerializeRangeToString(nsRange*, nsTSubstring<char16_t>&) src/dom/base/nsDocumentEncoder.cpp:993:10
    #10 0x7f61f1922e34 in nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsTSubstring<char16_t>&) src/dom/base/nsDocumentEncoder.cpp:1126:12
    #11 0x7f61f184539a in SelectionCopyHelper(nsISelection*, nsIDocument*, bool, short, unsigned int, nsITransferable**) src/dom/base/nsCopySupport.cpp:182:22
    #12 0x7f61f5ce9c82 in nsAutoCopyListener::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) src/layout/generic/nsFrameSelection.cpp:3064:10
...
see log.txt
Flags: in-testsuite?
Attached file log.txt
Regression range:
INFO: Last good revision: 9ce31e9f90cb0e534611b0f617c5bbc232ffe748 (2016-04-26)
INFO: First bad revision: ab0044bfa1df858919797bcd6a9aef76a668cd4a (2016-04-27)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ce31e9f90cb0e534611b0f617c5bbc232ffe748&tochange=ab0044bfa1df858919797bcd6a9aef76a668cd4a

Interesting that all the crash reports with this signature go back only as far as Fx56, but the regression range for the attached testcase goes back to Fx49. Though I see bug 1370737 made changes to this code in the Fx56 timeframe, so maybe that's related somehow?

Also, I could only reproduce this crash on Linux. No luck on Win10.
Crash Signature: [@ nsPlainTextSerializer::ScanElementForPreformat]
Has Regression Range: --- → yes
Henri, perhaps you have time to look at this?
Flags: needinfo?(hsivonen)
Priority: -- → P1
Keywords: sec-high
I'll have a look once I get my rr machine back to bootable state.
(why does this need rr?)
Assignee: nobody → hsivonen
I reproed it under rr

#6  0x00007f59e28712b7 in nsPlainTextSerializer::ScanElementForPreformat (this=0x7f59b4dd9ed0, aElement=0x7f59cacf74c0)
    at /home/jesup/src/mozilla/inbound/dom/base/nsPlainTextSerializer.cpp:384
384	  mPreformatStack.push(IsElementPreformatted(aElement));

(rr) p/x mPreformatStack
$4 = std::stack wrapping: std::deque with -1 elements = {0xe5, 0xe5, 0xe5,...

and down in push(): 
#0  0x00007f59e2891a00 in __gnu_cxx::new_allocator<bool>::construct<bool, bool> (this=0x7f59b4dd9fc0, __p=0xe5e5e5e5e5e5e7e4, 
    __args#0=@0x7fffe1822f9f: false) at /usr/include/c++/7/ext/new_allocator.h:136
136		{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }

Note __p

mSerializer (nsPlainTextSerializer) seems sane other than the mPreformatStack.
Looks like it gets set to e5's here:
#4  0x00007f59e27c1848 in nsDocumentEncoder::SerializeNodeEnd (this=0x7f59b7250600, aNode=0x7f59cacf74c0, aStr=...)
    at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:448
448	    mSerializer->ForgetElementForPreformat(aNode->AsElement());

(rr) x/32x &mSerializer.mRawPtr->mPreformatStack
0x7f59b4dd9fc0:	0xcc19aa80	0x00007f59	0x00000008	0x00000000
0x7f59b4dd9fd0:	0xb7250a00	0x00007f59	0xb7250a00	0x00007f59
0x7f59b4dd9fe0:	0xb7250c00	0x00007f59	0xcc19aa98	0x00007f59
0x7f59b4dd9ff0:	0xe5e5e7e4	0xe5e5e5e5	0xe5e5e5e5	0xe5e5e5e5
0x7f59b4dda000:	0xe5e5e7e5	0xe5e5e5e5	0xcc19aa90	0x00007f59
0x7f59b4dda010:	0xffffffff	0xe5e5e5e5	0xb4dda060	0x00007f59
0x7f59b4dda020:	0x00000000	0x00000000	0xba48ade8	0x00007f59
0x7f59b4dda030:	0x00000001	0x00020005	0x00000000	0x00000000

(This was watching 0x7f59b4dd9ff0, which had just been changed to mostly e5's)

(rr) where
#0  0x00007f59e2890bcf in std::deque<bool, std::allocator<bool> >::_M_pop_back_aux() (this=0x7f59b4dd9fc0)
    at /usr/include/c++/7/bits/deque.tcc:552
#1  0x00007f59e288fab8 in std::deque<bool, std::allocator<bool> >::pop_back() (this=0x7f59b4dd9fc0)
    at /usr/include/c++/7/bits/stl_deque.h:1612
#2  0x00007f59e288d1c6 in std::stack<bool, std::deque<bool, std::allocator<bool> > >::pop() (this=0x7f59b4dd9fc0)
    at /usr/include/c++/7/bits/stl_stack.h:261
#3  0x00007f59e28712e6 in nsPlainTextSerializer::ForgetElementForPreformat(mozilla::dom::Element*) (this=0x7f59b4dd9ed0, aElement=0x7f59cacf74c0) at /home/jesup/src/mozilla/inbound/dom/base/nsPlainTextSerializer.cpp:391
#4  0x00007f59e27c1848 in nsDocumentEncoder::SerializeNodeEnd(nsINode*, nsTSubstring<char16_t>&) (this=0x7f59b7250600, aNode=0x7f59cacf74c0, aStr=...) at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:448
#5  0x00007f59e27c31df in nsDocumentEncoder::SerializeRangeContextEnd(nsTArray<nsINode*> const&, nsTSubstring<char16_t>&) (this=0x7f59b7250600, aAncestorArray=const nsTArray<nsINode*> & = {...}, aString=...) at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:924
#6  0x00007f59e27c456a in nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsTSubstring<char16_t>&) (this=0x7f59b7250600, aMaxLength=0, aOutputString=...) at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:1119
#7  0x00007f59e27c3aac in nsDocumentEncoder::EncodeToString(nsTSubstring<char16_t>&) (this=0x7f59b7250600, aOutputString=...)
    at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:1018
It appears we pop() more elements than were pushed on: max was 6 elements (all 'false'), and things got bad when we get to -1 elements.

The final pop() that leaves it bad:
0  0x00007f59e2890bcf in std::deque<bool, std::allocator<bool> >::_M_pop_back_aux() (this=0x7f59b4dd9fc0)
    at /usr/include/c++/7/bits/deque.tcc:552
#1  0x00007f59e288fab8 in std::deque<bool, std::allocator<bool> >::pop_back() (this=0x7f59b4dd9fc0)
    at /usr/include/c++/7/bits/stl_deque.h:1612
#2  0x00007f59e288d1c6 in std::stack<bool, std::deque<bool, std::allocator<bool> > >::pop() (this=0x7f59b4dd9fc0)
    at /usr/include/c++/7/bits/stl_stack.h:261
#3  0x00007f59e28712e6 in nsPlainTextSerializer::ForgetElementForPreformat(mozilla::dom::Element*) (this=0x7f59b4dd9ed0, aElement=0x7f59cacf74c0) at /home/jesup/src/mozilla/inbound/dom/base/nsPlainTextSerializer.cpp:391
#4  0x00007f59e27c1848 in nsDocumentEncoder::SerializeNodeEnd(nsINode*, nsTSubstring<char16_t>&) (this=0x7f59b7250600, aNode=0x7f59cacf74c0, aStr=...) at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:448
#5  0x00007f59e27c31df in nsDocumentEncoder::SerializeRangeContextEnd(nsTArray<nsINode*> const&, nsTSubstring<char16_t>&) (this=0x7f59b7250600, aAncestorArray=const nsTArray<nsINode*> & = {...}, aString=...) at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:924
#6  0x00007f59e27c456a in nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsTSubstring<char16_t>&) (this=0x7f59b7250600, aMaxLength=0, aOutputString=...) at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:1119
#7  0x00007f59e27c3aac in nsDocumentEncoder::EncodeToString(nsTSubstring<char16_t>&) (this=0x7f59b7250600, aOutputString=...)
    at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:1018
#8  0x00007f59e27c5ad7 in nsHTMLCopyEncoder::EncodeToString(nsTSubstring<char16_t>&) (this=0x7f59b7250600, aOutputString=...)
    at /home/jesup/src/mozilla/inbound/dom/base/nsDocumentEncoder.cpp:1476
#9  0x00007f59e27735ac in SelectionCopyHelper(nsISelection*, nsIDocument*, bool, int16_t, uint32_t, nsITransferable**) (aSel=0x7f59ca89fd40, aDoc=0x7f59cad47000, doPutOnClipboard=true, aClipboardID=0, aFlags=524288, aTransferable=0x0)
    at /home/jesup/src/mozilla/inbound/dom/base/nsCopySupport.cpp:182
#10 0x00007f59e27743b6 in nsCopySupport::HTMLCopy(nsISelection*, nsIDocument*, short, bool) (aSel=0x7f59ca89fd40, aDoc=0x7f59cad47000, aClipboardID=0, aWithRubyAnnotation=false) at /home/jesup/src/mozilla/inbound/dom/base/nsCopySupport.cpp:308
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)
> Also, I could only reproduce this crash on Linux. No luck on Win10.

The test case performs a selection. On Linux, a selection get automatically exported to the X11 middle-click clipboard, so the serializer runs upon selection. This doesn't happen on Windows.
There's a pop for <font> without a corresponding push prior.
This is yet another bug arising from the special-casing of <tr> for bug 137450.
nsDocumentEncoder::SerializeRangeContextStart and nsDocumentEncoder::SerializeRangeContextEnd don't match.
I think the way to proceed here is to keep a stack of contexts computed by SerializeRangeContextStart and have SerializeRangeContextEnd replay (in reverse) the latest context on the stack and then pop it.

If SerializeRangeContextEnd isn't responsible for computing the context, it can't compute it differently compared to SerializeRangeContextStart.
(In reply to Henri Sivonen (:hsivonen) from comment #13)
> I think the way to proceed here is to keep a stack of contexts computed by
> SerializeRangeContextStart and have SerializeRangeContextEnd replay (in
> reverse) the latest context on the stack and then pop it.
> 
> If SerializeRangeContextEnd isn't responsible for computing the context, it
> can't compute it differently compared to SerializeRangeContextStart.

Makes lots of sense!
(In reply to Henri Sivonen (:hsivonen) from comment #13)
> I think the way to proceed here is to keep a stack of contexts computed by
> SerializeRangeContextStart and have SerializeRangeContextEnd replay (in
> reverse) the latest context on the stack and then pop it.
> 
> If SerializeRangeContextEnd isn't responsible for computing the context, it
> can't compute it differently compared to SerializeRangeContextStart.

My initial attempt at implementing this broke HTML serializer tests.

Part of the problem here is that I don't have a clear understanding of how the context stuff is supposed to work when serializing to the Windows HTML clipboard format.
(In reply to Henri Sivonen (:hsivonen) from comment #15)
> (In reply to Henri Sivonen (:hsivonen) from comment #13)
> > I think the way to proceed here is to keep a stack of contexts computed by
> > SerializeRangeContextStart and have SerializeRangeContextEnd replay (in
> > reverse) the latest context on the stack and then pop it.
> > 
> > If SerializeRangeContextEnd isn't responsible for computing the context, it
> > can't compute it differently compared to SerializeRangeContextStart.
> 
> My initial attempt at implementing this broke HTML serializer tests.

Or maybe my local mochitest run lost focus in a way that matters. Running the individual test passes.
I feel uneasy about now having a try run for this, but pushing this to try would probably reveal the bug.
Flags: needinfo?(hsivonen)
Attachment #8923721 - Flags: review?(bugs)
Note to self when requesting landing permission: AFAICT, the cause of this bug is an unfortunate interaction between the patch for bug 137450 and the patch for bug 1113238. The patch for bug 1385272 may have contributed, too.
Comment on attachment 8923721 [details] [diff] [review]
When ending a context, use the context that was computed when starting it

>@@ -874,6 +875,9 @@ nsDocumentEncoder::SerializeRangeContextStart(const nsTArray<nsINode*>& aAncesto
>   if (mDisableContextSerialize) {
>     return NS_OK;
>   }
>+
>+  auto serializedContext = mRangeContexts.AppendElement();
Please don't use auto in cases when it isn't totally obvious what the type is.
Now reader needs to go look at mRangeContexts definition to figure out what the type is.


>+nsDocumentEncoder::SerializeRangeContextEnd(nsAString& aString)
> {
>   if (mDisableContextSerialize) {
>     return NS_OK;
>   }
>-  int32_t i = 0, j;
>-  int32_t count = aAncestorArray.Length();
>-  nsresult rv = NS_OK;
> 
>-  // currently only for table-related elements
>-  j = GetImmediateContextCount(aAncestorArray);
>+  MOZ_RELEASE_ASSERT(!mRangeContexts.IsEmpty(), "Tried to end context without starting one.");
>+  auto serializedContext = mRangeContexts.LastElement();
Also here, don't use auto.


>@@ -992,7 +986,7 @@ nsDocumentEncoder::SerializeRangeToString(nsRange *aRange,
>     rv = SerializeRangeNodes(aRange, mCommonParent, aOutputString, 0);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
>-  rv = SerializeRangeContextEnd(mCommonAncestors, aOutputString);
>+  rv = SerializeRangeContextEnd(aOutputString);
>   NS_ENSURE_SUCCESS(rv, rv);
So if serialization fails, mRangeContexts may be non-empty and contains raw pointers to nodes, right?
Or if mDisableContextSerialize is true.
Can those cases happen? Should we rather have some stack object which clears mRangeContexts always.
I'd rather ensure we don't have raw pointers to possibly deleted objects anywhere.
Then we'd not need explicit mRangeContexts.Clear() calls.
Perhaps use ScopeExit to ensure the array is cleared?
Attachment #8923721 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 8923721 [details] [diff] [review]
> When ending a context, use the context that was computed when starting it
> 
> >@@ -874,6 +875,9 @@ nsDocumentEncoder::SerializeRangeContextStart(const nsTArray<nsINode*>& aAncesto
> >   if (mDisableContextSerialize) {
> >     return NS_OK;
> >   }
> >+
> >+  auto serializedContext = mRangeContexts.AppendElement();
> Please don't use auto in cases when it isn't totally obvious what the type
> is.

The purpose of auto is to avoid writing out long types, but OK.

> >@@ -992,7 +986,7 @@ nsDocumentEncoder::SerializeRangeToString(nsRange *aRange,
> >     rv = SerializeRangeNodes(aRange, mCommonParent, aOutputString, 0);
> >     NS_ENSURE_SUCCESS(rv, rv);
> >   }
> >-  rv = SerializeRangeContextEnd(mCommonAncestors, aOutputString);
> >+  rv = SerializeRangeContextEnd(aOutputString);
> >   NS_ENSURE_SUCCESS(rv, rv);
> So if serialization fails, mRangeContexts may be non-empty and contains raw
> pointers to nodes, right?
> Or if mDisableContextSerialize is true.
> Can those cases happen? Should we rather have some stack object which clears
> mRangeContexts always.
> I'd rather ensure we don't have raw pointers to possibly deleted objects
> anywhere.
> Then we'd not need explicit mRangeContexts.Clear() calls.
> Perhaps use ScopeExit to ensure the array is cleared?

I tried to check that there aren't early returns that would bypass the mRangeContexts.Clear() calls, but using RAII makes more sense, indeed.
With review comments addressed and with -U8.
Attachment #8923721 - Attachment is obsolete: true
Attachment #8923850 - Flags: review?(bugs)
Comment on attachment 8923850 [details] [diff] [review]
When ending a context, use the context that was computed when starting it, v2

s/gangeContextGuard/rangeContextGuard/g

This is scary stuff since DocumentEncoder is a bit icky.
Attachment #8923850 - Flags: review?(bugs) → review+
Thanks.

Attaching a patch with the typo fixed. Carrying forward r+.
Attachment #8923850 - Attachment is obsolete: true
Attachment #8923884 - Flags: review+
Comment on attachment 8923884 [details] [diff] [review]
When ending a context, use the context that was computed when starting it, v3

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

Not at all easily.

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

The nature of the problem, yes, but not on how to trigger it.

> Which older supported branches are affected by this flaw?

All.

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

AFAICT, the cause of this bug is an unfortunate interaction between the patch for bug 137450 and the patch for bug 1113238. The patch for bug 1385272 may have contributed, too.

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

I don't have backports, but I expect this patch to apply either directly or with trivial rebasing. Aan uplift is probably risky until the patch has been tested on Nightly as stated below.

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

This could break rich text (HTML) copy and paste. It could benefit from manual testing to copy and paste things such as partial tables (cell ranges selected by holding the ctrl key), numbered lists and stuff like that from Firefox to Microsoft Office on Windows.
Attachment #8923884 - Flags: sec-approval?
I think it is too late to get this into 57. Assuming thast's the case, we should wait to take it on trunk so we don't point an arrow at a sec-high issue. I'm ni? release managers to get their opinion.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
I would also tend to wait till part way through the 58 cycle.  
We have 1 more beta build this week, then the release candidate build next week, so not much time to detect and fix any regressions. Breaking copy/paste would likely be a release blocking issue.
Flags: needinfo?(lhenry)
I'm going to say that this can be checked in on December 1 then, which is roughly two weeks into the next cycle.
Whiteboard: [checkin on 12/1]
Attachment #8923884 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(rkothari)
(In reply to Al Billings [:abillings] from comment #27)
> I'm going to say that this can be checked in on December 1 then, which is
> roughly two weeks into the next cycle.

I'll be traveling on Dec 1, but I'll land this on Dec 4.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0a3a621f5c85b5e9a4eb97de77fcb2558ac17c6

This grafts cleanly to Beta as-landed but needs a rebased patch for ESR52.
Flags: needinfo?(hsivonen)
Whiteboard: [checkin on 12/1]
https://hg.mozilla.org/mozilla-central/rev/b0a3a621f5c8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Hmm. ESR is marked as affected, but the crash is invoked from code that landed in bug 1370737, which wasn't backported to ESR.
This backport to ESR52 makes all the same changes as the trunk patch, except the MOZ_RELEASE_ASSERT from nsPlaintextSerializer.cpp is missing, because the method into which the assertion was added does not exist. (It was added in bug 1370737.)
Flags: needinfo?(hsivonen)
Attachment #8934156 - Flags: review?(bugs)
Attachment #8934156 - Flags: review?(bugs) → review+
Attachment #8934156 - Flags: checkin?
Comment on attachment 8934156 [details] [diff] [review]
Backport to ESR52

You still need to request Beta/ESR52 approval on the relevant patches (sec-approval just covers the initial trunk landing). Once they're approved, the needs-uplift bug queries will find them from there :)
Attachment #8934156 - Flags: checkin?
Comment on attachment 8923884 [details] [diff] [review]
When ending a context, use the context that was computed when starting it, v3

Approval Request Comment
> [Feature/Bug causing the regression]:

Unclear. Might have been bug 1370737 but more likely bug 1113238.

> [User impact if declined]:

Out of bounds memory access.

> [Is this code covered by automated tests?]:

No (in order not to reveal how to trigger the bug).

> [Has the fix been verified in Nightly?]:

Yes.

> [Needs manual test from QE? If yes, steps to reproduce]: 

Load the attached test case in an ASAN build.

> [List of other uplifts needed for the feature/fix]:

None.

> [Is the change risky?]:

Probably not.

> [Why is the change risky/not risky?]:

The patch replaces the assumption that two pieces of code perform the reverse steps with the first piece recording what it has done and the second piece reversing the recording, so it's pretty likely that the second phase does the right thing now.

> [String changes made/needed]:

None.
Attachment #8923884 - Flags: approval-mozilla-beta?
Comment on attachment 8934156 [details] [diff] [review]
Backport to ESR52

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

sec-high

> User impact if declined: 

If bug 1370737 was the cause, none. If bug 1113238 was the cause, out of bounds memory access.

> Fix Landed on Version:

59

> Risk to taking this patch (and alternatives if risky): 

Should be low risk.

> String or UUID changes made by this patch: 

None.
Attachment #8934156 - Flags: approval-mozilla-esr52?
Comment on attachment 8923884 [details] [diff] [review]
When ending a context, use the context that was computed when starting it, v3

Fix a sec-high. Beta58+ & ESR52+.
Attachment #8923884 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8934156 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Group: dom-core-security → core-security-release
Do I need to take more action for ESR?
Flags: needinfo?(ryanvm)
No, we just wanted to wait until we were clear of the dot releases that went out and the All Hands.
Flags: needinfo?(ryanvm)
Managed to reproduce the initial issue on linux64-asan-debug 57.0.3 (20171226083017). I can confirm that linux64-asan-debug esr52.5.3 (20171227194825), linux64-asan-debug 58.0b13 (20171227090910) and linux64-asan-debug 59.0a1 (20171228094453) builds are verified fixed using Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main58+][adv-esr52.6+]
Group: core-security-release
Component: DOM → DOM: Core & HTML
Regressions: 1433073
You need to log in before you can comment on or make changes to this bug.