when global XSS is enabled, input sanitization runs unconditionally #346

Closed
joeauty opened this Issue Aug 29, 2011 · 25 comments

Projects

None yet

9 participants

@joeauty
joeauty commented Aug 29, 2011

when global XSS is enabled, it seems impossible to include any sort of login form where passwords may include characters such as &. Shouldn't $this->input->post('var', FALSE) not only disable XSS cleaning, but also sanitization, or there otherwise be some option to not sanitize these sorts of fields?

@gaker
Contributor
gaker commented Aug 29, 2011

Yes, these will always be escaped when global xss filtering is on. If you want fine-grained control, don't turn on global xss-cleaning.

http://www.gregaker.net/2011/mar/30/what_is_xss_clean_in_codeigniter_and_why_should_i_use_it/

@gaker gaker closed this Aug 29, 2011
@joeauty
joeauty commented Aug 29, 2011

Thanks gaker, I would argue that this is a bug though.

Your URL is quite clear as to when not to use global XSS, I will definitely file that away for future reference, but from a design perspective I think it is not unreasonable to expect that a $this->input->post('var', FALSE) will disable all scrubbing of any kind to be the equivalent of what one would expect with global XSS disabled, right? What value does global XSS have if it can't be overridden? Like I said, it seems to make all password entries with characters such as & impossible.

So, is this accurate?

with global XSS disabled:

  • $this->input->post('var') : no manipulation, when activerecord is used input is sanitized for DB input
  • $this->input->post('var', TRUE) : XSS cleaning, don't know about sanitization
  • $this->input->post('var', FALSE) : dunno

with global XSS enabled:

  • $this->input->post('var') : both XSS cleaning and sanitization
  • $this->input->post('var', TRUE) : dunno
  • $this->input->post('var', FALSE) : XSS cleaning disabled, sanitization remains

This is a little wacky, don't you think?

@gaker
Contributor
gaker commented Aug 29, 2011

yep, you're right.

Thanks. Reopening

@gaker gaker reopened this Aug 29, 2011
@ktomk
Contributor
ktomk commented Aug 29, 2011

Three cases but just a bool. How could that be mapped?

@joeauty
joeauty commented Aug 29, 2011

I would suggest the following:

with global XSS disabled:

$this->input->post('var') : no manipulation
$this->input->post('var', TRUE) : XSS cleaning only, no DB sanitization (DB sanitization is triggered by DB/ActiveRecord class)
$this->input->post('var', FALSE) : redundant, unnecessary

with global XSS enabled:

$this->input->post('var') : XSS cleaning only
$this->input->post('var', TRUE) : redundant, unnecessary
$this->input->post('var', FALSE) : XSS cleaning disabled, no DB sanitization (DB sanitization is triggered by DB/ActiveRecord class)

IOW, this setting is as advertised: controls for XSS cleaning - no freebie DB sanitization/htmlspecialchars. If you feel that free htmlspecialchars is a needed part of this design, how about the above with the following modifications:

with global XSS disabled:

$this->input->post('var', TRUE) : XSS and DB sanitization

with global XSS enabled:

$this->input->post('var', FALSE) : no XSS, no DB sanitization

The FALSE part of this would allow an override for global XSS, allowing password input.

@joeauty
joeauty commented Aug 29, 2011

Alternatively, if you want to be really explicit:

$this->input->post('var', [xss_clean boolean], [sanitization boolean])

@ktomk
Contributor
ktomk commented Aug 29, 2011

The alternative looks misleading. Either it's extending from bool to a bitwise flag, or there really is a third parameter on it's own which I doubt. XSS is sanitization by current use. So two parameters for the same domain really sounds misleading even if it appears as a "simple" solution upfront. Never-mind I'm doing clever-speaking here only, so don't trust my words. This issue brings up XSS in CI for the first time to me, thanks for the insight.

And I assume global XSS is just a global variable that can be changed any-time within the current request, so a global variable and not a constant. Which could solve your problem first-hand: If it is a password field, read the option, set it to false, process the password field input, set it to the old value. Not very comfortable but this could be hidden into the form helpers in case of code re-use.

Just some thoughts, take it as feedback. Not really concrete suggestions.

@joeauty
joeauty commented Aug 30, 2011

how about:

$this->input->post('var', array(
'xss' => true,
'sanitize' => true
));

I'm not sure if this addresses the point you made though, ktomk...

I thought about the on-the-fly global config file adjustment thing, but I haven't had luck with that sort of approach with other config file arguments in the past. I'm not sure if there is a way to reinit a global config, where by doing so everything that is affected by is altered to match the environment even if this has already taken place.

I think that setting global XSS to true will manipulate any POST/GET variables that exist for that page load, but there is no way to unsanitize stuff that is already sanitized. In order for this to work you'd have to somehow do this before the initial config value is read and acted on.

Does this make sense?

At any rate, I think that modifying the API and/or behavior is a better, more elegant option than asking/expecting the developer to implement on-the-fly global overrides.

@ktomk
Contributor
ktomk commented Aug 30, 2011

I'm not sure if this addresses the point you made though, ktomk...

Yes it does. Be it an array hash or just a single value with bits sets, this in principle expresses the same, it's just that bitwise flags are more direct in terms of memory usage and access. Anyway, this keeps related settings in one parameter.

