scoumbourdis / grocery-crud

Grocery CRUD is a PHP Codeigniter Framework library that creates a full functional CRUD system without the requirement of extra customisation to the JavaScripts or the CSS to do it so.
http://www.grocerycrud.com
GNU General Public License v3.0
1.01k stars 472 forks source link

Xss major vulnerabilities #331

Closed alindima closed 8 years ago

alindima commented 8 years ago

Hello everyone.
Grocery crud is extremely vulnerable to xss attacks..The output that is being displayed in a regular table is not escaped at all,and as we all know, that is a massive problem. Please fix this ASAP. Thanks.

airways commented 8 years ago

This is extremely irresponsible disclosure. Way to go.

alindima commented 8 years ago

@airways ,sorry but i did not get your point

airways commented 8 years ago

@alindima https://en.wikipedia.org/wiki/Responsible_disclosure

scoumbourdis commented 8 years ago

Hello @alindima,

I am sorry to admit it but @airways is right. Can you please send me at my personal email a more specific issue of what the problem exactly is and suggestions to actually solve the issue?

Thanks Johnny

heruprambadi commented 8 years ago

any solution how to prevent that ?

buoncri commented 8 years ago

Using GC only in Admin area :-D

scoumbourdis commented 8 years ago

I did start a new branch to fix that issue. If you urgently need it then you can have a hot fix of just filter all the inputs from here: https://github.com/scoumbourdis/grocery-crud/commit/cb82d941e82b4cb90a06229d9194e58a8fde8dcb of course this branch it is not completed yet as it needs to have a config file and some other validation checks. I just published this change in case someone needs that urgently.

Thanks Johnny

agung-wete commented 8 years ago

As suggested by codeigniter user guide :

rule about XSS cleaning is that it should only be applied to output, as opposed to input data.

We’ve made that mistake ourselves with our automatic and global XSS cleaning feature (see previous step about XSS above), so now in an effort to discourage that practice, we’re also removing ‘xss_clean’ from the officially supported list of form validation rules.

Sometimes, we need that "special" character that filtered by execute xss_clean on input (for example: password field)

Because this is CRUD library, we cant determine which input need this, so we shouldnt do xss_clean globally.

To prevent XSS we need to do html_escape in output field (function change_list, get_add_input_fields, get_edit_input_fields, get_read_input_fields). This is also make sure ' " ' from data dont break input tag.

Suggestion :

Thank you

scoumbourdis commented 8 years ago

@agung-wete thanks for that and to be honest I like that this is now a thread for brainstorming :)

I actually agree with what Codeigniter is saying to not have it globally. That's why the fix is half way through and it is not completed yet.

@agung-wete first of all although grocery CRUD is depended on Codeigniter the library is built with that way so one day will NOT be Codeigniter depended at the future. So currently the only libraries that grocery CRUD is using from CI are:

Nothing more than that (not even the session or the layout is not in use)

Second xss_clean of Codeigniter is really really slow (especially for big texts). That's the main reason that I don't want to use it and I am trying to find another solution.

I will update you for any changes that I will make to the code. The code is not yet completed yet and new ideas are more than welcome :)

scoumbourdis commented 7 years ago

New upcoming version 1.5.7 with XSS clean. If you are interested you can download the master branch. Enjoy :)

heruprambadi commented 7 years ago

hurray ! 💃 💃

scoumbourdis commented 7 years ago

The version is now in public with plus an optimisation of the totals on big tables

heruprambadi commented 7 years ago

Where should i find documentation about it ?

scoumbourdis commented 7 years ago

There is not currently any documentation. It is as easy as setting the config file grocery_crud_xss_clean into true.

More specifically the file is at: application/config/grocery_crud.php

The main reason I did that is that in case you will need to skip the xss_clean validation only for 1 of your CRUD, you can simply do this:

 $this->load->config('grocery_crud');
 $this->config->set_item('grocery_crud_xss_clean', false);

before calling the grocery CRUD.

Also have in mind (I am actually copying the comments at the config) that:

// Turn XSS clean into true in case you are exposing your CRUD into public. 
// Please be aware that this is stripping all the HTML and do not just trim the extra javascript
heruprambadi commented 7 years ago

Oh, ok. I just need to know how to use it. Thanks 😆 Anyway, where (again) i should find details about change log ? Like how to use "faster way to calculate total" (gc 1.5.7).

scoumbourdis commented 7 years ago

I am trying to keep track at the github. So for example if you see a number like (#370: A faster way to calculate the totals) The #370 is a link that you can click and see all the discussions/commits/pull requests and history of the enhancement.

scoumbourdis commented 7 years ago

You can basically see the change logs (with the urls) here: http://www.grocerycrud.com/documentation/change_logs

heruprambadi commented 7 years ago

So there is change log on your site.. ok thanks again 😀