1349138 - Subclasses of nsITransaction shouldn't use raw reference to store EditorBase

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, some subclasses of nsITransaction stores editor with |EditorBase&|. However, it seems that this is dangerous. The XPCOM object is scriptable. Therefore, even after its transaction manager and its editor are destroyed, its undoTransaction() and redoTransaction() can be called.

So, I think that attacker may be able to refer (run) some addresses which were allocated with the editor.


The list:
http://searchfox.org/mozilla-central/search?q=(EditorBase%7CTextEditor%7CHTMLEditor)%26&case=false&regexp=true&path=Transaction

FYI: Attackers need to make victims install XUL add-on to access transaction or transaction manager. If they succeeded to steal transaction manager, they can replace existing editor's transaction manager with destroyed editor's one. Then, execCommand can cause calling nsITransaction.undoTransaction() and nsITransaction.redoTransaction().

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 2

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-moderate, if it requires installing a hostile addon.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for mozilla-central

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Aurora

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Beta

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

These patches make each reference or pointer to editor to RefPtr<EditorBase> (or nsCOMPtr<nsIEditor> in Beta).

Then, adding them into cycle collection and null check at DoTransaction()/UndoTransaction()/RedoTransaction() called for preventing new crash.

Note that DoTransaction() is called when RedoTransaction() is called and it's not overridden by subclass.


By this change, each transaction class and editor refers each other.  Therefore, when EditorBase::PreDestroy() is called, EditorBase needs to cut the cycle for allowing its refcount to become 0.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Aurora

Sorry, there is wrong nullptr check (forgot |!|).

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Beta

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

# Although, I'm not sure if it's worthwhile to uplift individual patches with risk...

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: patch for beta can be applied to esr52.

Comment 13

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

Comment on attachment 8849913 [details] [diff] [review]
Patch for Aurora

Since we aren't aware of any security bugs because of this, I would just land to nightly to ensure we don't get memory leaks because of this change.

Comment 14

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

If the attack requires an addon to be installed, that isn't really attack at all.
But I do agree that it is safer to make the member variables strong.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8849906 [details] [diff] [review]
Patch for mozilla-central

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

I'm not sure how easy to run specific method with after free pointer/reference. If it's easy for attackers, they must realize what this patch fixes immediately if they know relation between nsITransaction, nsITransactionManager and nsIEditor.

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

Not explicitly, but can guess, probably.

Which older supported branches are affected by this flaw?

ESR45, ESR52 too. However, ESR45 needs different patch.

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

I wrote for Aurora, Beta (and ESR52). If it's necessary, I can write it for ESR45. However, each of them are different, not just hand merge. So, could be risky. Before I request review, I took a mistake which doesn't work undo with a transaction. However, it wasn't detected on tryserver with any test. So, this is not covered code by automated tests, sigh.

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

If new |if| conditions are reverted, undo/redo won't work with the transaction. Although, it can be tested manually with rich text editor which supports all command to create the touching transactions.



Anyway, for using this bug, attacker needs to make target users install an addon which keep storing nsITransaction which implementation stores reference or pointer to editor until after the editor is destroyed. Therefore, I think that it's not so easy to attach with this actually. I recommend that this is fixed on only Nightly or at least 52ESR and later.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8849906 [details] [diff] [review]
Patch for mozilla-central

sec-moderate bugs don't need sec approval.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/596faf466bbc07412cfa2277ef4d233c748fc233
Bug 1349138 Edit transactions should store their editor instance with strong reference r=smaug

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/596faf466bbc

but i guess this still need approval requests for aurora/beta

Comment 19

[Ryan VanderMeulen (PTO, back July 27)]

How comfortable are you rebasing this for ESR52 too? Doesn't seem worth the effort for ESR45, though, since it's nearly EOL anyway.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8849913 [details] [diff] [review]
Patch for Aurora

Okay, then, let's uplift them.

I'd like smaug to review this and the other patch because I needed to rewrite the patches due to changed a lot round here.

Comment 21

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

Why do we want this to aurora/beta/esr?

Comment 22

[Ryan VanderMeulen (PTO, back July 27)]

I'm not the one who made the request?

Comment 23

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

Comment on attachment 8849913 [details] [diff] [review]
Patch for Aurora

I still see no reason to land this stuff to branches. Comment 13 is valid as far as I see.

Or is there some particular reason why this should land to branches?

Comment 24

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

Comment on attachment 8849914 [details] [diff] [review]
Patch for Beta

Same here.
I'm more worried about memory leaks than security issues, given that there isn't currently any known cases where web pages could trigger bad things here.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yep, I agree with that. I think that this is not so critical.