Bugzilla

Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]

Assignee

Comment 2

•

7 years ago

There's a bug in how eTreeOpStreamEnded is processed in nsHtml5TreeOperation.cpp. Instead of calling aBuilder->DidBuildModel(false); directly, it should set a flag on aBuilder and then nsHtml5TreeOpExecutor::RunFlushLoop() should check for the flag and call DidBuildModel(false); outside the loop at the end of the method if the flag is set.

Flags: needinfo?(hsivonen)

Reporter

Comment 3

•

7 years ago

Attached patch WIP patch (obsolete) — Details — Splinter Review

Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]

Reporter

Comment 4

•

7 years ago

Comment on attachment 8868457 [details] [diff] [review]
WIP patch

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

Hi Henri
I did as your comment 2 said, but it isn't working,
Can you check did I miss anything?

Thanks

Attachment #8868457 - Flags: feedback?(hsivonen)

Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]

Assignee

Comment 5

•

7 years ago

Comment on attachment 8868457 [details] [diff] [review]
WIP patch

This is exactly what I meant. I'm surprised that it doesn't work.

Do you mean the symptoms are the same as before or are there now different adverse symptoms?

Attachment #8868457 - Flags: feedback?(hsivonen) → feedback+

Reporter

Comment 6

•

7 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #5)
> Comment on attachment 8868457 [details] [diff] [review]
> WIP patch
> 
> This is exactly what I meant. I'm surprised that it doesn't work.
> 
> Do you mean the symptoms are the same as before or are there now different
> adverse symptoms?

New differnt problem, now even the test doesn't run, and it seems to be an idle state. Perhaps I need more time to find out the problem, if you have some suggestions feel free to let me know :)

Hsin-Yi Tsai (she/her) [:hsinyi]

Updated

•

7 years ago

Priority: -- → P2

Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]

Reporter

Updated

•

7 years ago

Blocks: 1363629

Ethan Tseng [:ethan]

Comment 7

•

7 years ago

Hi Yoshi,

What's the current status of this bug?
Do we still need to fix it?

Flags: needinfo?(allstars.chh)

Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]

Reporter

Updated

•

7 years ago

Status: NEW → RESOLVED

Closed: 7 years ago

Flags: needinfo?(allstars.chh)

Resolution: --- → WONTFIX

Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]

Assignee

Comment 8

•

7 years ago

Why WONTFIX? This assertion firing is quite concerning and, I think, worth investigating at least on *some* schedule.

Flags: needinfo?(allstars.chh)

Reporter

Comment 9

•

7 years ago

Sorry,I don't know the status, my original plan was to fix bug 1363629, but now since ckerschb and smaug took over that bug and that bug is landed, so I guess somehow this bug is fixed already. Feel free to reopen it if you think it still exists.

Flags: needinfo?(allstars.chh)

Assignee

Comment 10

•

7 years ago

Reopening, since it appears the test case was changed instead of fixing the parser.

Status: RESOLVED → REOPENED

Resolution: WONTFIX → ---

Assignee

Updated

•

7 years ago

Group: core-security

Assignee

Updated

•

7 years ago

Assignee: nobody → hsivonen

Status: REOPENED → ASSIGNED

Assignee

Comment 11

•

7 years ago

Root cause:
document.close() checks for script-createdness of the parser. A parser is not script-created iff it has a stream parser. However, upon EOF, nsHtml5Parser drops its nsHtml5StreamParser before the nsHtml5Parser becomes unreachable from nsHTMLDocument, so a parser that should count as not script-created looks like a script-created parser for a moment. During that moment, async scripts can run if they have been fetched by the time the EOF of the HTML document is seen.

Solution:
We must unlink nsHtml5StreamParser from nsHtml5Parser *after* we've unlinked nsHtml5Parser from nsHTMLDocument.

Assignee

Comment 12

•

7 years ago

Pushed to try with clean-up diversion:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c27a58a10b6a2eb87f935d07af091db51f701b47

(Not really feasible to land a patch like this without a try run. The actual fix is the GetParser()->DropStreamParser(); line moving down.)

Assignee

Comment 13

•

7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c852ba9724b92eb0de2b2e2fcb5a6eedb0fb569c

Assignee

Comment 14

•

7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b25283a874040cd8f429b024e7b4cde7854a88

Assignee

Comment 15

•

7 years ago

Attached patch Drop stream parser later, clean up how DidBuildModel is called (obsolete) — Details — Splinter Review

