XUL runs script under AttributeChanged notifications, when it's explicitly not really OK

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug, {sec-other})

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox40 wontfix, firefox41 wontfix, firefox42 fixed, firefox-esr31 wontfix, firefox-esr38 wontfix, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main42-])

Attachments

(1 attachment)

Stack:

#1  0x0000000103081aec in mozilla::dom::AutoEntryScript::AutoEntryScript (this=0x7fff5fbfa9f8, aGlobalObject=0x12ad56340, aReason=0x107f51ae7 "XPCWrappedJS method call", aIsMainThread=true, aCx=0x0) at ScriptSettings.cpp:549
#2  0x0000000102696ce0 in nsXPCWrappedJSClass::CallMethod (this=0x12b49b560, wrapper=0x12adde980, methodIndex=4, info_=0x115d9ae88, nativeParams=0x7fff5fbfac40) at XPCWrappedJSClass.cpp:967
#3  0x0000000102696be5 in nsXPCWrappedJS::CallMethod (this=0x12adde980, methodIndex=4, info=0x115d9ae88, params=0x7fff5fbfac40) at XPCWrappedJS.cpp:525
#4  0x0000000101743583 in PrepareAndDispatch (self=0x12a2d5900, methodIndex=4, args=0x7fff5fbfad60, gpregs=0x7fff5fbface0, fpregs=0x7fff5fbfad10) at xptcstubs_x86_64_darwin.cpp:122
#5  0x0000000101741f8b in SharedStub () at nsDebug.h:81
#6  0x0000000105047752 in mozilla::dom::XULDocument::Persist (this=0x1285d6000, aElement=0x1285905e0, aNameSpaceID=0, aAttribute=0x11914a6d0) at XULDocument.cpp:1316
#7  0x0000000105046ce8 in mozilla::dom::XULDocument::AttributeChanged (this=0x1285d6000, aDocument=0x1285d6000, aElement=0x1285905e0, aNameSpaceID=0, aAttribute=0x11914a6d0, aModType=2) at XULDocument.cpp:1015
#8  0x000000010504798d in non-virtual thunk to mozilla::dom::XULDocument::AttributeChanged(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int) (this=0x1285d6388, aDocument=0x1285d6000, aElement=0x1285905e0, aNameSpaceID=0, aAttribute=0x11914a6d0, aModType=2) at XULDocument.cpp:1019
#9  0x000000010321b9a5 in nsNodeUtils::AttributeChanged (aElement=0x1285905e0, aNameSpaceID=0, aAttribute=0x11914a6d0, aModType=2) at nsNodeUtils.cpp:141
#10 0x0000000103008df5 in mozilla::dom::Element::SetAttrAndNotify (this=0x1285905e0, aNamespaceID=0, aName=0x11914a6d0, aPrefix=0x0, aOldValue=@0x7fff5fbfb988, aParsedValue=@0x7fff5fbfb958, aModType=2 '\002', aFireMutation=false, aNotify=true, aCallAfterSetAttr=true) at Element.cpp:2322
#11 0x00000001030085fa in mozilla::dom::Element::SetAttr (this=0x1285905e0, aNamespaceID=0, aName=0x11914a6d0, aPrefix=0x0, aValue=@0x7fff5fbfbb08, aNotify=true) at Element.cpp:2194
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8631403 - Flags: review?(peterv) → review?(khuey)
Attachment #8631403 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/091a6a24c8d1

This probably should have had a security rating and/or sec-approval (if needed) before landing. Can we start with a rating at least?
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(bzbarsky)
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
I would call this sec-other.  A malicious or incompetent extension could, with some effort, cause a security problem here, but...

The main reason this is closed, from my point of view, is to avoid drawing attention to the stack in comment 0, which might lead someone investigating it to find other cases where it _is_ a problem.  I haven't had a chance to investigate those yet, because I have yet to get a browser to start with the assertion involved.
Flags: needinfo?(bzbarsky)
Do we need to consider any backports?
I don't think we should worry about backporting this.
Keywords: sec-other
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42-]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.