incubator-jspwiki-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Janne Jalkanen <janne.jalka...@ecyrd.com>
Subject Re: svn commit: r917390 - in /incubator/jspwiki/trunk: ./ src/java/org/apache/wiki/ src/java/org/apache/wiki/plugin/ src/java/org/apache/wiki/preferences/ src/java/org/apache/wiki/rpc/json/ src/java/org/apache/wiki/tags/ src/java/org/apache/wiki/ui/
Date Sun, 07 Mar 2010 17:27:21 GMT

On 7 Mar 2010, at 17:40, Andrew Jaquith wrote:

>> Can you detail exactly which security issue is raised by this?
> 
> I'm worried about cross-site scripting. It should only be possible to
> source scripts that are stored on the server. That's why I want to
> check the requested resource against the ServletContext resource list.
> This check provides protection against plugins that are written
> maliciously, poorly or without security in mind. I don't think we can
> guarantee that every plugin will behave the way we expect -- even the
> ones that may someday be part of JSPWiki -- so this "origin check"
> provides a last line of defense.

...but you can include the script already *directly*, if you want to make the damage.

Also, the admin needs to make the call whether they want to install a plugin or not.  So it's
not our problem.

>> After all, plugins are a) already running at the exact same rights as the rest of
JSPWiki,
> 
> That's true, but sooner or later we're going to get it to work in a
> security manager -- that's been a JIRA issue forever. And by "we" I
> mean "me", because nobody else is gonna do it. At some point, what
> JSPWiki (and third party JARs) can do will be more constrained when
> it's running in a security manager. But this issue doesn't relate to
> my real concern -- XSS.

But we don't have it now.  Besides, any plugin can write XSS code directly on the page.

> 
>> I just simply cannot see that your method provides ANY added security, and it simply
adds a TON of implementational complexity, pain to the developer, and still does not resolve
the dynamic problem.
> 
> Personal opinion: I don't see it as particularly complex. I'm also
> offering to do the work -- in case that wasn't clear -- and I work
> cheap. :)

But I do see that as a plugin writer, *I* need to essentially stick to 2.8.  If adding dynamic
code with plugins is not possible, I have no reason whatsoever to use 3.0 anywhere.

> On the security front, I hope you can understand that my concerns
> about XSS are well-founded. Here is a cheat sheet:
> http://ha.ckers.org/xss.html

I *do* know about XSS. It's my opinion that as long as a plugin can write <script> tags
into HTML, any concerns about putting <script src> in the HEAD of the document are irrelevant.

>> This isn't really a theoretical question for future plugins, I've got a number of
plugins that rely on this functionality.  We did, after all, agree to keep the API similar
so that transferring from 2.x to 3.0 is not that complicated.
> 
> I've offered you two opportunities already to name the plugins you
> have that require this capability, and you haven't named any. Not a
> single built-in JSPWiki plugin needs to add resources. But I take your
> word for it that this is something you (Janne) require even though
> JSPWiki itself does not.

*shrug* Not all of my plugins are available in an open source manner, and it should not matter
- it is a removal of functionality and therefore breaks the API promise, and hence should
be voted on.

>> Also, I don't think that page rendering code (incl. WikiPlugin.execute()) is guaranteed
to execute before layout code.
> 
> I've tested this. The layout code DOES render after the plugins.
> Assuming layout tags are used, it's guaranteed.

We don't guarantee it.  It may work that way right now, but we don't guarantee it.  Saying
that we do would again create an API promise, and we should test for it and decide that that's
how we're going to work in the future.

> - Add back the TemplateManager.addResourceRequest() method, as a
> deprecated method.

> This will delegate back to
> WikiContext.addResourceRequest(), which is a better place for this
> code.

Moving the API is fine, but this is exactly why I've called for the creation of an API package.

> - Add origin-validation logic when the resources are added to protect
> against XSS. The resources allowed would be limited just to those on
> the server. To keep it simple, we could just perform this check only,
> and not worry about checking against annotation/XML manifests until
> after Alpha.

I think this is much easier to do - just simply check whether the resource request contains
"://".  This should be admin-configurable though; I don't use it personally but I do people
who like to include Javascript files from CDN servers.  Like if you want to include YUI libs,
you want to use Yahoo's CDN instead of hosting everything on your own.

> - Restore the InsertResources and ResourceRequest tags (retrofitted to
> use the new WikiContext method). The internal workings would be
> refactored a bit, but the results would be similar.

Since we're changing the JSP layout completely anyway, I don't think that those tags are necessarily
needed, as long as the equivalent functionality is in there.

> - Rely on the assumption that because the layout tags are used, we do
> not need use markers or buffer the response. If we determine these
> things are needed, we can add them after the Alpha release.

I think this needs a set of tests.

> I'm sorry this has been so contentious. Really.

No worries. My only concern is that this discussion has been "after the fact" - this sort
of discussion should be done on the JIRA or on the mailing list BEFORE the code modifications
are committed to allow everyone to participate and weigh in - not just those of us who actually
read commit logs.

/Janne


Mime
View raw message