The real fix is dropping the stream parser after nsContentSink::DidBuildModel. See comment 11.

The way nsHtml5TreeOpExecutor::DidBuildModel is called turned out to be just clean-up, but I left it in the patch as a diversion for people who watch our security landings.

I'll write a follow-up patch with comments and the test case.

Attachment #8868457 - Attachment is obsolete: true

Attachment #8923700 - Flags: review?(bugs)

Assignee

Comment 16

•

7 years ago

Attached patch Drop stream parser later, clean up how DidBuildModel is called, v2 (obsolete) — Details — Splinter Review

This time without trailing whitespace.

Attachment #8923700 - Attachment is obsolete: true

Attachment #8923700 - Flags: review?(bugs)

Attachment #8923702 - Flags: review?(bugs)

Assignee

Comment 17

•

7 years ago

Filed bug 1413101 as a follow-up.

Assignee

Comment 18

•

7 years ago

Attached patch Follow-up patch for later landing: test cases and clarifying comment — Details — Splinter Review

Attachment #8923706 - Flags: review?(bugs)

Assignee

Comment 19

•

7 years ago

Note for the purpose of giving this a security rating:

The assertion that's triggered here normally indicates that the code is about to potentially do the "free" part of use-after-free. In this case, however, it *seems* to me that this happens when the last item of the array potentially about to be freed has already been read, so it *seems* like the "use" part of UAF is averted in this case. However, I'm not 100% sure that there doesn't exist a way to have an actual UAF here under more complex circumstances than I can think about.

AFAICT, this bug does potentially involve what's technically UB (comparing a pointer that no longer points to a valid object to exit the loop that iterated over the array that maybe got freed after its last item was read but before the loop terminated), but in this case the compiler shouldn't be able to know that and do any UB-based transformations.

Updated

•

7 years ago

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

Comment 20

•

7 years ago

in general, it would be easier to read patches if they were created with -U 8.

Comment 21

•

7 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> Created attachment 8923700 [details] [diff] [review]
> Drop stream parser later, clean up how DidBuildModel is called
> 
> The real fix is dropping the stream parser after
> nsContentSink::DidBuildModel. See comment 11.
I don't understand this. drop stream parser after what?

Comment 22

•

7 years ago

Comment on attachment 8923702 [details] [diff] [review]
Drop stream parser later, clean up how DidBuildModel is called, v2

