258511 - Preference to disable SVG (for security reasons).

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Doron Rosenberg (IBM)]

When XSLT landed, we had a preference to disable it in case a major security
hole was found.  Might be usefull for SVG when it gets build by default for the
first major milestone that uses it (1.8, 1.9, whatever).

Comment 1

[Jonathan Watt [:jwatt]]

Taking and increasing severity since this blocks SVG from being turned on by
default. 

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: rough draft

Okay, this is my work in progress. Stopping SVG documents from being created
isn't quite good enough yet. If the pref is set to turn SVG off, I get a
warning and two assertions as a result of the changes to nsContentDLF.cpp.

WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(docLoaderFactory->CreateInstance("view",
aOpenedChannel, aLoadGroup, aContentType, static_cast<
nsIContentViewerContainer * >(this), 0, aContentHandler, aViewer))) failed,
file c:/mozilla/trees/trunk2/mozilla/docshell/base/nsDocShell.cpp, line 4821
###!!! ASSERTION: DoContent returned no listener?: 'abort ||
m_targetStreamListener', file
c:/mozilla/trees/trunk2/mozilla/uriloader/base/nsURILoader.cpp, line 755
###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error',
file  c:/mozilla/trees/trunk2/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 459
Error loading URL http://jwatt.org/moz/circles1.svg : 8000ffff
(NS_ERROR_UNEXPECTED)

http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#4856
http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsURILoader.cpp#755
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsInputStreamPump.cpp#459


If I move the check from nsContentDLF.cpp to NS_NewSVGDocument and have that
function pass back a null pointer and return NS_ERROR_FAILURE I get the exact
same warning and assertions.

I haven't even checked to see if having the pref turned off will cause mozilla
to look for and render SVG using a plug-in. For now I need some sleep. I plan
to continue with this tomorrow, but in the mean time comments would be welcome.
:-)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Those assertions happen because you register a content viewer for that type
without checking the pref....

Comment 4

[Jonathan Watt [:jwatt]]

I see, thanks. Unfortunately the code isn't written to allow types to be
dynamically registered/unregistered, and I'll need to register/unregister the
type in my pref observer. Is it okay to expose RegisterTypes and UnregisterTypes
in nsContentDLF.cpp to other files?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wouldn't it be better to put the SVG_CheckMasterSVGPref check inside
NS_NewSVGElement and NS_NewSVGDocument functions instead. That way the code
wouldn't be as spread out and the risk tha a creationpoint was missed would be
smaller.

Comment 6

[Jonathan Watt [:jwatt]]

I could do, but if I add something like

#ifdef MOZ_SVG
  // methods to facilitate turning SVG support on/off dynamically
  static NS_IMETHODIMP RegisterSVG();
  static NS_IMETHODIMP UnregisterSVG();
#endif

to the declaration of nsContentDLF, I'll be touching nsContentDLF.h and
nsContentDLF.cpp anyway. By doing the checks in nsContentDLF.cpp instead of
nsSVGDocument.cpp the check gets done a bit earlier, and I touch fewer files.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IMHO the most important part is that the code is easy and simple to follow, as
well as secure aginst bugs (the former helps the latter). Not how many files you
touch or how early the break is made. Espcially this isn't performance critical

Comment 8

[Jonathan Watt [:jwatt]]

You're right. I'll move the check to NS_NewSVGDocument.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Are we actually trying to make this a pref that users can toggle at runtime? 
Not just one that takes effect after restart?

Comment 10

[Jonathan Watt [:jwatt]]

That's what I'm trying to do, yes. Do you think I shouldn't?

Comment 11

[Boris Zbarsky [:bzbarsky]]

I don't really have a strong opinion, but what happens to SVG documents that are
already loaded when the pref is toggled?  To scripts running in them?  What
about SVG documents that are in the middle of being parsed?

Comment 12

[Jonathan Watt [:jwatt]]

I only plan to prevent the creation of new SVG documents and elements. Documents
that are already loaded when the pref is toggled will be left in place. Scripts
that are running in such documents will break if they try to create new SVG
documents or elements. SVG documents that are in the middle of being parsed will
fail to create any remaining elements.

Comment 13

[tor]

Using nsContentUtils::RegisterPrefCallback might simplify the code.

Comment 14

[tor]

The pref should also be added to modules/libpref/src/init/all.js.

Comment 15

