Closed Bug 1425520 Opened 2 years ago Closed 2 years ago

Crash in nsPlainTextSerializer::ForgetElementForPreformat

Categories

(Core :: Serializers, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 59+ fixed
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 + fixed
firefox60 + fixed

People

(Reporter: philipp, Assigned: hsivonen)

References

(Regressed 1 open bug)

Details

(4 keywords, Whiteboard: [safe crash on 58 and later][adv-main59+][adv-esr52.7+])

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is
report bp-197fe104-2455-48cb-b07d-55a400171214.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsPlainTextSerializer::ForgetElementForPreformat dom/base/nsPlainTextSerializer.cpp:391
1 xul.dll nsDocumentEncoder::SerializeNodeEnd dom/base/nsDocumentEncoder.cpp:450
2 xul.dll nsDocumentEncoder::SerializeToStringRecursive dom/base/nsDocumentEncoder.cpp:523
3 xul.dll nsDocumentEncoder::SerializeToStringRecursive dom/base/nsDocumentEncoder.cpp:518
4 xul.dll nsDocumentEncoder::EncodeToStringWithMaxLength dom/base/nsDocumentEncoder.cpp:1160
5 xul.dll nsDocumentEncoder::EncodeToStream dom/base/nsDocumentEncoder.cpp:1216
6 xul.dll mozilla::WebBrowserPersistLocalDocument::WriteContent dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp:1361
7 xul.dll mozilla::WebBrowserPersistDocumentChild::RecvPWebBrowserPersistSerializeConstructor dom/webbrowserpersist/WebBrowserPersistDocumentChild.cpp:137
8 xul.dll mozilla::PWebBrowserPersistDocumentChild::OnMessageReceived ipc/ipdl/PWebBrowserPersistDocumentChild.cpp:309
9 xul.dll mozilla::dom::PContentChild::OnMessageReceived ipc/ipdl/PContentChild.cpp:4896

=============================================================

this crash is regressing in 58.0b10 & 59.0a1 with MOZ_RELEASE_ASSERT(!mPreformatStack.empty()) (Tried to pop without previous push.)
Flags: needinfo?(hsivonen)
Group: core-security
Flags: needinfo?(hsivonen)
Could be a security bug on old branches.
Group: core-security → dom-core-security
Priority: -- → P1
The assertion is added in Bug 1409951(Dec 1).
So even if we have the problem before, we cannot meet this symptom before Dec 1.

For mPreformatStack, the related code is changed on bug 1370737(Jun 12).
We may have the problem after this patch.
For manipulating mPreformatStack,they are slightly different before landing the patch of bug 1370737.
Don't know if we should also add "isContainer" checking into the function of ScanElementForPreformat and ForgetElementForPreformat.

BZ reviewed the patch of bug 1370737 and bug 1113238(mPreformatStack first added).
He may know more about this.


 NS_IMETHODIMP
 nsPlainTextSerializer::ScanElementForPreformat(Element* aElement)
 {
-  mPreformatStack.push(IsElementPreformatted(aElement));
+  nsAtom* id = GetIdForContent(aElement);
+  bool isContainer = !FragmentOrElement::IsHTMLVoid(id);
+
+  if (isContainer) {
+    mPreformatStack.push(IsElementPreformatted(aElement));
+  }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPlainTextSerializer::ForgetElementForPreformat(Element* aElement)
 {
-  MOZ_RELEASE_ASSERT(!mPreformatStack.empty(), "Tried to pop without previous push.");
-  mPreformatStack.pop();
+  nsAtom* id = GetIdForContent(aElement);
+  bool isContainer = !FragmentOrElement::IsHTMLVoid(id);
+
+  if (isContainer) {
+    MOZ_RELEASE_ASSERT(!mPreformatStack.empty(), "Tried to pop without previous push.");
+    mPreformatStack.pop();
+  }
+
   return NS_OK;
 }
smaug reviewed bug 1409951 which added the assertion here. Can you take a quick look and see if there's anything to be done before Henri gets back?
Flags: needinfo?(bugs)
(Trying to find time for this today)
So is this a case where this is essentially a dupe of bug 1409951 except that fix (in 58+ESR) turned it into a safe crash? Or do you think this a case where the assert is catching another problem that might be exploitable in slightly different conditions?
Flags: needinfo?(alchen)
(In reply to Daniel Veditz [:dveditz] from comment #6)
> So is this a case where this is essentially a dupe of bug 1409951 except
> that fix (in 58+ESR) turned it into a safe crash? Or do you think this a
> case where the assert is catching another problem that might be exploitable
> in slightly different conditions?

I don't have access right for bug 1409951.
Flags: needinfo?(alchen)
Duplicate of this bug: 1426921
Flags: needinfo?(bugs) → needinfo?(hsivonen)
Oops, still keeping needinfo for myself too.
Flags: needinfo?(bugs)
(In reply to Daniel Veditz [:dveditz] from comment #6)
> So is this a case where this is essentially a dupe of bug 1409951 except
> that fix (in 58+ESR) turned it into a safe crash? Or do you think this a
> case where the assert is catching another problem that might be exploitable
> in slightly different conditions?

There evidently is a variety of ways to cause the kind of push/pop imbalance that bug 1409951 turned into a safe crash on 58+. All root causes of push/pop imbalance should now result in a safe crash on 58+.

However, the backport of bug 1409951 to ESR didn't add the safe crash on push/pop imbalance, so remaining ways to cause push/pop imbalance could still potentially be dangerous on ESR. (Hence comment 1.)
Flags: needinfo?(hsivonen)
Marking the tracking flags as claiming that ESR and 57 are affected. Obviously, they can't crash with the exactly matching signature, but it's more likely than not that whatever root cause it at work here applies to them, too.
I checked 15 or so crash reports, and they are all about saving a DOM into a stream (as opposed to a clipboard string).
Hi :smaug & :hsivonen,
Any updates? Is it possible to have fix before releasing 58? Next week is the RC week.
Flags: needinfo?(hsivonen)
Without steps to reproduce I don't really know where to start, so having a fix by 58 is unlikely.
Flags: needinfo?(hsivonen)
(In reply to Henri Sivonen (:hsivonen) from comment #12)
> I checked 15 or so crash reports, and they are all about saving a DOM into a
> stream (as opposed to a clipboard string).

(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Without steps to reproduce I don't really know where to start, so having a
> fix by 58 is unlikely.



Crash report comments say that users tried to save the web page to disk, many of them containing URLs. Did you look into those pages?
Flags: needinfo?(hsivonen)
(In reply to Frederik Braun [:freddyb] from comment #15)
> Crash report comments say that users tried to save the web page to disk,
> many of them containing URLs. Did you look into those pages?

I think Socorro doesn't let me see that info. I did try to save pages to the disk but didn't manage to trigger the bug.
Flags: needinfo?(hsivonen)
i can reproduce the crash when attempting to save https://www.raamoprusland.nl/dossiers/kremlin/civil-society/847 as .txt (a site that got mentioned in one of the comments)

bp-c15cdf3a-ff59-469f-b28d-793060180209
(In reply to [:philipp] from comment #17)
> i can reproduce the crash when attempting to save
> https://www.raamoprusland.nl/dossiers/kremlin/civil-society/847 as .txt (a
> site that got mentioned in one of the comments)
> 
> bp-c15cdf3a-ff59-469f-b28d-793060180209

Henri, at the risk of being annoying, I'd like to ask you to look into this rather timely, while this is reproducible.

For phillipp: Could you use an ASAN build and attach the output next time you manage to reproduce things that are hard to track down? Thank you!
Flags: needinfo?(hsivonen)
(In reply to Frederik Braun [:freddyb] from comment #18)
> (In reply to [:philipp] from comment #17)
> > i can reproduce the crash when attempting to save
> > https://www.raamoprusland.nl/dossiers/kremlin/civil-society/847 as .txt (a
> > site that got mentioned in one of the comments)

Thank you. I was able to reproduce with this case. The mismatched pop is "html", which means that something somewhere on the whole page got out of sync...

> Henri, at the risk of being annoying, I'd like to ask you to look into this
> rather timely, while this is reproducible.

I have an rr trace of this now. However, I'm about to go away for two weeks, so chances are I won't get this figured out before I go. :-(
Flags: needinfo?(hsivonen)
The crash happens if the page has a <base> element.
Whiteboard: [safe crash on 58 and later]
We replace the <base> with a comment when processing element start:
https://searchfox.org/mozilla-central/source/dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp#910

But then we process element end on the element node instead of the comment node.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
Attachment #8954709 - Flags: review?(bugs)
Attachment #8954709 - Flags: review?(bugs) → review+
Comment on attachment 8954709 [details] [diff] [review]
Use the same node for end as for start

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

It's pretty easy to figure out from the patch that <base> had buggy behavior. Unclear if it's easy or even possible to exploit the bug.

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

No, but it's pretty easy to figure out it's related to serializing <base>. (But not clear that it's about plain text serialization.)

> Which older supported branches are affected by this flaw?

All have have a bug, but it's a safe crash on 58 and above, so this is a potential security bug only in ESR. Therefore, check-in to trunk needs to be considered from the security approval angle only in terms of ESR-related disclosure.

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

The combination of bug 1113238 with ancient questionable code.

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

The same fix should apply.

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

Should be unlikely, but then this area has been quite a chain of regressions already. Looking at what "fix-ups" we can do, the one applying to <base> is the only one that changes the node type.
Attachment #8954709 - Flags: sec-approval?
If we can come up with a fix for the crash in 59 it might be still worth pursuing in a follow up bug, since it looks like a high volume crash (even if safe) on 58.
Sec-approval+ for ESR52 (though this really needs an ESR nomination technically).
Liz points out that even though it is "safe", this crash is pretty high volume on 58. Ideally, we'd fix it on 59 and trunk so we don't have the crash.
Attachment #8954709 - Flags: sec-approval? → sec-approval+
The patch here looks like it's against nightly, and would apply equally well (with 'fuzz') to beta and ESR-52
If this is nightly only, that's good (sec-approval+ applies). Let's get Beta and ESR52 patches made and nominated ASAP.
Flags: needinfo?(hsivonen)
Comment on attachment 8954709 [details] [diff] [review]
Use the same node for end as for start

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

sec-high

> User impact if declined: 

Memory-unsafety when saving a document that contains a <base> element as plain text.

> Fix Landed on Version:

60

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

Should be low risk, but then this area has been quite a chain of regressions already. Looking at what "fix-ups" we can do, the one applying to <base> is the only one that changes the node type.

> String or UUID changes made by this patch: 

None.

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

The combination of bug 1113238 with ancient questionable code. Bug 1426921 ensured it crashes safely, which resulted in the signature reported here.

> [User impact if declined]:

Content process crashes when saving a document as plain text if the document contains <base>.

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

No (to avoid pointing out the exact problem in the ESR context where this isn't a safe crash).

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

Not yet.

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

Open attachment 8949705 [details]. Choose Save As... from the File menu. Change the file type to Text Files. Save.

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

None.

> [Is the change risky?]:

Should be low risk.

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

There is some risk, because this area has been quite a chain of regressions already. Looking at what "fix-ups" we can do, the one applying to <base> is the only one that changes the node type, which should limit the risk.

> [String changes made/needed]:

None.
Flags: needinfo?(hsivonen)
Attachment #8954709 - Flags: approval-mozilla-esr52?
Attachment #8954709 - Flags: approval-mozilla-beta?
Attachment #8954709 - Flags: approval-mozilla-esr52?
Attachment #8954709 - Flags: approval-mozilla-esr52+
Attachment #8954709 - Flags: approval-mozilla-beta?
Attachment #8954709 - Flags: approval-mozilla-beta+
Great, this should make it into the 59 RC build, which will go out to the beta channel early next week.
https://hg.mozilla.org/mozilla-central/rev/e20052aa341d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Whiteboard: [safe crash on 58 and later] → [safe crash on 58 and later][adv-main59+][adv-esr52.7+]
Group: dom-core-security → core-security-release
Group: core-security-release
Regressions: 1570799
You need to log in before you can comment on or make changes to this bug.