Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stored XSS in v2.1-rc1 #456

Closed
We5ter opened this issue Mar 4, 2017 · 11 comments
Closed

stored XSS in v2.1-rc1 #456

We5ter opened this issue Mar 4, 2017 · 11 comments

Comments

@We5ter
Copy link

We5ter commented Mar 4, 2017

1.create a standard editor named "test",
screen shot 2017-03-04 at 4 43 36 pm

2.write a new entry

<img src=1 onerror=alert(1)>

screen shot 2017-03-04 at 4 44 22 pm

3.then post this entry,when admin view it ,XSS occur!

screen shot 2017-03-04 at 4 44 36 pm

@onli
Copy link
Member

onli commented Mar 4, 2017

Hi @We5ter

Thanks for reporting. But there is no sound defence against that, as far as I can see. It is absolutely necessary that javascript embedded into entries gets executed, as that enables a broad range of use cases for writing better articles - think charts, for example. We could now escape the js only when an admin is logged in, but then he would no longer realize what happens on the blog.

If you are in a multi-user blog, consider installing the serendipity_event_xsstrust plugin. With it you can define which users get the ability to post html in entries.

@We5ter
Copy link
Author

We5ter commented Mar 4, 2017

@onli Thank you for your quickly reply.

As you said,I have installed serendipity_event_xsstrust plugin and set value to "3"(plugin say this mean banned),but from this pugin description,this plugin just shows all authors with their ethic value,but cannot stop users get the ability to post html in entries.

And,I suggest:

As a rich text editor,other cms also allow inject html/js in entries,so I think serendipity should use CSP or httponly or filter user's input using Regular Expression

screen shot 2017-03-04 at 7 57 31 pm

@onli
Copy link
Member

onli commented Mar 4, 2017

Oh, that looks like a valid bug in htmlpurifier! I will try out whether the current version of that fixes it.

@We5ter
Copy link
Author

We5ter commented Mar 4, 2017

@onli Thank you for your hard work!

@onli
Copy link
Member

onli commented Mar 4, 2017

There is still an issue, but you installed the wrong plugin.

You installed serendipity_plugin_xsstrust, the sidebar plugin that only shows the values. You'd need the serendipity_event_xsstrust plugin, which comes in the archive, but is in the other plugin menu tab. Note that it is broken in current s9y versions. The fix:

Change L96 from

function set_config($name, $value) {

to

function set_config($name, $value, $implodekey = '^') {

That plugin allows you to filter html completely, and that is one solution. However, that means really completely, even the output from markup plugins is fixed, which maybe should be optional.

That is supposed to be better with htmlpurifier. Enabling the htmlpurifier option in there should filter out the script options. But it does not work for me. It seems that the purifier is not being run, and it seems like it does not matter whether it is the current or the bundled version.

@garvinhicking, sorry to ping you again, but I'm stumped here.

@We5ter
Copy link
Author

We5ter commented Mar 4, 2017

@onli It seems that I have installed wrong plugin indeed...

@garvinhicking
Copy link
Member

@onli That's bad :-D

I fixed it, it checked for the wrong parameter, obviously we no longer seem to propagate $eventData['authorid'] in the presave entry hook, so I now use the currently logged in user for that.

@onli
Copy link
Member

onli commented Mar 8, 2017

@We5ter Does that fix it for you?

@We5ter
Copy link
Author

We5ter commented Mar 8, 2017

@onli It seems @garvinhicking has fixed that plugin,so this issue has been fixed yet.

@We5ter
Copy link
Author

We5ter commented Mar 8, 2017

@onli @garvinhicking Thank you for your hard work.

@onli
Copy link
Member

onli commented Mar 8, 2017

Then I'll close here. Thanks for the report. It was a real bug after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants