1425520 - Crash in nsPlainTextSerializer::ForgetElementForPreformat

Status: RESOLVED FIXED

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

Description

[[:philipp]]

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.)

Comment 1

[Henri Sivonen (:hsivonen)]

Could be a security bug on old branches.

Comment 2

[Alphan Chen [:alchen]]

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.

Comment 3

[Alphan Chen [:alchen]]

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;
 }

Comment 4

[Andrew Overholt [:overholt]]

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?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(Trying to find time for this today)

Comment 6

[Daniel Veditz [:dveditz]]

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?

Comment 7

[Alphan Chen [: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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Oops, still keeping needinfo for myself too.

Comment 10

[Henri Sivonen (:hsivonen)]

(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.)

Comment 11

[Henri Sivonen (: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.

Comment 12

[Henri Sivonen (:hsivonen)]

I checked 15 or so crash reports, and they are all about saving a DOM into a stream (as opposed to a clipboard string).

Comment 13

[Gerry Chang [:gchang]]

Hi :smaug & :hsivonen,
Any updates? Is it possible to have fix before releasing 58? Next week is the RC week.

Comment 14

[Henri Sivonen (:hsivonen)]

Without steps to reproduce I don't really know where to start, so having a fix by 58 is unlikely.

Comment 15

[Frederik Braun [:freddy]]

(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?

Comment 16

[Henri Sivonen (: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.

Comment 17

[[:philipp]]

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

Comment 18

[Frederik Braun [:freddy]]

(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!

Comment 19

[Henri Sivonen (: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. :-(

Comment 20

[Henri Sivonen (:hsivonen)]

Attachment: Minimized test case (contains simply <base>)

The crash happens if the page has a <base> element.

Comment 21

[Henri Sivonen (:hsivonen)]

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.

Comment 22

[Henri Sivonen (:hsivonen)]

Attachment: Use the same node for end as for start

Comment 23

[Henri Sivonen (:hsivonen)]

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.

Comment 24

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 26

[Daniel Veditz [:dveditz]]

The patch here looks like it's against nightly, and would apply equally well (with 'fuzz') to beta and ESR-52

Comment 27

[Al Billings [:abillings - ex-MoCo]]

If this is nightly only, that's good (sec-approval+ applies). Let's get Beta and ESR52 patches made and nominated ASAP.

Comment 28

[Henri Sivonen (: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.

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e20052aa341d6da4505eb84be6e061bb7f37de51
https://hg.mozilla.org/releases/mozilla-beta/rev/a330f1d4d12a7c144797f3686f35a99c37b0755e

Comment 30

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Great, this should make it into the 59 RC build, which will go out to the beta channel early next week.

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/e20052aa341d

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/4904c0f4a645