Closed Bug 1625333 Opened 4 years ago Closed 4 years ago

ASAN crashes (UAF) and debug assertion failures when converting the edit bookmark panel to fluent

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla77
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- fixed

People

(Reporter: Gijs, Assigned: mossop)

References

(Regression)

Details

(Keywords: csectype-uaf, regression, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main77+r])

Attachments

(1 file, 1 obsolete file)

Bug 1625333: Ignore changes to the is attribute made by fluent. r=zbraniecki!
47 bytes, text/x-phabricator-request
dveditz
: sec-approval+
Details | Review

The patch in bug 1624713 got backed out for failures in bc tests caused by assertion failures in debug builds and UAFs in ASAN builds. Marking sec-sensitive because it's a use-after-free, though because it involves the XUL document cache and fluent, it's probably not triggerable by web content. Then again, why run the risk?

So, concretely. I pushed:

https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=294912742&resultStatus=testfailed%2Cbusted%2Cexception&revision=49509cff8c94d3758e0713c26d3ed43a7762570b

Patch at https://phabricator.services.mozilla.com/D68118 .

Debug assertion:

https://treeherder.mozilla.org/logviewer.html#?job_id=294912742&repo=autoland

stack:

Assertion failure: Type() == eAtom (wrong type), at /builds/worker/checkouts/gecko/dom/base/nsAttrValueInlines.h:230
#01: nsXULPrototypeDocument::RebuildL10nPrototype(mozilla::dom::Element*, bool) [dom/xul/nsXULPrototypeDocument.cpp:529]
#02: mozilla::dom::DOMLocalization::ApplyTranslations(nsTArray<nsCOMPtr<mozilla::dom::Element> >&, nsTArray<mozilla::dom::L10nMessage>&, nsXULPrototypeDocument*, mozilla::ErrorResult&) [dom/l10n/DOMLocalization.cpp:500]
#03: ElementTranslationHandler::ResolvedCallback(JSContext*, JS::Handle<JS::Value>) [dom/l10n/DOMLocalization.cpp:232]
#04: mozilla::dom::`anonymous namespace'::PromiseNativeHandlerShim::ResolvedCallback(JSContext*, JS::Handle<JS::Value>) [dom/promise/Promise.cpp:384]
#05: mozilla::dom::NativeHandlerCallback(JSContext*, unsigned int, JS::Value*) [dom/promise/Promise.cpp:336]
#06: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:476]
#07: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:568]
#08: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:631]
#09: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:648]
#10: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.h:103]
#11: PromiseReactionJob(JSContext*, unsigned int, JS::Value*) [js/src/builtin/Promise.cpp:1880]
#12: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:476]
#13: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:568]
#14: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:631]
#15: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:648]
#16: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2798]
#17: mozilla::dom::PromiseJobCallback::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::ErrorResult&) [s3:gecko-generated-sources:fbfdbc97d873d463f9ffcf1f5609d5f3e6a873871fed36c07c628fc0be95758dea1cf5ead3762aaa157d8b13918529228d012695c5e2ea4cc30dde5517a8cfcb/dom/bindings/PromiseBinding.cpp::28]
#18: mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) [s3:gecko-generated-sources:09cbe7f9e1409cd4cca288356b597724157d7f93ab5efbaede65be8bf535e6469c7590bf6c7211a89f760ea37ac901f3d1d5fcbeb89c9dfc80643c98c831255f/dist/include/mozilla/dom/PromiseBinding.h::91]
#19: mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) [xpcom/base/CycleCollectedJSContext.cpp:214]
#20: mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) [xpcom/base/CycleCollectedJSContext.cpp:645]
#21: mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) [xpcom/base/CycleCollectedJSContext.cpp:467]
#22: XPCJSContext::AfterProcessTask(unsigned int) [js/xpconnect/src/XPCJSContext.cpp:1332]
#23: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1247]
#24: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:481]
#25: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:109]
#26: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
#27: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:309]
#28: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
#29: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:139]
#30: nsAppShell::Run() [widget/windows/nsAppShell.cpp:406]
#31: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:272]
#32: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4626]

ASAN crash: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=294916567&repo=autoland&lineNumber=2563

==1235==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000c2d590 at pc 0x7fdeebe84864 bp 0x7ffe696e7380 sp 0x7ffe696e7378
READ of size 4 at 0x604000c2d590 thread T0
    #0 0x7fdeebe84863 in IsStatic /builds/worker/workspace/obj-build/dist/include/nsAtom.h:48:34
    #1 0x7fdeebe84863 in Release /builds/worker/workspace/obj-build/dist/include/nsAtom.h:232:10
    #2 0x7fdeebe84863 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:50:40
    #3 0x7fdeebe84863 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:381:36
    #4 0x7fdeebe84863 in RefPtr<nsAtom>::~RefPtr() /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:81:7
    #5 0x7fdef28eff43 in nsXULPrototypeElement::~nsXULPrototypeElement() /builds/worker/checkouts/gecko/dom/xul/nsXULElement.h:143:48
    #6 0x7fdef28eff9d in nsXULPrototypeElement::~nsXULPrototypeElement() /builds/worker/checkouts/gecko/dom/xul/nsXULElement.h:143:36
    #7 0x7fdeebba64e8 in SnowWhiteKiller::Visit(nsPurpleBuffer&, nsPurpleBufferEntry*) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:2455:9
    #8 0x7fdeebb84dcd in void nsPurpleBuffer::VisitEntries<SnowWhiteKiller>(SnowWhiteKiller&) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:957:27
    #9 0x7fdeebb855f5 in nsCycleCollector::FreeSnowWhiteWithBudget(js::SliceBudget&) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:2620:14
    #10 0x7fdeed6eab6c in AsyncFreeSnowWhite::Run() /builds/worker/checkouts/gecko/js/xpconnect/src/XPCJSRuntime.cpp:148:9
    #11 0x7fdeebd6a3f8 in IdleRunnableWrapper::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:326:22
    #12 0x7fdeebd46570 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1220:14
    #13 0x7fdeebd5104c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:481:10
    #14 0x7fdeecdb9f2a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:87:21
    #15 0x7fdeecce6777 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:315:10
    #16 0x7fdeecce6777 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:308:3
    #17 0x7fdeecce6777 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:290:3
    #18 0x7fdef2d8d128 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #19 0x7fdef632b00b in nsAppStartup::Run() /builds/worker/checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp:271:30
    #20 0x7fdef6533126 in XREMain::XRE_mainRun() /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4626:22
    #21 0x7fdef6534f81 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4761:8
    #22 0x7fdef6535cc3 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4812:21
    #23 0x5622d029848b in do_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:217:22
    #24 0x5622d029848b in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:331:16
    #25 0x7fdf0b666b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #26 0x5622d01ed998 in _start (/builds/worker/workspace/build/application/firefox/firefox+0x9d998)
0x604000c2d590 is located 0 bytes inside of 46-byte region [0x604000c2d590,0x604000c2d5be)
freed by thread T0 here:
    #0 0x5622d026565d in free /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:123:3
    #1 0x7fdeeef96d8a in nsAttrValue::Reset() /builds/worker/checkouts/gecko/dom/base/nsAttrValue.cpp:196:14
    #2 0x7fdef28f2ec8 in ~nsXULPrototypeAttribute /builds/worker/checkouts/gecko/dom/xul/nsXULElement.cpp:1218:1
    #3 0x7fdef28f2ec8 in Destruct /builds/worker/workspace/obj-build/dist/include/nsTArray.h:648:45
    #4 0x7fdef28f2ec8 in nsTArray_Impl<nsXULPrototypeAttribute, nsTArrayInfallibleAllocator>::DestructRange(unsigned long, unsigned long) /builds/worker/workspace/obj-build/dist/include/nsTArray.h:2382:7
    #5 0x7fdef28e3806 in ClearAndRetainStorage /builds/worker/workspace/obj-build/dist/include/nsTArray.h:1463:5
    #6 0x7fdef28e3806 in Clear /builds/worker/workspace/obj-build/dist/include/nsTArray.h:1963:5
    #7 0x7fdef28e3806 in nsXULPrototypeElement::Unlink() /builds/worker/checkouts/gecko/dom/xul/nsXULElement.cpp:1533:15
    #8 0x7fdef28eff3a in nsXULPrototypeElement::~nsXULPrototypeElement() /builds/worker/checkouts/gecko/dom/xul/nsXULElement.h:143:38
    #9 0x7fdef28eff9d in nsXULPrototypeElement::~nsXULPrototypeElement() /builds/worker/checkouts/gecko/dom/xul/nsXULElement.h:143:36
    #10 0x7fdeebba64e8 in SnowWhiteKiller::Visit(nsPurpleBuffer&, nsPurpleBufferEntry*) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:2455:9
    #11 0x7fdeebb84dcd in void nsPurpleBuffer::VisitEntries<SnowWhiteKiller>(SnowWhiteKiller&) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:957:27
    #12 0x7fdeebb855f5 in nsCycleCollector::FreeSnowWhiteWithBudget(js::SliceBudget&) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:2620:14
    #13 0x7fdeed6eab6c in AsyncFreeSnowWhite::Run() /builds/worker/checkouts/gecko/js/xpconnect/src/XPCJSRuntime.cpp:148:9
    #14 0x7fdeebd6a3f8 in IdleRunnableWrapper::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:326:22
    #15 0x7fdeebd46570 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1220:14
    #16 0x7fdeebd5104c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:481:10
    #17 0x7fdeecdb9f2a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:87:21
    #18 0x7fdeecce6777 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:315:10
    #19 0x7fdeecce6777 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:308:3
    #20 0x7fdeecce6777 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:290:3
    #21 0x7fdef2d8d128 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #22 0x7fdef632b00b in nsAppStartup::Run() /builds/worker/checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp:271:30
    #23 0x7fdef6533126 in XREMain::XRE_mainRun() /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4626:22
    #24 0x7fdef6534f81 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4761:8
    #25 0x7fdef6535cc3 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4812:21
    #26 0x5622d029848b in do_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:217:22
    #27 0x5622d029848b in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:331:16
    #28 0x7fdf0b666b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
