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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({sec-moderate})

Trunk
mozilla55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [adv-main55+][post-critsmash-triage])

Attachments

(3 attachments, 2 obsolete attachments)

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().
I'm going to mark this sec-moderate, if it requires installing a hostile addon.
Keywords: sec-moderate
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.
Sorry, there is wrong nullptr check (forgot |!|).
Attachment #8849907 - Attachment is obsolete: true
Attachment #8849907 - Flags: review?(bugs)
Attachment #8849913 - Flags: review?(bugs)
# Although, I'm not sure if it's worthwhile to uplift individual patches with risk...
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.
Attachment #8849913 - Flags: review?(bugs)
Attachment #8849914 - Flags: review?(bugs)
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.
Attachment #8849906 - Flags: review?(bugs) → review+
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.
Attachment #8849906 - Flags: sec-approval?
Comment on attachment 8849906 [details] [diff] [review]
Patch for mozilla-central

sec-moderate bugs don't need sec approval.
Attachment #8849906 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/596faf466bbc

but i guess this still need approval requests for aurora/beta
Target Milestone: --- → mozilla55
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
How comfortable are you rebasing this for ESR52 too? Doesn't seem worth the effort for ESR45, though, since it's nearly EOL anyway.
Flags: needinfo?(masayuki)
Group: core-security → core-security-release
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.
Flags: needinfo?(masayuki)
Attachment #8849913 - Flags: review?(bugs)
Why do we want this to aurora/beta/esr?
Flags: needinfo?(ryanvm)
I'm not the one who made the request?
Flags: needinfo?(ryanvm)
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?
Attachment #8849913 - Flags: review?(bugs)
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.
Attachment #8849914 - Flags: review?(bugs)
Whiteboard: [adv-main55+]
Flags: qe-verify-
Whiteboard: [adv-main55+] → [adv-main55+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.