Closed Bug 1349138 Opened 6 years ago Closed 6 years ago

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


(Core :: DOM: Editor, defect)

Not set



Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed


(Reporter: masayuki, Assigned: masayuki)


(Keywords: sec-moderate, Whiteboard: [adv-main55+][post-critsmash-triage])


(3 files, 2 obsolete files)

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:

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
Attached patch Patch for Aurora (obsolete) — Splinter Review
Attachment #8849907 - Flags: review?(bugs)
Attached patch Patch for Beta (obsolete) — Splinter Review
Attachment #8849909 - Flags: review?(bugs)
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.
Attached patch Patch for AuroraSplinter Review
Sorry, there is wrong nullptr check (forgot |!|).
Attachment #8849907 - Attachment is obsolete: true
Attachment #8849907 - Flags: review?(bugs)
Attachment #8849913 - Flags: review?(bugs)
Attached patch Patch for BetaSplinter Review
Attachment #8849909 - Attachment is obsolete: true
Attachment #8849909 - Flags: review?(bugs)
Attachment #8849914 - Flags: review?(bugs)
# Although, I'm not sure if it's worthwhile to uplift individual patches with risk...
FYI: patch for beta can be applied to esr52.
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)
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?
No longer depends on: 1217700
Bug 1349138 Edit transactions should store their editor instance with strong reference r=smaug

but i guess this still need approval requests for aurora/beta
Target Milestone: --- → mozilla55
Closed: 6 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)
Attachment #8849914 - 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)
Yep, I agree with that. I think that this is not so critical.
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.