Closed Bug 1330294 Opened 7 years ago Closed 7 years ago

Tab crash for disabled SVG with script tags

Categories

(Core :: DOM: Security, defect, P2)

Product:
Core
Component:
DOM: Security
Version:
52 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: jkt, Assigned: jkt)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

Bug 1330294 - Fixing disabled script tags that cause crashes in disabled SVG nodes
59 bytes, text/x-review-board-request
smaug
: review+
hsivonen
: review+
Details 
As part of 1216893 I have noticed a crash for certain SVG images.

STR:
1. Go to about:config
2. Set svg.disabled to true
3. Browser to https://stuffandnonsense.co.uk/assets/banners/shifter/img/shifter--full.svg

GDB trace:
#0  0x00007ffff6e1775d in nanosleep () at ../sysdeps/unix/syscall-template.S:84
#1  0x00007ffff6e176aa in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55
#2  0x00007fffe80fc30d in ah_crap_handler(int) (signum=11) at /home/jonathan/projects/firefox/toolkit/xre/nsSigHandlers.cpp:103
#3  0x00007fffe80fc358 in child_ah_crap_handler(int) (signum=11) at /home/jonathan/projects/firefox/toolkit/xre/nsSigHandlers.cpp:115
#4  0x00007fffe9a0137d in js::UnixExceptionHandler(int, siginfo_t*, void*) (signum=11, info=0x7fffffffb730, context=0x7fffffffb600) at /home/jonathan/projects/firefox/js/src/ds/MemoryProtectionExceptionHandler.cpp:267
#5  0x00007ffff7bcb390 in <signal handler called> () at /lib/x86_64-linux-gnu/libpthread.so.0
#6  0x00007fffe46891d8 in nsCOMPtr<nsIScriptElement>::operator->() const (this=0x7fffffffbc40) at /home/jonathan/projects/firefox/obj-devedition/dist/include/nsCOMPtr.h:762
#7  0x00007fffe6c5fe38 in nsXMLContentSink::CreateElement(char16_t const**, unsigned int, mozilla::dom::NodeInfo*, unsigned int, nsIContent**, bool*, mozilla::dom::FromParser) (this=
    0x7fffc744f000, aAtts=0x7fffc77bd600, aAttsCount=0, aNodeInfo=0x7fffc74f2e80, aLineNumber=1, aResult=0x7fffffffbce0, aAppendContent=0x7fffffffbccf, aFromParser=mozilla::dom::FROM_PARSER_NETWORK) at /home/jonathan/projects/firefox/dom/xml/nsXMLContentSink.cpp:479
#8  0x00007fffe6c6199e in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, bool) (this=0x7fffc744f000, aName=0x7fffc77f9cc0 u"http://www.w3.org/2000/svg\xffffscript", aAtts=0x7fffc77bd600, aAttsCount=0, aLineNumber=1, aInterruptable=true)
    at /home/jonathan/projects/firefox/dom/xml/nsXMLContentSink.cpp:960
#9  0x00007fffe6c61668 in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int) (this=0x7fffc744f000, aName=0x7fffc77f9cc0 u"http://www.w3.org/2000/svg\xffffscript", aAtts=0x7fffc77bd600, aAttsCount=0, aLineNumber=1) at /home/jonathan/projects/firefox/dom/xml/nsXMLContentSink.cpp:917
#10 0x00007fffe460d8ff in nsExpatDriver::HandleStartElement(char16_t const*, char16_t const**) (this=0x7fffc77ea5c0, aValue=0x7fffc77f9cc0 u"http://www.w3.org/2000/svg\xffffscript", aAtts=0x7fffc77bd600) at /home/jonathan/projects/firefox/parser/htmlparser/nsExpatDriver.cpp:382
#11 0x00007fffe460c7f6 in Driver_HandleStartElement(void*, XML_Char const*, XML_Char const**) (aUserData=0x7fffc77ea5c0, aName=0x7fffc77f9cc0 u"http://www.w3.org/2000/svg\xffffscript", aAtts=0x7fffc77bd600) at /home/jonathan/projects/firefox/parser/htmlparser/nsExpatDriver.cpp:68
#12 0x00007fffe86d8f3d in doContent (parser=0x7fffc77a5400, startTagLevel=0, enc=0x7fffed6050a0 <little2_encoding_ns>, s=0x7fffc74c32a2 "<", end=0x7fffc74c3940 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7fffffffc1e0, haveMore=1 '\001') at /home/jonathan/projects/firefox/parser/expat/lib/xmlparse.c:2442
#13 0x00007fffe86d7fcd in contentProcessor (parser=0x7fffc77a5400, start=0x7fffc748e020 "<", end=0x7fffc74c3940 "\344\344\344", <incomplete sequence \344>, endPtr=0x7fffffffc1e0) at /home/jonathan/projects/firefox/parser/expat/lib/xmlparse.c:2098
#14 0x00007fffe86ddc92 in doProlog (parser=0x7fffc77a5400, enc=0x7fffed6050a0 <little2_encoding_ns>, s=0x7fffc748e020 "<", end=0x7fffc74c3940 "\344\344\344", <incomplete sequence \344>, tok=29, next=0x7fffc748e020 "<", nextPtr=0x7fffffffc1e0, haveMore=1 '\001') at /home/jonathan/projects/firefox/parser/expat/lib/xmlparse.c:4078
#15 0x00007fffe86dd09c in prologProcessor (parser=0x7fffc77a5400, s=0x7fffc748e020 "<", end=0x7fffc74c3940 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7fffffffc1e0) at /home/jonathan/projects/firefox/parser/expat/lib/xmlparse.c:3812
#16 0x00007fffe86dcb06 in prologInitProcessor (parser=0x7fffc77a5400, s=0x7fffc748e020 "<", end=0x7fffc74c3940 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7fffffffc1e0) at /home/jonathan/projects/firefox/parser/expat/lib/xmlparse.c:3629
#17 0x00007fffe86d71f7 in MOZ_XML_Parse (parser=0x7fffc77a5400, s=0x7fffc748e020 "<", len=219424, isFinal=0) at /home/jonathan/projects/firefox/parser/expat/lib/xmlparse.c:1530
#18 0x00007fffe4610112 in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*) (this=0x7fffc77ea5c0, aBuffer=0x7fffc748e020 u"<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"100 50 1300 435\"><style type=\"text/css\">.st0{fill:#D3D9D0;} .st1{fill:#FEEDD1;} .st2{fill:#DAD5E0;} .st3{fill:#A5BC8F;} .st4{fill:#C49E7A;} .st5{fill:#"..., aLength=109712, aIsFinal=false, aConsumed=0x7fffffffc27c) at /home/jonathan/projects/firefox/parser/htmlparser/nsExpatDriver.cpp:1016
#19 0x00007fffe46107bc in nsExpatDriver::ConsumeToken(nsScanner&, bool&) (this=0x7fffc77ea5c0, aScanner=..., aFlushTokens=@0x7fffffffc49a: false) at /home/jonathan/projects/firefox/parser/htmlparser/nsExpatDriver.cpp:1112
#20 0x00007fffe4617347 in nsParser::Tokenize(bool) (this=0x7fffc777dc90, aIsFinalChunk=false) at /home/jonathan/projects/firefox/parser/htmlparser/nsParser.cpp:1945
#21 0x00007fffe461601a in nsParser::ResumeParse(bool, bool, bool) (this=0x7fffc777dc90, allowIteration=true, aIsFinalChunk=false, aCanInterrupt=true) at /home/jonathan/projects/firefox/parser/htmlparser/nsParser.cpp:1463
#22 0x00007fffe4616f14 in nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (this=0x7fffc777dc90, request=0x7fffc7403080, aContext=0x0, pIStream=0x7fffd45561f0, sourceOffset=0, aLength=109712) at /home/jonathan/projects/firefox/parser/htmlparser/nsParser.cpp:1842
#23 0x00007fffe459cad9 in nsDocumentOpenInfo::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (this=0x7fffc77d3f20, request=0x7fffc7403080, aCtxt=0x0, inStr=0x7fffd45561f0, sourceOffset=0, count=109712) at /home/jonathan/projects/firefox/uriloader/base/nsURILoader.cpp:321
#24 0x00007fffe3a161fb in mozilla::net::HttpChannelChild::DoOnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (this=0x7fffc7403000, aRequest=0x7fffc7403080, aContext=0x0, aStream=0x7fffd45561f0, offset=0, count=109712) at /home/jonathan/projects/firefox/netwerk/protocol/http/HttpChannelChild.cpp:807
#25 0x00007fffe3a15ccd in mozilla::net::HttpChannelChild::OnTransportAndData(nsresult const&, nsresult const&, unsigned long, unsigned long const&, unsigned long const&, unsigned int const&, nsCString const&) (this=0x7fffc7403000, channelStatus=@0x7fffd4556160: nsresult::NS_OK, transportStatus=@0x7fffd4556164: -2142568440, progress=109712, progressMax=@0x7fffd4556170: 109712, offset=@0x7fffd4556188: 0, count=@0x7fffd4556190: 109712, data="<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"100 50 1300 435\"><style type=\"text/css\">.st0{fill:#D3D9D0;} .st1{fill:#FEEDD1;} .st2{fill:#DAD5E0;} .st3{fill:#A5BC8F;} .st4{fill:#C49E7A;} .st5{fill:#"...)
    at /home/jonathan/projects/firefox/netwerk/protocol/http/HttpChannelChild.cpp:737
#26 0x00007fffe3a3369a in mozilla::net::TransportAndDataEvent::Run() (this=0x7fffd4556150) at /home/jonathan/projects/firefox/netwerk/protocol/http/HttpChannelChild.cpp:611
#27 0x00007fffe39b4599 in mozilla::net::ChannelEventQueue::RunOrEnqueue(mozilla::net::ChannelEvent*, bool) (this=0x7fffc77e5330, aCallback=0x7fffd4556150, aAssertionWhenNotQueued=false) at /home/jonathan/projects/firefox/obj-devedition/dist/include/mozilla/net/ChannelEventQueue.h:138
#28 0x00007fffe3a1589f in mozilla::net::HttpChannelChild::RecvOnTransportAndData(nsresult const&, nsresult const&, unsigned long const&, unsigned long const&, unsigned long const&, unsigned int const&, nsCString const&) (this=0x7fffc7403000, channelStatus=@0x7fffffffc8d8: nsresult::NS_OK, transportStatus=@0x7fffffffc8dc: -2142568440, progress=@0x7fffffffc8f0: 109712, progressMax=@0x7fffffffc900: 109712, offset=@0x7fffffffc910: 0, count=@0x7fffffffc8e0: 109712, data="<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"100 50 1300 435\"><style type=\"text/css\">.st0{fill:#D3D9D0;} .st1{fill:#FEEDD1;} .st2{fill:#DAD5E0;} .st3{fill:#A5BC8F;} .st4{fill:#C49E7A;} .st5{fill:#"...)
    at /home/jonathan/projects/firefox/netwerk/protocol/http/HttpChannelChild.cpp:641
#29 0x00007fffe3e0168a in mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&) (this=0x7fffc7403000, msg__=...) at /home/jonathan/projects/firefox/obj-devedition/ipc/ipdl/PHttpChannelChild.cpp:702
#30 0x00007fffe4237711 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (this=0x7ffff6925020, msg__=...) at /home/jonathan/projects/firefox/obj-devedition/ipc/ipdl/PContentChild.cpp:5939
#31 0x00007fffe3c91235 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (this=0x7ffff6925118, aMsg=...) at /home/jonathan/projects/firefox/ipc/glue/MessageChannel.cpp:1773
#32 0x00007fffe3c90cc7 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=0x7ffff6925118, aMsg=<unknown type in /home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so, CU 0x23d0d4f, DIE 0x2471e60>) at /home/jonathan/projects/firefox/ipc/glue/MessageChannel.cpp:1708
#33 0x00007fffe3c903e2 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (this=0x7ffff6925118, aTask=...) at /home/jonathan/projects/firefox/ipc/glue/MessageChannel.cpp:1589
#34 0x00007fffe3c90632 in mozilla::ipc::MessageChannel::MessageTask::Run() (this=0x7fffc79b9280) at /home/jonathan/projects/firefox/ipc/glue/MessageChannel.cpp:1614
#35 0x00007fffe346606b in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fffe082a300, aMayWait=false, aResult=0x7fffffffd077) at /home/jonathan/projects/firefox/xpcom/threads/nsThread.cpp:1240
#36 0x00007fffe34db1e1 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fffe082a300, aMayWait=false) at /home/jonathan/projects/firefox/xpcom/glue/nsThreadUtils.cpp:390
#37 0x00007fffe3c9543e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7ffff69d8420, aDelegate=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/glue/MessagePump.cpp:96
#38 0x00007fffe3c95e9e in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7ffff69d8420, aDelegate=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/glue/MessagePump.cpp:301
#39 0x00007fffe3bf96f8 in MessageLoop::RunInternal() (this=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/chromium/src/base/message_loop.cc:238
#40 0x00007fffe3bf967c in MessageLoop::RunHandler() (this=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/chromium/src/base/message_loop.cc:231
#41 0x00007fffe3bf9640 in MessageLoop::Run() (this=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/chromium/src/base/message_loop.cc:211
#42 0x00007fffe6e9d244 in nsBaseAppShell::Run() (this=0x7fffd50d9200) at /home/jonathan/projects/firefox/widget/nsBaseAppShell.cpp:156
#43 0x00007fffe80f79fb in XRE_RunAppShell() () at /home/jonathan/projects/firefox/toolkit/xre/nsEmbedFunctions.cpp:924
#44 0x00007fffe3c95d33 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7ffff69d8420, aDelegate=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/glue/MessagePump.cpp:269
#45 0x00007fffe3bf96f8 in MessageLoop::RunInternal() (this=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/chromium/src/base/message_loop.cc:238
#46 0x00007fffe3bf967c in MessageLoop::RunHandler() (this=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/chromium/src/base/message_loop.cc:231
#47 0x00007fffe3bf9640 in MessageLoop::Run() (this=0x7fffffffd450) at /home/jonathan/projects/firefox/ipc/chromium/src/base/message_loop.cc:211
#48 0x00007fffe80f73eb in XRE_InitChildProcess(int, char**, XREChildData const*) (aArgc=4, aArgv=0x7fffffffd7d8, aChildData=0x7fffffffd650) at /home/jonathan/projects/firefox/toolkit/xre/nsEmbedFunctions.cpp:756
#49 0x0000000000405407 in content_process_main(int, char**) (argc=6, argv=0x7fffffffd7d8) at /home/jonathan/projects/firefox/browser/app/../../ipc/contentproc/plugin-container.cpp:115
#50 0x000000000040601a in main(int, char**, char**) (argc=7, argv=0x7fffffffd7d8, envp=0x7fffffffd818) at /home/jonathan/projects/firefox/browser/app/nsBrowserApp.cpp:429
Assignee: nobody → jkt
This should cover the changes made by Kathy here: https://github.com/guardianproject/tor-browser/commit/357c0f7364d506dcfd7e8dc9a8239d8a38d4feb5
Flags: needinfo?(brade)
Flags: needinfo?(arthuredelstein)
Comment on attachment 8825846 [details]
Bug 1330294 - Fixing disabled script tags that cause crashes in disabled SVG nodes

https://reviewboard.mozilla.org/r/103918/#review104838

::: dom/xml/nsXMLContentSink.cpp:482
(Diff revision 2)
>      ) {
>      nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(content);
> +    if (sele) {
> -    sele->SetScriptLineNumber(aLineNumber);
> +      sele->SetScriptLineNumber(aLineNumber);
> -    sele->SetCreatorParser(GetParser());
> +      sele->SetCreatorParser(GetParser());
> -  }
> +    }

Please add an else branch where you MOZ_ASSERT that SVG is disabled.

::: dom/xml/nsXMLContentSink.cpp:560
(Diff revision 2)
>  
>    if (nodeInfo->Equals(nsGkAtoms::script, kNameSpaceID_XHTML)
>        || nodeInfo->Equals(nsGkAtoms::script, kNameSpaceID_SVG)
>      ) {
>      nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(aContent);
> +    if (!sele) {

Please MOZ_ASSERT that SVG is disabled if this branch is taken.

::: dom/xslt/xslt/txMozillaXMLOutput.cpp:300
(Diff revision 2)
>              element->DoneAddingChildren(true);
>          } else if (element->IsSVGElement(nsGkAtoms::script) ||
>                     element->IsHTMLElement(nsGkAtoms::script)) {
>              nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(element);
> +            if (sele) {
> -            MOZ_ASSERT(sele, "script elements need to implement nsIScriptElement");
> +              MOZ_ASSERT(sele, "script elements need to implement nsIScriptElement");

Please move the assertion into an else branch and make it assert on whether SVG is disabled.

::: parser/html/nsHtml5TreeBuilderCppSupplement.h:857
(Diff revision 2)
>    NS_ASSERTION(aElement, "No element!");
>    if (deepTreeSurrogateParent && currentPtr <= MAX_REFLOW_DEPTH) {
>      deepTreeSurrogateParent = nullptr;
>    }
> -  if (aNamespace == kNameSpaceID_MathML) {
> +  if (aNamespace == kNameSpaceID_MathML ||
> +      (aNamespace == kNameSpaceID_SVG && nsNameSpaceManager::GetInstance()->mSVGDisabled)) {

This code runs off the main thread. I suspect calling `nsNameSpaceManager::GetInstance()->mSVGDisabled)` off the main thread isn't OK. Is it? If not, it's safer to not make this change and let the parser do a bit of work that turns out to be useless when we get to nsHtml5TreeOperation.

::: parser/html/nsHtml5TreeOpExecutor.cpp:645
(Diff revision 2)
>    }
>  
>    NS_ASSERTION(aScriptElement, "No script to run");
>    nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(aScriptElement);
> +  if (!sele) {
> +    return;

Please MOZ_ASSERT that SVG is disabled.

::: parser/html/nsHtml5TreeOperation.cpp:842
(Diff revision 2)
> -      NS_ASSERTION(sele, "Node didn't QI to script.");
> +      if (sele) {
> -      sele->SetScriptLineNumber(mFour.integer);
> +        sele->SetScriptLineNumber(mFour.integer);
> -      sele->FreezeUriAsyncDefer();
> +        sele->FreezeUriAsyncDefer();
> +      } else {
> +        NS_ASSERTION(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Node didn't QI to script, but SVG wasn't disabled.");
> +      }

Please use `MOZ_` assertion variants in new code.
Attachment #8825846 - Flags: review?(hsivonen) → review-
Priority: -- → P2
Whiteboard: [domsecurity-active]
Comment on attachment 8825846 [details]
Bug 1330294 - Fixing disabled script tags that cause crashes in disabled SVG nodes

https://reviewboard.mozilla.org/r/103918/#review105090

::: dom/xml/nsXMLFragmentContentSink.cpp:235
(Diff revision 3)
>         aContent->IsSVGElement(nsGkAtoms::script))) {
>      nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(aContent);
> -    NS_ASSERTION(sele, "script did QI correctly!");
> +    if (sele) {
> -    sele->PreventExecution();
> +      sele->PreventExecution();
> +    } else {
> +      NS_ASSERTION(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Script did QI correctly, but wasn't a disabled SVG!");

Please use MOZ_ASSERT instead of NS_ASSERTION.
Attachment #8825846 - Flags: review?(hsivonen) → review+
Comment on attachment 8825846 [details]
Bug 1330294 - Fixing disabled script tags that cause crashes in disabled SVG nodes

https://reviewboard.mozilla.org/r/103918/#review105586
Attachment #8825846 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Autoland can't push this until all pending issues are marked as resolved in MozReview.
Open issues resolved sorry about that.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b7beea9e064d
Fixing disabled script tags that cause crashes in disabled SVG nodes r=hsivonen,smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b7beea9e064d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Thanks for working on this, Jonathan, Henri, and Olli!
Flags: needinfo?(arthuredelstein)
Since this fix seems to have automated coverage, I don't think manual testing would be of much help here.

Jonathan, if you think Manual QA should instead be looking at this, feel free to flip the qe-verify flag or ni? me directly.
Flags: qe-verify-
I don't think a QA verify is needed, I don't think we test prefs like this. However the STR should no longer be reproducible of a tab crash if followed.
Flags: needinfo?(brade)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: