XSS security hole in Oil generated admin templates

[ichilton]

Hi,

I've found an XSS security problem with the oil generated admin screens.

If you do:
oil generate admin posts title:string slug:string summary:text body:text user_id:int

The generated code uses:
$this->template->set_global('post', $post, false);

Setting false as the 3rd parameter means that it's not filtered.

But then it's used as follows - and Form::input doesn't escape the values either:
Form::input('title', Input::post('title', isset($post) ? $post->title : ''), array('class' => 'span6'));

Thanks,

Ian

[ichilton]

I've submitted a pull request here: #133

[ichilton]

Ok, it just occurred to me that this should probably go into 1.2/develop so re-issuing the pull request.

[ichilton]

Here we go - same commit, but in 1.2/develop: #134

[philsturgeon]

Do you have an example of an attack?

[ichilton]

Discusion about this on IRC: http://scrp.at/bxm

[ichilton]

@philsturgeon I don't actually - I couldn't understand why it was done like that and realised it was a problem.

[philsturgeon]

It's mainly to avoid the XSS filtering screwing up any useful characters that may go in the title, as a & ended up double-encoded on output.

[ichilton]

Yeah, that's what I thought.

I think a better fix would be to change the template so it outputs:
Form::input('title', Input::post('title', isset($post) ? e($post->title) : ''), array('class' => 'span6'));

What do you think?

[ichilton]

Hmm - I just tried to exploit it and it didn't work.

I put this is a test in a field and saved it, and it is being correctly escaped - i'm just not sure where!

[ichilton]

lol - Github XSS filter instead of escape then :)

[kenjis]

All Form methods escapes values with prep_value(), doesn't it?

[ichilton]

@WanWizard said it wasn't, but it must be I guess?

[WanWizard]

I was refering to data passed to a view, and you were asking about input (which I assumed was about the Input class, so POST data, not input form fields).

The different form methods will prep the value passed when the form "prep_value" setting is true (in your config/form.php), and the attribute "dont_prep" isn't set to true. prep_value() does the same as passing data to a view, it escapes it.

So indeed it's not a good idea to pass data to a view with escaping, and then use Form methods in a view (unless you've set "prep_value" to false in your config).

[kenjis]

But if you use $post->title outside of Form methods in a view, XSS could be done.
So is it better the default of fuelphp is with escaping? I mean default by secure.

Security::htmlentites default is not double-encoding. So most users will not get troubled.

[WanWizard]

Personally I find it confusing that the Form methods escape, while it is best practice to escape everything passed to the view. So a form method prepping should be the exception, not the rule.

But I assume it has to do with the fact that until now, database objects have to be passed to the view raw as they are read-only and can not be escaped. So for those not using ORM (or array conversions), prepping is probably a good thing.

[WanWizard]

This is a very confusing issue. Is this still a problem, and if so, can someone tell me exactly what the issue is?