Serendipity Forums

Hi


Yellowled informs me that this code is a security risk:
I have been using it for a year in the header image selection found in, among others, freshy theme which is on spartacus. The code above is in a function which generates html form code which is added into a content type in the template's $_template array. $text['var'] would be a theme variable like header_image and accessible via $template_option.header_image . Full code

http://netmirror.org/mirror/serendipity ... el.inc.php

Also is there a fix for the stale data that is returned by serendipity_loadThemeOptions right after form submission?

Hi!

Using this code means that someone can simply make a HTTP POST request to your site (i.e. automatted through javascript) with which he could change your template options.

This could lead to all possible things, depending on the template options you have. The people could place a different header image on your site, maybe even include remote files if your template options allow this.

$serendipity['POST'] is user input, and you should never trust that. By replacing the template options with that user input, you are accepting whatever a VISITOR submits to you, he must not even be an author.

Of course those things are not permanently applied to your blog, only if visitors come from foreign HTTP Post forms, but this is the usual attack vector of XSS attacks.

The code you posted here uses a different approach than the one yellowled committed; as long as you check the POST input data vor valid keys and valid contents, you can remove the security risk. It would also be good to check for serendipity_userLoggedIn() and if the permission 'adminTemplates' exists (if serendipity_checkPermission('adminTemplates')).

So I believe the freshy code would need that security checks if a user is really privileged to accept POST data.

About the "stale data", sadly the current concept doesn't really allow to change that; the config.inc.php is required to be able to save the config data - and we cannot simply re-include config.inc.php again after saving, because some template's config.inc.phps define functions that would throw a fatal error if included twice. I believe the only workable approach for dynamic config fields within config.inc.php is to inspect the POST-Data with previously ensuring that the user is logged in and allowed to accept POST-Data (through that permission check).

HTH,
Garvin

Thanks for your detailed response.

I understand that this isn't good coding and I will make updates to it but after thinking about it a little more I see that this particular code cannot cause theme options to be changed by a non-logged in user. Please bear with me and tell me if the logic I am thinking is right or not

The code returned by the above function custom_imgsel is put into a content type variable of the $templates array. Since a content type variable is only output to the browser on the manage styles page, a not logged in user cannot achieve anything by changing its value. Also a logged in user without the correct permissions cannot access the manage styles page (right?) so he can't do anything with it either.

What remains is a logged in admin unwittingly visiting a malicious page that POSTs to serendipity admin causing the his browser to display manage styles page and execute injected JS. If such a situation is possible it would be easier to simply POST a new set of site configuration options (db name, host etc. ) taking the entire site down.

Hi!

Yes, your logic sounds reasonable. The code committed by YellowLED yesterday globally replaced the $template_options array, which would also be used by the frontend - and that was what I mainly referred to as insecure.

However adding that security check for userLoggedIn + checkPermission would also make those more secure on the backend.

And of course, also the attack vector you describe. Usually, s9y makes sure on all pages that a Security Token needs to be present before changes are applied. I see that this token is not there on the Manage Styles page, but it is inside the s9y configuration screen. So I will add a token check on the place where the POST is evaluated now, but this would not fix the situation where a template's config.inc.php would output (not store!) the invalid data.

Best regards,
Garvin

(Fix committed to rev. #2250 of SVN trunk)

I have updated freshy and if you like to can update the version on spartacus with the new zip file. The new version also has support for 2 sidebars.

Hi!

Thanks a lot, updated it!

I had to change commentpopup.tpl and index.tpl to include
because that xml:lang is required to render als XHTML 1.1 compliant. Maybe you want to update that in your ZIP?

Regards,
Garvin