Closed Bug 146401 Opened 22 years ago Closed 22 years ago

need XSLT enable/disable pref

Categories

(Core :: XSLT, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: shaver, Assigned: shaver)

Details

Attachments

(1 file, 1 obsolete file)

Now with the all.js part and real refcounting. (Shame on me.)
11.05 KB, patch
peterv
: review+
jst
: superreview+
endico
: approval+
Details | Diff | Splinter Review 
I think we're going to want to take this for 1.0.1, but it's a useful backstop
in case of late-breaking issues with XSL-T.
I'll attach a UI-less patch shortly, once I get the darned thing working.
Status: NEW → ASSIGNED
Summary: need XSL-T enable/disable pref → need XSLT enable/disable pref
Target Milestone: --- → mozilla1.0.1
Why exactly do we need this? If you don't want XSLT in the build, just remove
the transformiix library.
as the 1st comment says, it's probably about breaking features.
If we want this, it should be like the js prefs, that is, the switch should be
made available for specific sites/domains.
Drivers and others want this patch, to protect against future bugs in XSLT. 
Not everyone can remove the DLL in their installation, and it's a lot harder to
write instructions for all platforms on how to find and remove it than to tell
them how to flip the pref.  Also, turning XSLT back on after removing the DLL
requires a lot of foresight and care, else a reinstall.

This patch doesn't use an entity for the pref label, because the UI freeze date
has come and gone, and I'm willing to bet that nobody would actually make
"XSLT" into a different string.  In a better world, we've have better text for
it, but that's a post-1.0 thing.
What sort of future bugs are we talking about and how is this different for the
Transformiix module than for other code? Why no pref for turning off CSS for
example? I could understand that you want to block stylesheet loads for a
document in the same way that you block image loads etc., but you would
implement that in the content module and make it work with all kinds of
stylesheet loads and it would allow host specific setting, etc.
Why did no one (not drivers nor anyone of the "others" shaver mentions) discuss
this with anyone working on the XSLT module? I think we're reasonable people, we
try to respond to requests for help and comments and we do fix bugs. Railroading
in a fix like this does certainly demoralize me.
Comment on attachment 84761 [details] [diff] [review]
Pref with (non-localizable) UI for XSLT switching

sr=jst
Attachment #84761 - Flags: superreview+
1. shaver says drivers spoke with heikki
2. there's at least one bug about css, bug 32372
'. i'm going to try to make a very redimentry patch to disable css
Comment on attachment 84761 [details] [diff] [review]
Pref with (non-localizable) UI for XSLT switching

Duh, this is no good, we need some refcounting fixes here... Shaver's on it...
Attachment #84761 - Flags: needs-work+
I'd like to blame lack of sleep, but the fact is that I'm just an idiot.
Attachment #84761 - Attachment is obsolete: true
Comment on attachment 84788 [details] [diff] [review]
Now with the all.js part and real refcounting.  (Shame on me.)

sr=jst
Attachment #84788 - Flags: superreview+
Peter, this was discussed at the drivers/staff meeting on Wednesday.  After the
recent security problem with xmlhttprequest and the fact that xsl contains
similar functionality we thought it might be worth it to make sure that it was
possible for a user to disable the feature if there was another problem found in
the XSLT module.  You have to admit that XSLT is a large chunk of code, that
allows you to do a lot of things with data from a lot of different sources.  CSS
allows you to do similar things but doesn't have the breadth that XSLT does so
XSLT seems like the first natural target.  We thought that it was worth the time
and effort to make it possible to turn it off.

We knew that you were busy with other things so shaver was volunteered to help
you out.  Sorry about the last minute nature of this, but we're running up
against the wall on a release and we're trying to make it easier on you, if
possible.
I guess it was easiest to do it this way, but it would be kind of nice to have a
complete pref panel for XML features. I tried to do that once but the prefs
never saved. Anyone have cycles to provide an XML panel that saves its prefs, I
could then take over to add the stuff that I think would benefit from prefs?
So, I might see an argument for rushing something in for 1.0. Not very good ones,
but wth. I don't want this patch on the trunk though as it's plain wallpaper.
E.g. the rationales given here may work for XSLT, but they don't work for the 
XPath DOM functionality. And the pref says nothing about the side effects that 
this patch induces, as switching off XPath, and not only XSLT.
Plus, this patch probably conflicts with work in progress, and so harms the 
little work we can do on the trunk at this stage.

PS: I assume that drivers are aware that they pretty much screwed any procedure 
one could come up with in such a situation.
I created JavaScript.  Thank God I didn't whine and fume about a pref to disable
it, because that saved my bacon more times than I care to recall (mostly due to
DOM level 0, layout engine, and network code security holes, but that's beside
the point).  My advice to anyone feeling offended by a "disable XSLT" pref is to
Get Over It.

How exactly drivers has screwed up a procedure here, other than by designating
shaver to go fast for 1.0, is not clear to me.  "Procedure" is not at issue;
apparently the existence of the pref is.

