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

Reporter

Description

•

5 years ago

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?

:Gijs (he/him)

Reporter

Comment 1

•

5 years ago

(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)

Dave Townsend [:mossop]

Assignee

Comment 2

•

5 years ago

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)

Dave Townsend [:mossop]

Assignee

Comment 3

•

5 years ago

Attached file Bug 1625333: Don't assume that an element's `is` attribute is always atomized. r=smaug! (obsolete) (deleted) — Details

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.

Phabricator Automation

Updated

•

5 years ago

Assignee: nobody → dtownsend

Status: NEW → ASSIGNED

:Gijs (he/him)

Reporter

Comment 4

•

5 years ago

(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)

Dave Townsend [:mossop]

Assignee

Comment 5

•

5 years ago

(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)

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 6

•

5 years ago

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

Daniel Veditz [:dveditz]

Updated

•

5 years ago

Keywords: csectype-uaf, sec-moderate

Dave Townsend [:mossop]

Assignee

Comment 7

•

5 years ago

(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)

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 8

•

5 years ago

"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)

Dave Townsend [:mossop]

Assignee

Comment 9

•

5 years ago

(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.

Dave Townsend [:mossop]

Assignee

Comment 10

•

5 years ago

(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

Dave Townsend [:mossop]

Assignee

Comment 11

•

5 years ago

Attached file Bug 1625333: Ignore changes to the is attribute made by fluent. r=zbraniecki! (deleted) — Details

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.

Phabricator Automation

Updated

•

5 years ago

Attachment #9136444 - Attachment is obsolete: true

Dave Townsend [:mossop]

Assignee

Comment 12

•

5 years ago

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?

Daniel Veditz [:dveditz]

Updated

•

5 years ago

status-firefox74: --- → wontfix

status-firefox75: --- → affected

status-firefox77: --- → affected

status-firefox-esr68: --- → unaffected

Keywords: regression

Regressed by: 1517880

BMO Automation

Updated

•

5 years ago

Has Regression Range: --- → yes

Daniel Veditz [:dveditz]

Comment 13

•

5 years ago

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+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 14

•

5 years ago

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: 5 years ago

status-firefox77: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla77

Ryan VanderMeulen [:RyanVM]

Comment 15

•

5 years ago

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

status-firefox75: affected → wontfix

Flags: needinfo?(dtownsend)

Dave Townsend [:mossop]

Assignee

Comment 16

•

5 years ago

(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?

Ryan VanderMeulen [:RyanVM]

Comment 17

•

5 years ago

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.

Ryan VanderMeulen [:RyanVM]

Comment 18

•

5 years ago

I guess that's a yes to wontfixing.

status-firefox76: affected → wontfix

Flags: needinfo?(dtownsend)

Brindusa Tot, Desktop QA

Updated

•

5 years ago

Flags: qe-verify-

Whiteboard: [post-critsmash-triage]

Frederik Braun [:freddy]

Updated

•

4 years ago

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main77+r]

Daniel Veditz [:dveditz]

Updated

•

4 years ago

Group: core-security-release

Emil Ghitta, QA [:emilghitta]

Updated

•

4 years ago

Regressions: 1659949

Bug 1625333: Don't assume that an element's `is` attribute is always atomized. r=smaug! 5 years ago Dave Townsend [:mossop] (deleted), text/x-phabricator-request		Details
Bug 1625333: Ignore changes to the is attribute made by fluent. r=zbraniecki! 5 years ago Dave Townsend [:mossop] (deleted), text/x-phabricator-request	dveditz : sec-approval+	Details