> NS_IMETHODIMP
> nsHtml5TreeOpExecutor::DidBuildModel(bool aTerminated)
> {
>-  if (!aTerminated) {
>-    // This is needed to avoid unblocking loads too many times on one hand
>-    // and on the other hand to avoid destroying the frame constructor from
>-    // within an update batch. See bug 537683.
>-    EndDocUpdate();
Why we can drop this call now?

Comment 23

•

7 years ago

Could you explain the patch a bit.

Flags: needinfo?(hsivonen)

Comment 24

•

7 years ago

Comment on attachment 8923702 [details] [diff] [review]
Drop stream parser later, clean up how DidBuildModel is called, v2

> nsHtml5TreeOpExecutor::DidBuildModel(bool aTerminated)
> {
ok, based on IRC discussion, please MOZ_ASSERT here that we are actually in an update.

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

Assignee

Comment 25

•

7 years ago

Attached patch Drop stream parser later, clean up how DidBuildModel is called, v3 (obsolete) — Details — Splinter Review

Thank you. Added an assertion. Carrying forward r+.

Attachment #8923702 - Attachment is obsolete: true

Flags: needinfo?(hsivonen)

Attachment #8923857 - Flags: review+

Assignee

Comment 26

•

7 years ago

Comment on attachment 8923857 [details] [diff] [review]
Drop stream parser later, clean up how DidBuildModel is called, v3

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

Most of the patch distracts from the real fix, so spotting the real fix is not obvious. It's not clear that there even is an exploit to be constructed. (See comment 19.)

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

No. (They are in the follow-up patch.)

> Which older supported branches are affected by this flaw?

All.

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

Probably goes back to Firefox 4.

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

I have not created backports, but creating a backport for ESR 52 shouldn't be hard. If this patch survives on Nightly for a few days, a backport should not be too risky.

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

It could have subtle problems but it's not likely and if there are problems, they are likely to be in edge cases that wouldn't affect most sites. The fix passes our test suite.

The idea is to land this patch together with the patch for bug 1410848.

Attachment #8923857 - Flags: sec-approval?

Comment 27

•

7 years ago

Talking to Dan, we think this is a sec-moderate issue. As such, it doesn't need sec-approval to go into mozilla-central, so I'm going to clear the request. Just check it into trunk.

Keywords: sec-moderate

Updated

•

7 years ago

Attachment #8923857 - Flags: sec-approval?

Daniel Veditz [:dveditz]

Updated

•

7 years ago

Group: core-security → dom-core-security

Keywords: csectype-uaf

Assignee

Comment 28

•

7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c504828652999e4e6088558937d8b2749ffd566

Assignee

Comment 29

•

7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96453c7e3f1e8d0eb778e75c1268542d7f88bf20

Assignee

Comment 30

•

7 years ago

Attached patch Drop stream parser later, clean up how DidBuildModel is called, v4 — Details — Splinter Review

Fixing the condition of the assertion added after review in response to review comment. (Unlike I claimed on IRC yesterday, the method can be called from within a flush from an update batch pause (i.e. not from within an update batch) when the parser is stopped by JS before the parse is complete, i.e. when aTerminated is true. However, the old removed bit of code was conditional on aTerminated being false, so its removal doesn't fail to account for any situation that wasn't accounted for previously.)

Attachment #8923857 - Attachment is obsolete: true

Attachment #8924116 - Flags: review+

Assignee

Comment 31

•

7 years ago

> wasn't accounted

s/wasn't/was/

Assignee

Comment 32

•

7 years ago

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/388ffddeb4623b932ed67554f2ddc434b1fed8fb(In reply to Al Billings [:abillings] from comment #27)
> Talking to Dan, we think this is a sec-moderate issue. As such, it doesn't
> need sec-approval to go into mozilla-central, so I'm going to clear the
> request. Just check it into trunk.

Thanks. Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/388ffddeb4623b932ed67554f2ddc434b1fed8fb

Any advice on when to land the follow-up patch that adds the test case and a comment that explains the problem?

Flags: needinfo?(abillings)

Daniel Veditz [:dveditz]

Comment 33

•

7 years ago

Adding the in-testsuite? flag to remind us to check in the second part with tests after we ship the fix.

Flags: in-testsuite?

Ryan VanderMeulen [:RyanVM]

Comment 34

•

7 years ago

Henri, we should hold off on checking in tests for a security bug until the fix has been shipped to the public.

Flags: needinfo?(abillings)

Comment 35

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/388ffddeb462

Not clear to me from comment 26 if this is something we want to backport to affected branches or just let ride the trains. Please request approval or set flags to wontfix accordingly :)

Status: ASSIGNED → RESOLVED

Closed: 7 years ago → 7 years ago

status-firefox56: --- → wontfix

status-firefox57: --- → affected

status-firefox58: --- → fixed

status-firefox-esr52: --- → affected

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Daniel Veditz [:dveditz]

Updated

•

7 years ago

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

Sylvestre Ledru [:Sylvestre]

Comment 36

•

7 years ago

Wontfix on all branches!

status-firefox57: affected → wontfix

status-firefox-esr52: affected → wontfix

Assignee

Updated

•

7 years ago

Depends on: 1414490

Assignee

Comment 37

•

7 years ago

This needs a backout (and re-fixing differently) for causing bug 1414490.

Ryan VanderMeulen [:RyanVM]

Assignee

Comment 38

•

7 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #37)
> This needs a backout (and re-fixing differently) for causing bug 1414490.

Nevermind. I'll fix bug 1414490 without backing this out first.

Updated

•

7 years ago

Depends on: 1418000

Matt Wobensmith [:mwobensmith][:matt:]

Updated

•

6 years ago

Whiteboard: [adv-main58+]

Updated

•

6 years ago

Flags: qe-verify-

Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]

Andrew McCreight [:mccr8]

Updated

•

6 years ago

Group: core-security-release

Andrew McCreight [:mccr8]

Comment 39

•

6 years ago

The test case could land now.

Flags: needinfo?(hsivonen)

Natalia Csoregi [:nataliaCs]

Assignee

Comment 40

•

6 years ago

Try run to see if it still works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a84f9b1a58ef956266cfc274087ae6035fbada1

Pulsebot

Comment 41

•

6 years ago

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9623f9b14d93
follow-up - Test cases and clarifying comment. r=smaug.

Comment 42

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/9623f9b14d93