[Justin Wood (:Callek)]

jwatt: I would suggest for possibly a different bug that any and all scripts SVG
related get flushed and the SVG content gets replaced with a static "SVG Not
Supported" (or some sort of reload on the page as if it was normal initially). 
Something other than just letting stuff break.

This of course if dynamic support is the final intent.

I can see arguements for "This works dynamically but really should be a restart"
since we will want a plugin SVG viewer to work when this pref is off if I
remember initial "thoughts" on the pref thing right.

/me wonders if he went on too much of a tangent, if so ignore me

Comment 16

[Doron Rosenberg (IBM)]

Requiring a restart is fine(In reply to comment #12)
> I only plan to prevent the creation of new SVG documents and elements. Documents
> that are already loaded when the pref is toggled will be left in place. Scripts
> that are running in such documents will break if they try to create new SVG
> documents or elements. SVG documents that are in the middle of being parsed will
> fail to create any remaining elements.

I'd have no problems with it only working on a restart too :)

Comment 17

[tor]

Attachment: different attempt

Another attempt, just trying for a restart style pref.	Not sure
how to conditionally register the content handlers - what I have
now seems rather gross and doesn't work.  Element factory untouched
until I have basic document stuff working.

Comment 18

[tor]

Attachment: working live pref

Comment 19

[Jonathan Watt [:jwatt]]

Comment on attachment 173662 [details] [diff] [review]
working live pref

Thanks for taking this tor! I wouldn't have managed to figure out the rest
before the freeze. I do see one little error in that patch though. In
NS_NewSVGElement you forgot the call parentheses, so it should be

  if (!SVGEnabled())
    return NS_NewXMLElement(aResult, aNodeInfo);

Comment 20

[tor]

Attachment: fix mistake pointed out by jwatt

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173694 [details] [diff] [review]
fix mistake pointed out by jwatt

>Index: modules/libpref/src/init/all.js
>+pref("svg.enabled", false);

Don't we want to default to true?  I think we do...  Otherwise all existing SVG
builds will suddenly stop working and people will be confused.	;)

>Index: layout/build/nsLayoutModule.cpp
>+PRBool SVGEnabled();

That should be extern, no?

>Index: content/svg/content/src/nsSVGElementFactory.cpp
>+SVGEnabled()
>+  gSVGEnabled = nsContentUtils::GetBoolPref(SVG_PREF_STR);

Pass PR_TRUE for the default?  I think we want SVG on if the pref is not set.

>Index: uriloader/base/Makefile.in

I'm not particularly happy with SVG code in uriloader.... wouldn't it make more
sense for the SVG content handlers to just refuse to do anything if SVG is
disabled?  That is, just return NS_ERROR_WONT_HANDLE_CONTENT?

Comment 22

[tor]

(In reply to comment #21)
> (From update of attachment 173694 [details] [diff] [review] [edit])
> >Index: modules/libpref/src/init/all.js
> >+pref("svg.enabled", false);
> 
> Don't we want to default to true?  I think we do...  Otherwise all existing SVG
> builds will suddenly stop working and people will be confused.	;)

Yes, but drivers currently want svg disabled by default in the 1.8
timeframe, so I constructed the patch that way.

> >Index: layout/build/nsLayoutModule.cpp
> >+PRBool SVGEnabled();
> 
> That should be extern, no?

Don't think that's necessary - my reading of the C specification is
that external linkage is the default for function declarations.

> >Index: content/svg/content/src/nsSVGElementFactory.cpp
> >+SVGEnabled()
> >+  gSVGEnabled = nsContentUtils::GetBoolPref(SVG_PREF_STR);
> 
> Pass PR_TRUE for the default?  I think we want SVG on if the pref is not set.

See above for which way we're defaulting.

> >Index: uriloader/base/Makefile.in
> 
> I'm not particularly happy with SVG code in uriloader.... wouldn't it make more
> sense for the SVG content handlers to just refuse to do anything if SVG is
> disabled?  That is, just return NS_ERROR_WONT_HANDLE_CONTENT?

I'll experiment with that tomorrow, but I think that might be too late
to make the loading system fall back to using the plugin (if available).

Comment 23

[Boris Zbarsky [:bzbarsky]]

Drivers want SVG off in builds that explicitly --enable-svg, by default?  Or
they want the --enable-svg option off by default?  The latter I believe; the
former seems odd (pay the space cost for the code, but have it off).

> but I think that might be too late to make the loading system fall back to
> using the plugin 

The plugin, if it exists, will get picked up in step #1 in the URILoader for the
normal case (clicks on links, iframes, etc).  You're changing step #4....  That
code will only get hit if we have no way to handle this internally and have to
decide on content handler vs external helper app.

Comment 24

[tor]

(In reply to comment #23)
> Drivers want SVG off in builds that explicitly --enable-svg, by default?  Or
> they want the --enable-svg option off by default?  The latter I believe; the
> former seems odd (pay the space cost for the code, but have it off).

It's the former.

> > but I think that might be too late to make the loading system fall back to
> > using the plugin 
> 
> The plugin, if it exists, will get picked up in step #1 in the URILoader for the
> normal case (clicks on links, iframes, etc).  You're changing step #4....  That
> code will only get hit if we have no way to handle this internally and have to
> decide on content handler vs external helper app.

Step #4 seemed to be where things went horribly wrong when the categories
were gone but the content handlers were still around.  Aren't the plugins
part of step #6 (helper apps)?

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

no, plugins are not helper apps. helper apps are always executables launched as
a separate process.

plugins are gecko-content-viewers... but they don't overwrite built-in viewers,
I believe. so maybe changing the SVG pref needs to trigger plugin re-registration?

Comment 26

[Justin Wood (:Callek)]

Hmm, I wonder if people who build --enable-svg after this patch will be
surprised by the sudden need to set a pref to make SVG work even after they need
to --enable it.

Which is why, imho, we should pass true for now, and then change the pref to
false once we patch to make --enable be --disable (for SVG to be built by default).

People who currently build --enable-svg will want it on, people who do after
this patch, _because of_ this patch will change the pref, and the rest of the
people, won't be affected until after it is built by default, no?

Comment 27

[tor]

Attachment: remove uriloader change - require bug 250936 fix

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173798 [details] [diff] [review]
remove uriloader change - require bug 250936 fix

> Don't think that's necessary - my reading of the C specification is
> that external linkage is the default for function declarations.

Yes, but why bother depending on what compilers happen to implement when we can
avoid all that _and_ increase code clarity?

In fact, please add a comment saying where this function is actually defined.

r=bzbarsky with that.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 173798 [details] [diff] [review]
remove uriloader change - require bug 250936 fix

In SVGPrefChanged, add a check that the value has actually changed. Because in
some situations we seem to be able to get a callback even when the value hasn't
changed (e.g., when the user value is cleared but the user value is the same as
the default value).

Comment 30

[tor]

Attachment: updated patch

Adjust per bz and roc's comments, plus:

  Add SVGEnabled() checks to nsContentDLF::CreateInstance so that we won't
  create a SVG document before the plugin can get a chance (pref off case).

  Add SVGEnabled() checks to nsCSSFrameConstructor so that we won't try
  creating SVG frame atop generic XML nodes when dealing with compound
  documents.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174622 [details] [diff] [review]
updated patch

r=bzbarsky

Comment 32

[tor]

Check in, information about the preference posted to mozilla.svg
newsgroup and the mozilla svg update weblog.

Comment 33

[Peter Lairo]

Is there some website where we can test (and oogle over) this new SVG
fuctionality? :-)

Comment 34

[tor]

SVG is not built in the standard configuration yet - you'll still need
explicitly enable it in your builds or download ones with svg enabled.

Comment 35

[Peter Lairo]

I thought "checked-in"(1) meant they would soon (tomorrow?) be in the trunk
nightlies. 

I knew about "about:config | svg.enabled = true" from your blog(1).

(1) http://weblogs.mozillazine.org/tor/archives/2005/02/important_svg_b.html

Comment 36

[Boris Zbarsky [:bzbarsky]]

tor, with a build post this checkin, with the pref disabled, I don't get a
helper app dialog when I open up an svg file; I just get a 

Error loading URL file:///home/bzbarsky/test.svg : 8000ffff (NS_ERROR_UNEXPECTED)

in the xterm where I launched Mozilla.

Any idea what's going on there?  Can you reproduce that?

Comment 37

[tor]

Attachment: keep svg mimetypes out of persistent repository

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175895 [details] [diff] [review]
keep svg mimetypes out of persistent repository

r+sr=bzbarsky