/be
BTW, "wallpaper" connotes adding a null-check to avoid a crash, when invariants
that should preclude a null value somehow varied, and we don't have time to
figure out why.  Worse, "spackle" is when you add a null-check, but the pointer
is probably corrupt and may be non-zero but invalid.  We don't like either, but
we'll take wallpaper at the last minute if the crash is frequent (topcrash).

Adding a pref (for JS, CSS, XSLT, or other "executable content"), OTOH, is
neither wallpaper nor spackle.  It's "belt and braces", and it is good for users
who may need it if there is a firedrill we can't foresee.  Such users cannot
easily remove a .dll or .so file, and probably won't reinstall the library
later, when the coast is clear.

/be
Hmm, I think I would expect this pref to disable XPath too. If this doesn't do
it, then could you Axel describe what is the best way to do that? What side
effects does disabling XSLT/XPath have? What code is there that depends on
XPath/XSLT? I don't know the XPath API to actually know if it could be a
security issue (except with buffer overflows etc.).

The patch as is is mostly adding code, so it should be trivial to update your tree.

You can look at the rationale for this pref by comparing XSLT to a programming
language, like JavaScript. There is a pref to turn JS off, so one could argue it
should hold for XSLT too. The existence of that JS pref has been really useful
in the past buying some time to fix the issues and getting the fix to the users.
For example, that helped with the XMLHttpRequest vulnerability (although it
would have been nice if there had been a pref just to disable that instead of
disabling all of JS). We have prefs to turn off cookies, and disable image
loading as well.

The pref would be on by default, so most users would never even see it. It would
only become relevant if we learned of a security exploit in XSLT, in which case
we could publish a bulletin to disable it while we are working on the fix.

I did not understand your PS, could you explain?
re be, I was not talking against the pref itself, but about the nature of the
current patch.
Heikki et al, see comment #3 for example. Note that this comment has been made
before the patch came, I don't see this addressed yet.
The pref only talks about XSLT. And yes, XSLT is a complex engine with parts in
the spec that allow evil things to be done. Not just potential bugs in our impl,
but more or less intentional bad things in standards-conforming content. Note
the recursive template problem for example. So a pref is useful, but as I
suggested, it makes more sense if it's customizable, like the js dom prefs are.
As to side effects,
[] XSLT
does not imply XPath, nor does it mention DOM level 3. XSLT bases on XPath, but
the evil it can do has nothing to do with the nature of XPath.
Oh, is netscape aware that switching off XSLT like this will disable the P3P
policy viewer, too?
Axel: no one is switching XSLT off by default.

BTW, mozilla.org doesn't care about p3p for 1.0.

/be
I don't think disabling any feature per site is sufficient if there is a
security hole. If you go to a site for the first time it will not be on your
list and by the time the page starts loading it is too late (this could work
better if you explicitly deny all but few trusted sites). Or if you REALLY
wanted to make it fully customizable you'd have a dialog box alerting you to
every feature on the page and asking you if you want to block it for that page
or allow it etc. That is obviously too much for anyone to do in practice.
Disabling popup adds per site, or window resize per size etc. is ok though,
because it is not about hiding a security hole but denying sites to annoying
things you don't want to experience. Disabling XSLT of course means that any
page using it is totally unusable so it is not about annoyance factor.

Or maybe you meant disabling certain features of XSLT, possibly based on site.
That would be slightly better, but my understanding of XSLT is that if you
disabled document function (for example) for all sites expect the ones on your
explicit allow list, you'd get some very unpredictable results on sites that
used it legitimately and were blocked from doing this. In any case, this is
obviously too much work to have any change of getting into 1.0: the sledgehammer
approach is the only one we can safely take.

I am worried about your comment that the XSLT spec itself allows bad things to
happen. Please file a security sensitive bug where we can discuss this more.

The XSLT pref is a perception thing I guess. I assumed automatically that it
would also disable XPath, but I see some people wouldn't make that connection.
So after 1.0 we need more explanation/configurability. Please file a new bug for
more advanced pref settings and we can work things out there.

The summary view in policy viewer would break if XSLT was disabled. We can live
with it.
Comment on attachment 84788 [details] [diff] [review]
Now with the all.js part and real refcounting.  (Shame on me.)

a=tor,scc,(chofmann|brendan)
assuming this gets an r=
Attachment #84788 - Flags: approval+
Comment on attachment 84788 [details] [diff] [review]
Now with the all.js part and real refcounting.  (Shame on me.)

r=peterv. I'll file follow-up bugs to implement this in a more configurable
way.
Attachment #84788 - Flags: review+
I'm in on the branch.  I think we're going to take the more flexible version
peterv's filing about for the trunk, so this baby's closed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED
we didn't verify for a long time.
I really checked, so VERIFIED.
Status: RESOLVED → VERIFIED
No further news for the trunk? I am trying to isolate some bugs (e.g., bug
184159, bug 189338, bug 189725) and such a pref to bypass XSLT for a moment
would be useful, even if it just for debugging/perception purposes.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: