Preference to disable SVG (for security reasons).

RESOLVED FIXED

Status

()

defect
--
major
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: doronr, Assigned: tor)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

15 years ago
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).
Taking and increasing severity since this blocks SVG from being turned on by
default. 
Assignee: alex → jonathan.watt
Blocks: 122092
Severity: normal → major
Hardware: PC → All
Posted patch rough draft (obsolete) — Splinter Review
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.
:-)
Those assertions happen because you register a content viewer for that type
without checking the pref....
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?
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.
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.
Status: NEW → ASSIGNED
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
You're right. I'll move the check to NS_NewSVGDocument.
Are we actually trying to make this a pref that users can toggle at runtime? 
Not just one that takes effect after restart?
That's what I'm trying to do, yes. Do you think I shouldn't?
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?
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.
(Assignee)

Comment 13

14 years ago
Using nsContentUtils::RegisterPrefCallback might simplify the code.
(Assignee)

Comment 14

14 years ago
The pref should also be added to modules/libpref/src/init/all.js.
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
(Reporter)

Comment 16

14 years ago
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 :)
(Assignee)

Comment 17

14 years ago
Posted patch different attempt (obsolete) — Splinter Review
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.
(Assignee)

Comment 18

14 years ago
Posted patch working live pref (obsolete) — Splinter Review
(Assignee)

Updated

14 years ago
Assignee: jonathan.watt → tor
Attachment #173434 - Attachment is obsolete: true
Attachment #173662 - Flags: review?(bzbarsky)
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);
(Assignee)

Updated

14 years ago
Attachment #173662 - Attachment is obsolete: true
Attachment #173662 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

14 years ago
Attachment #173694 - Flags: review?(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?
(Assignee)

Comment 22

14 years ago
(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).
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.
(Assignee)

Comment 24

14 years ago
(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)?
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?
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?
(Assignee)

Updated

14 years ago
Depends on: 250936
(Assignee)

Updated

14 years ago
Attachment #173694 - Attachment is obsolete: true
Attachment #173694 - Flags: review?(bzbarsky)
(Assignee)

Comment 27

14 years ago
Attachment #173798 - Flags: review?(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.
Attachment #173798 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

14 years ago
Attachment #173798 - Flags: superreview?(roc)
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).
Attachment #173798 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 30

14 years ago
Posted patch updated patchSplinter Review
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.
Attachment #173056 - Attachment is obsolete: true
Attachment #173798 - Attachment is obsolete: true
Attachment #174622 - Flags: superreview?(roc)
Attachment #174622 - Flags: review?(bzbarsky)
Attachment #174622 - Flags: superreview?(roc) → superreview+
Comment on attachment 174622 [details] [diff] [review]
updated patch

r=bzbarsky
Attachment #174622 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 32

14 years ago
Check in, information about the preference posted to mozilla.svg
newsgroup and the mozilla svg update weblog.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 33

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

Comment 34

14 years ago
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

14 years ago
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
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?
(Assignee)

Comment 37

14 years ago
Attachment #175895 - Flags: review?(bzbarsky)
Comment on attachment 175895 [details] [diff] [review]
keep svg mimetypes out of persistent repository

r+sr=bzbarsky
Attachment #175895 - Flags: superreview+
Attachment #175895 - Flags: review?(bzbarsky)
Attachment #175895 - Flags: review+
You need to log in before you can comment on or make changes to this bug.