The on-the-fly global config file adjustment is far more complex and my talking about it obviously looks short-sighted. Additionally modifying a context that is merely undefined to control the actually processing in that context is really insane. It highly likely will break with upcoming changes. Could have made it under circumstances, but I think $_POST is as you write processed pretty in the beginning, so not really worth to deal with. Far better to make the behaviour of "global XSS" predictable and especially control-able.

@kenjis
Contributor
kenjis commented Aug 30, 2011
@ruthlessfish
Contributor

I don't use it either.

@joeauty
joeauty commented Aug 30, 2011

I don't use it either

@kenjis
Contributor
kenjis commented Aug 30, 2011

@joeauty

What do you mean by "sanitization" or "DB sanitization"?
I thought it means _sanitize_globals() in https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Input.php#L424
But it's not related DB.

@joeauty
joeauty commented Aug 30, 2011

by sanitization I mean htmlspecialchars(), which is traditionally used pre database inserts

@ktomk
Contributor
ktomk commented Aug 30, 2011

by sanitization I mean htmlspecialchars(), which is traditionally used pre database inserts

Not my tradition. Sounds like a major flaw in data processing within an application to me. But totally okay if the database is the file-system you save static HTML files to that can be accessed via the browser. But I fear that is not the database you've meant :).

@kenjis
Contributor
kenjis commented Aug 30, 2011

xss filter does not use htmlspecialchars(). its logic is very very complex.
See: https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Security.php#L251

And it is better you use htmlspecialchars() when you output HTML, not pre database insertion.

@joeauty
joeauty commented Aug 30, 2011

"Not my tradition. Sounds like a major flaw in data processing within an application to me. But totally okay if the database is the file-system you save static HTML files to that can be accessed via the browser. But I fear that is not the database you've meant :)."

Right, sorry, my mistake, I forgot that there are other ways to escape characters other than htmlspecialchars. It's been so long since I've had to do pure PHP db manipulations, I guess I'm pretty spoiled by frameworks like CodeIgniter, that I didn't consider this. Please excuse my imprecision.

Instead of making assumptions about what is going on, cause I haven't yet scanned through the Security class to study it, I'll just say that the manipulation that is going on right now with global XSS enabled and $this->input->post('var', FALSE) is conversion of, for example, & to & amp; (without the space between the & and amp). You can perhaps see why my brain jumped to conclusions WRT htmlspecialchars :)

What do you guys think is the most elegant fix for this problem and all of the confusion caused by the different kinds of text manipulation and the framework syntax? What I wrote above would what I think would be developer-friendly, but perhaps there are other variables I'm not taking into account?

@kenjis
Contributor
kenjis commented Aug 30, 2011

My understanding is follows.

Global XSS filtering run at initialization of CI. So all POST data has been filtered already when your controllers run.
So if you use $this->input->post('var', FALSE) in your controller, it makes no effects, because $this->input->post('var', FALSE) does not recover the original post data (CI has no method like that).

XSS filter does not convert & to & amp; but it puts ";" at the end of string which begin with & and more than 2 alphabets.
For example, "EPA&DHA" is converted to "EPA&DHA;"

In my opinion, the best practice is "don't use global xss filtering at any time".

@joeauty
joeauty commented Aug 30, 2011

kenjis: that is not quite correct, because with Global XSS enabled and the false argument set on $this->input->post there is some sort of difference between that and with true set, I've been able to verify that with one of my users who was having problems logging in when set to true - setting the option to false was a fix for him. As it turns out it was not a complete fix, since there have been other users having problems.

I agree that for right now the "dont' use global XSS at all" seems to be sound advice, but I'm assuming just temporary advice until this feature can be fixed or scrapped altogether?

@kenjis
Contributor
kenjis commented Aug 30, 2011

Oh, there are something I don't notice.

with Global XSS enabled and the false argument set on $this->input->post there is some sort of difference between that and with true set

I checked the source code.
If Global XSS enabled and $this->input->post('var', TRUE), XSS fitered called twice. It may make difference.

XSS filter changes or removes some chars, so the only solution is like below?
Save the original POST data in Security class and $this->input->post('var', FALSE) returns it.

@joeauty
joeauty commented Sep 8, 2011

I'm not sure I'm following this, but I can definitely see XSS filtering called twice being problematic.

Sorry for my delay in participating in this topic...

@cryode
Contributor
cryode commented Dec 24, 2012

So has anything been done with this at all? Both issues mentioned here have been brought up multiple times.

@t3nsor
t3nsor commented Mar 2, 2013

Is this ever going to get fixed?

@narfbg narfbg added a commit that closed this issue Jan 8, 2014
@narfbg narfbg Fix #346
When ['global_xss_filtering'] was turned on, the , ,  &
superglobals were automatically overwritten. This resulted in one of the following problems:

 - xss_clean() being called twice
 - Inability to retrieve the original (not filtered) value

XSS filtering is now only applied on demand by the Input class, and the default value for
the  parameter in CI_Input methods is changed to NULL. Unless a boolean value is
passed to them, whether XSS filtering is applied depends on the ['global_xss_filtering']
value.
80a16b1
@narfbg narfbg closed this in 80a16b1 Jan 8, 2014
@narfbg
Contributor
narfbg commented Jan 8, 2014

Done. Please read the full commit message for details on the solution.

@nmfzone
nmfzone commented Jan 10, 2015

Hey guys, anyone in here?
May i know, when i was set global_xss_filtering TRUE, must i add xss_clean in form_validation rules?
And what is the effort if i add and not add xss_clean in form_validation when global_xss_filtering TRUE?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment