Stored XSS issues in multiple screens about opensourcepos

[jekkos]

DId some review of the first xss part. i think once config options are loaded and added to $CI->config they will be properly escaped and ready to use. That's why I omitted two redundant calls in the config load hook.

[jekkos]

Ok nice thanks for all this cleanup!

[daN4cat]

I assume this issue is valid only if somebody manages to log in in the application. Or can this happen even without having logged in?

[jekkos]

Correct. Or if someone has indirect access to the database. For example if there's another public website using the same database tables (eg external new customer form to subscribe to newsletter)

[daN4cat]

So to fix this issue would the escape function around the search string solve it?

[daN4cat]

Would be good to ask a retest of 2.3.3. Any contact with the guy that originally posted the test scan report?

[jekkos]

Most of these issues will be fixed by bootstrap tables (except for the ones in register and receivings)

[daN4cat]

What do we need to do in Sales and Register to get this sorted?

[jekkos]

Try to execute this code on db update ospos_customers set first_name = '<script>alert(document.cookie);</script>'; then navigate around this js will be executed. Quite a serious problem if you have a eg your db exposed with a customer form to public internet. One can hijack ospos session and thus get admin rights very easily.

[jekkos]

Think we need to use the xss filtering on db fields that are echo'ed in ospos views.

[daN4cat]

Given bootstrap-tables change, do we need to do something more about this?

[jekkos]

Yes we'll need to escape all database output in places outside of bstables. Think this is quite a critical bug and I want to have it fixed in 3.0.

[jekkos]

Think ospos has some csrf problems as well (cross site request forgery). By presenting a certain link on a random webpage to a logged in pos admin, manipulations might be done without his knowledge.

[daN4cat]

I once tried to enable CI csrf protection in the config but all stopped working.
I didn't have time to check more but probably we should investigate more these parts.

Also going forward if we start adding RESTful API we should try to be more robust and careful about the security.
A good exercise to learn more about this topic as really important.

[daN4cat]

AJAX + CSRF protection in CI https://arjunphp.com/ajax-csrf-protection-in-codeigniter/

[daN4cat]

Small thing, but I thought that adding a sanity check on uploaded images and excel files is a good thing. Maybe excel files are less dangerous if under the user control but better to be paranoid.

[jekkos]

Good point. Nasty thing can happen in this kind of situation. You could for example upload a php shell script disguised as an excel file and have it executed. We should certainly check that an attacker is not able to upload a .htaccess file (as he could then take over the server easily)

[jekkos]

http://stackoverflow.com/questions/8025236/is-it-possible-to-execute-php-with-extension-file-php-jpg

[daN4cat]

That's why my .htaccess is read only

[jekkos]

Think the way CI approaches xss isn't really correct.. one should escape when outputting data and not on input.. Information will be lost along the way.

[daN4cat]

Agreed, we said that we need to sanity check before representing in views. For the specific case I wanted to check on the input side too. Assume you upload a picture with malicious data in it, if you read that data from another app you inject the issue to another side.

[daN4cat]

By the way, I assume we need to XSS clean data going to the views and read from DB/file system at controller level, right?

[daN4cat]

I thought about this, probably the sanity check goes on both ways. If something from an input is logged in a database or file that could be read from a different system / client. Therefore the injection would work on input side as well.

[daN4cat]

And we need to escape the data going to database for SQL injection too with $this->db->escape, I guess this requires a review of all the models.

[daN4cat]

To be paranoid we should sanity check even the language files entries.

[jekkos]

Think we should really only consider variable output that can be user modified at runtime.. language files might already be a step too far. If all applications escape correctly at output time there should never be a problem.

Input sanitization might be problematic when processing eg html markup coming from an editor, which would cause the input to be altered in db. Had this kind of issue before with CI and ckEditor.. sometimes specific data was cleaned and removed and I didn't have a clue why.

[daN4cat]

Why XSS filtering on input is wrong: http://stackoverflow.com/questions/20375382/xss-filtering-in-codeigniter

[daN4cat]

Long thread discussion about CI XSS filtering: bcit-ci/CodeIgniter#2667

[daN4cat]

I started working on this:

• removed any XSS filtering present in post & get (agreed there is no purpose for those on input side)
• XSS clean of Config and all the configuration data loading
• XSS clean of Giftcard

The plan is to go in alphabetic order through the Controllers and adjust any View and Model (I wanted to make the data passing to views explicit and cleaned if possible)

[daN4cat]

Agreed.

There is however a question around the change of config and whether the autoload is called on a change? I didn't check/tested that case.

[jekkos]

I think that the config is reloaded every request, however I'm not really sure. Actually I would like to see which queries are executed on page refresh to see if there's anything we can optimize there. Adding a memcached (session or global key-value cache) for static query results would benefit performance in the long term.

[daN4cat]

With this commit:

• XSS clean Customers, Employees, Suppliers, Person and Item_kits
• minor changes to Giftcard and Config

[daN4cat]

It looks like bstables has XSS cleaning using the escape option: wenzhixin/bootstrap-table#1049

Therefore the escaping is either on bstable side or Controller side. It's something to check and also we would need to understand if both are the same or one is better than the other as filter.

More ref to XSS cleaning on bstables: wenzhixin/bootstrap-table#1854,

[daN4cat]

In manage_tables.js there is no escape option initialised, however if I read the code correctly escape is set true by default: wenzhixin/bootstrap-table@b740ffb

Is that escaping enough? Not sure because checking system/core/Security.php it looks like a far more complex story.

Therefore I would set escape: false on bstable side and leave the xss_clean as it is in our Controller.

[daN4cat]

Disabled bstables escape feature, I believe that support is naive. If one reads CI one cases you can understand.
Strange but CI XSS_clean is still commented as something that should run at DB commit time not at runtime, but we agreed not to do that.

That said I cannot enable XSS_clean in Report inventory as causing a PHP timeout. At the moment the function call is commented out, so not sure what we can do here. Thoughts?

[daN4cat]

With my last commit:

• disabled bstable escaping on other tables so we rely just on CI XSS clean
• I refactored a bit the Controllers class hierarchy:
  • renamed Secure_area to be Secure_Controller
  • removed an interface that was not used at all and made at the moment the methods parts of the base class
  • created a xss_clean protected method in the base class to be used by the sub classes. In this way if we feel we need to change or add an option to config to enable / disable it for performance reasons we can do (e.g. standalone and closed system don't need to worry about xss issues)
  • renamed Person_controller to be Persons, in line with the rest of Controllers naming convention

[daN4cat]

XSS cleaned Items

@jekkos I have an issue, I don't seem able to show a notification with red banner ('success':false) in Items.
If you try to import something with excel that is not the right one the json encode array format looks fine : {"success":false,"message":"Most items imported. But some were not, here is list of their codes (1): 1"}
however there is a js error Uncaught TypeError: Cannot read property 'title' of undefined.

Any clue?

[daN4cat]

OK, excel import notifications fixed. I hope that is the right fix, but it seems to work fine.

[daN4cat]

With XSS clean task I'm left with Sales and Receivings register and I need to check table_helper too.

[daN4cat]

XSS clean Sales, quite a bit of job even because I needed to refactor the code to make it easier to support.

[jekkos]

I'll try and test some stored xss scenarios later this week.

[daN4cat]

Ok, I plan to finish Receiving with a bit of refactoring. The testing should be probably systematic injecting a js script in each field of each table and then run a full test. I had in mind to create a SQL script that adds what I said and then ask more people to add the data in the DB and do manual testing of everything and see if something pops up.

[daN4cat]

With my last commit the XSS clean work is completed with Receivings.

I also did few other things on Receivings:

• added complete Suppliers details
• display company always where supplier is required instead of a inconsistent mix up
• fixed a bug in Receivings detail report as the supplier was not shown at all
• fixed a bug with Receiving form as there was no way to save a change
• added glyphicons to receipt buttons
• fixed translations
• moved payments to Receiving model

[daN4cat]

This is done and requires testing.

I may add a config parameter (config file not Store Config) to disable the XSS clean for the people that uses OSPOS in a closed environment with no Internet so they don't get a performance impact for no reason.

[daN4cat]

With my last commit the OSPOS XSS clean is configurable from application/config/config.php

If people don't add the config param it will be disabled by default, but the standard OSPOS distribution will have it enabled.

I believe this is done, and we should just test.

[daN4cat]

I made a test on each view adding <script>alert(‘XSS attack’)</script> just to make sure the basic sanitasation works and it's effective. The injection appears as [removed]alert(‘XSS attack’)[removed]

[jekkos]

Ok nice one. I was trying to prepare a db with some stored javascript snippets to see whether something comes out when clicking around.

[daN4cat]

Yes, a more systematic testing would be good.

[daN4cat]

It looks like that with big data Inventory are not working. Not sure what's wrong.

[daN4cat]

I read this: Uncaught SyntaxError: Unexpected token ,

$('#table').bootstrapTable({
columns: [{"field":0,"title":"Item Name","sortable":true,"switchable":true},{"field":1,"title":"Item Number","sortable":true,"switchable":true},{"field":2,"title":"Description","sortable":true,"switchable":true},{"field":3,"title":"Quantity","sortable":true,"switchable":true},{"field":4,"title":"Reorder Level","sortable":true,"switchable":true},{"field":5,"title":"Stock Location","sortable":true,"switchable":true}],
pageSize: 20,
striped: true,
sortable: true,
showExport: true,
pagination: true,
showColumns: true,
showExport: true,
data: ,
iconSize: 'sm',
paginationVAlign: 'bottom',
escape: false
});

The issue is with data: ,

[daN4cat]

I created a Items temp table to support the Inventory creation and performance improvement since XSS clean of many rows slows down things. I reduced the fields of one so we have less parsing.

[daN4cat]

I close this one unless somebody can find the time to do a full test, some manual test showed to be effective.