previously allocated by thread T0 here:
    #0 0x5622d02658dd in malloc /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145:3
    #1 0x7fdeebb06d35 in nsStringBuffer::Alloc(unsigned long) /builds/worker/checkouts/gecko/xpcom/string/nsSubstring.cpp:201:42
    #2 0x7fdeeef9788e in nsAttrValue::GetStringBuffer(nsTSubstring<char16_t> const&) const /builds/worker/checkouts/gecko/dom/base/nsAttrValue.cpp:1827:9
    #3 0x7fdeeef96795 in nsAttrValue::SetTo(nsTSubstring<char16_t> const&) /builds/worker/checkouts/gecko/dom/base/nsAttrValue.cpp:340:25
    #4 0x7fdeeee1f757 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) /builds/worker/checkouts/gecko/dom/base/Element.cpp:2208:15
    #5 0x7fdef231394d in SetAttr /builds/worker/workspace/obj-build/dist/include/mozilla/dom/Element.h:828:12
    #6 0x7fdef231394d in mozilla::dom::PrototypeDocumentContentSink::AddAttributes(nsXULPrototypeElement*, mozilla::dom::Element*) /builds/worker/checkouts/gecko/dom/prototype/PrototypeDocumentContentSink.cpp:1075:20
    #7 0x7fdef230e20e in mozilla::dom::PrototypeDocumentContentSink::CreateElementFromPrototype(nsXULPrototypeElement*, mozilla::dom::Element**, bool) /builds/worker/checkouts/gecko/dom/prototype/PrototypeDocumentContentSink.cpp:1044:10
    #8 0x7fdef230f6e8 in mozilla::dom::PrototypeDocumentContentSink::ResumeWalkInternal() /builds/worker/checkouts/gecko/dom/prototype/PrototypeDocumentContentSink.cpp:499:16
    #9 0x7fdef230d7aa in mozilla::dom::PrototypeDocumentContentSink::ResumeWalk() /builds/worker/checkouts/gecko/dom/prototype/PrototypeDocumentContentSink.cpp:411:17
    #10 0x7fdef231334e in mozilla::dom::PrototypeDocumentContentSink::OnScriptCompileComplete(JSScript*, nsresult) /builds/worker/checkouts/gecko/dom/prototype/PrototypeDocumentContentSink.cpp:920:8
    #11 0x7fdef2312c9e in mozilla::dom::PrototypeDocumentContentSink::OnStreamComplete(nsIStreamLoader*, nsISupports*, nsresult, unsigned int, unsigned char const*) /builds/worker/checkouts/gecko/dom/prototype/PrototypeDocumentContentSink.cpp:849:10
    #12 0x7fdeec0993a7 in mozilla::net::nsStreamLoader::OnStopRequest(nsIRequest*, nsresult) /builds/worker/checkouts/gecko/netwerk/base/nsStreamLoader.cpp:89:20
    #13 0x7fdeed76c049 in nsJARChannel::OnStopRequest(nsIRequest*, nsresult) /builds/worker/checkouts/gecko/modules/libjar/nsJARChannel.cpp:1040:16
    #14 0x7fdeed76f6bc in non-virtual thunk to nsJARChannel::OnStopRequest(nsIRequest*, nsresult) /builds/worker/checkouts/gecko/modules/libjar/nsJARChannel.cpp
    #15 0x7fdeebfc17f7 in nsInputStreamPump::OnStateStop() /builds/worker/checkouts/gecko/netwerk/base/nsInputStreamPump.cpp:686:16
    #16 0x7fdeebfbfd0a in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /builds/worker/checkouts/gecko/netwerk/base/nsInputStreamPump.cpp:434:21
    #17 0x7fdeebca23a6 in nsInputStreamReadyEvent::Run() /builds/worker/checkouts/gecko/xpcom/io/nsStreamUtils.cpp:93:20
    #18 0x7fdeebd46570 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1220:14
    #19 0x7fdeebd5104c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:481:10
    #20 0x7fdeecdb9f1f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:109:5
    #21 0x7fdeecce6777 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:315:10
    #22 0x7fdeecce6777 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:308:3
    #23 0x7fdeecce6777 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:290:3
    #24 0x7fdef2d8d128 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #25 0x7fdef632b00b in nsAppStartup::Run() /builds/worker/checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp:271:30
    #26 0x7fdef6533126 in XREMain::XRE_mainRun() /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4626:22
    #27 0x7fdef6534f81 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4761:8
    #28 0x7fdef6535cc3 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4812:21
    #29 0x5622d029848b in do_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:217:22
    #30 0x5622d029848b in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:331:16
    #31 0x7fdf0b666b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/obj-build/dist/include/nsAtom.h:48:34 in IsStatic

Dave, are you in a position to take a look at what's going on here?

(In reply to :Gijs (he/him) from comment #0)

Dave, are you in a position to take a look at what's going on here?

I seem to have not succeeded in needinfo'ing Mossop.

Flags: needinfo?(dtownsend)

We treat the is attribute specially in prototype elements and XUL elements. In both cases we hold the attribute's value as an atom. So when we rebuild the prototype after a localisation pass we store the attribute's atom (https://searchfox.org/mozilla-central/rev/8526066f548af9ec3ebb462ff73c47ccc183f533/dom/xul/nsXULPrototypeDocument.cpp#477). But, for non-XUL elements the is attribute is just held as a string (should this change?) and so in this case the function we call to get the attribute's atom is casting a pointer to a string buffer to a pointer to an nsAtom (https://searchfox.org/mozilla-central/source/dom/base/nsAttrValueInlines.h#231). From here on bad things happen as you might expect.

This only happens with a non-XUL element with an is attribute and a data-l10n-id attribute in a document that is eligible for storing in the XUL cache. I couldn't see any other cases in tree currently and in any case the assertion in GetAtomValue will cause debug builds to crash in the presence of this case so since we aren't seeing that in tests I think it's safe to say that the patch in bug 1624713 is adding the first case and so this probably doesn't need to be considered a security issue.

Flags: needinfo?(dtownsend)

In prototype elements we hold the value of an is attribute as an atom and when
creating a XUL element from a prototype its is attribute is also held as an atom.
However other element types simply hold the value as a string. When rebuilding
the prototype after a localization pass we need to atomize the is attribute
if necessary.

Also clears the is attribute from the prototype when rebuilding, though it
seems very unlikely that a localization pass will change the is attribute.

Assignee: nobody → dtownsend
Status: NEW → ASSIGNED

(In reply to Dave Townsend [:mossop] (he/him) from comment #2)

This only happens with a non-XUL element with an is attribute and a data-l10n-id attribute in a document that is eligible for storing in the XUL cache ... I think it's safe to say that the patch in bug 1624713 is adding the first case and so this probably doesn't need to be considered a security issue.

For clarity's sake, which element is this? I don't see something obvious in the patch... the fluent conversion doesn't normally change node types nor does it add is attributes?

Flags: needinfo?(dtownsend)

(In reply to :Gijs (he/him) from comment #4)

(In reply to Dave Townsend [:mossop] (he/him) from comment #2)

This only happens with a non-XUL element with an is attribute and a data-l10n-id attribute in a document that is eligible for storing in the XUL cache ... I think it's safe to say that the patch in bug 1624713 is adding the first case and so this probably doesn't need to be considered a security issue.

For clarity's sake, which element is this? I don't see something obvious in the patch... the fluent conversion doesn't normally change node types nor does it add is attributes?

It is adding data-l10n-id to <html:input id="editBMPanel_tagsField" is="autocomplete-input"/>

Flags: needinfo?(dtownsend)

XUL elements in general don't have is attribute stored as atom, since well, 'is' isn't in stored on elements.

(In reply to Olli Pettay [:smaug] from comment #6)

XUL elements in general don't have is attribute stored as atom, since well, 'is' isn't in stored on elements.

Can you explain this further? Not sure I follow.

Flags: needinfo?(bugs)

"is" isn't stored in general as an attribute, but rather an internal slot.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08ae7c356bbe

Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #8)

"is" isn't stored in general as an attribute, but rather an internal slot.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08ae7c356bbe

Huh interesting, I missed that. That means that what this code is doing is more broken.

(In reply to Dave Townsend [:mossop] (he/him) from comment #9)

(In reply to Olli Pettay [:smaug] from comment #8)

"is" isn't stored in general as an attribute, but rather an internal slot.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08ae7c356bbe

Huh interesting, I missed that. That means that what this code is doing is more broken.

Oh, looks like it is still stored as an attribute, but as the mIsAtom as well: https://searchfox.org/mozilla-central/rev/064b0f9501ad76802853b43f18e33d8713fd54d3/dom/xul/nsXULElement.cpp#1466,1487

Per spec (https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements-core-concepts:attr-is)
changing an elements is attribute does not affect its behaviour so we don't need
to update the prototype is atom after an element has been localized.

Attachment #9136444 - Attachment is obsolete: true

Comment on attachment 9138925 [details]
Bug 1625333: Ignore changes to the is attribute made by fluent. r=zbraniecki!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Per comment 2, extremely difficult, you'd have to make modifications to a chrome document before it has been loaded and cached in the main process.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: Release, Beta
  • If not all supported branches, which bug introduced the flaw?: Bug 1517880
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Trivial.
  • How likely is this patch to cause regressions; how much testing does it need?: Extremely unlikely.
Attachment #9138925 - Flags: sec-approval?
Has Regression Range: --- → yes

Comment on attachment 9138925 [details]
Bug 1625333: Ignore changes to the is attribute made by fluent. r=zbraniecki!

sec-approval+

In general you don't need sec-approval for sec-moderate or sec-low bugs unless something about them makes you nervous and you want a second opinion. See https://wiki.mozilla.org/Security/Bug_Approval_Process

Attachment #9138925 - Flags: sec-approval? → sec-approval+

https://hg.mozilla.org/integration/autoland/rev/801142fddec6c9feeeebf6f81d57715033b312a9
https://hg.mozilla.org/mozilla-central/rev/801142fddec6

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox77: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

Please nominate this for Beta approval when you get a chance.

status-firefox75: affected → wontfix
Flags: needinfo?(dtownsend)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)

Please nominate this for Beta approval when you get a chance.

Given that this is practically unexploitable is it worth uplifting?

We're early in the Beta cycle and the fix seems pretty simple, so it doesn't seem particularly scary. But we can wontfix if you don't think it's worth it.

I guess that's a yes to wontfixing.

status-firefox76: affected → wontfix
Flags: needinfo?(dtownsend)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main77+r]
Group: core-security-release
Regressions: 1659949
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: