High risk of cross-site scripting

[DocBouCou]

I have a concern regarding security, especially cross site scripting.

When allowing formulas parsing, what is typed after the "=" is directly interpreting as code (probably using eval() ?). I tested it, we have access to the window scope, to the cookie (risk of session stealing), to everything (for instance, I am using vuejs, it means we may probably have access to the whole SPA from one cell), to xhr request (fetch request, to your server, or another one).
So, I thought that if I used "parseFormulas: false", I would be good to go, I was wrong. As pointed out in #447 , user may enter any html, even if the column type is text (why not use "Node.textContent" or innerText then?).
You can test these issues directly on bossanova.uk if you want...

You may argue that it is not so bad, because anyway the user may use the browser console, or the inspector tools to add some code anywhere.
But, in the case of jExcel, what the user types is most likely intended to be saved on the server side. Malicious code will be saved and executed when the data are loaded... (this is not the case if modifying client-side code using browser tools).
Or values from jExcel are also intended to be used directly in the front side, in my case to plot some charts (I do not expect the user to play with browser tools to commonly use my SPA...), thus leading to high security risks.

If concerned about security, we end up sanitizing data using "onChange" + "setValueFromCoords", which is quite heavy, error prone, time-consuming. And then, what if there are others security concerns with jExcel? Then, sadly, I do not feel confident using jExcel, even if I spent time to implement it in my application, and even if I can see the huge amount of work you put into it.

You may want to read about cross-site scripting to realize how bad a so obvious security flow might be : https://excess-xss.com/ or on medium.

[pphod]

Hi,
Firstly, I understand your concerns and I am very happy to discuss potential solutions.

In regards to the formulas, you can disable by using the flag you mentioned. In regards to adding HTML to the table, currently, you have options to overwrite the user input. Onbeforepaste, Onbeforechange, Onbeforeinsert, Onbeforerow. All of those allow you to intercepts what the user is entering. But, to avoid the overhead work I am happy to create an optional flag to create some auto sanitize user input.

As you mentioned, the most concern thing is the data sent to the server and persisted later to other users. This is a real concern but is not exclusive from Jexcel as a frontend data grid. I will be very happy to understand any suggestions for the data persistence and I would try to incorporate that in the next release.

In the real world, EXCEL, when you download a spreadsheet you must be careful with Macros, etc. So, the solution was not to get rid of macros but give users options to allow understand and deal with the risks, otherwise, Excel would lose a lot of part of their designed capabilities, and this is depending on how trustful is your environment.

I am very committed to bringing the Jexcel to one of the best data grids currently available. So, those are the actions I would be working with.

1. Create a flag to enable those who want to sanitize all HTML input native;
2. Automatic sanitize any input formula usage to block elements potentially harmful.
3. Take a look at the persistence options to create helpers to developers for data persistence.

If you have any further suggestions on the actions, are very welcome.

Regards,

[pphod]

The changes are going to be deployed during the weekend, as of version v4.3.0.

[DocBouCou]

Hi,

Thank you for your answer and your concern. I have seen your answer yesterday, and gave some thoughts about it during night.

I think that, for column-type: text, what you might do is simply something like that : (in my opinion, if the colum-type is text, not html, it should be sanitized by default)

      let tmp_div = document.createElement("div");
      tmp_div.innerHTML = what_the_user_typed; 
      what_the_user_typed = tmp_div.textContent;

If the column-type is html, maybe in the docs it might be suggested to the dev to (a) sanitize html (maybe suggest something like sanitize-html -on npm, never tried it- and provide a code snippet?), (b) to force the user to write through a rich text editor or (c) be aware of the risks.

Concerning formulas, maybe split '(', sanitize what is beetween '(' and ')' to keep numbers and a few mathematical operators and no more than two (or three) consecutive letters (2 means that you may have columns up to "ZZ", 676 col is ok for most users I guess?); outside parenthesis, keep only words which are the allowed formulas, doing that -then eval- a parenthesis at a time...
Maybe add another option, true by default?, to allow only proposed formulas, and disable advanced functionalities (so, something like disable macros in excell). If this new option is false, devs should be aware of the risks, and I think it may be as it is now?

It is going to be a good, important, update! :)
Regards,

[pphod]

There are two new flags to deal with security.
stripHTML: true // will strip all HTML except when the column is the HTML type. Default: true
secureFormulas: true // will deal only with uppercase formulas/variables. Default: true

Those are just the first step to bring more security to the table when dealing with persistence. It is important never to trust any data that will be sent to the server-side.

And more things will be brought soon.

Available on v4.2+

[MartinDawson]

@paulhodel

Hi Paul,

How does converting the formulas to uppercase provide more security for the formulas? I'm just curious to try and protect myself from any potential xss.

[joaovmvini]

@paulhodel

Hi Paul,

How does converting the formulas to uppercase provide more security for the formulas? I'm just curious to try and protect myself from any potential xss.

JS is case sensitive, using capital letters it will be familiar to excel methods but not for js. as an example, if a malicious user persists for example=fetch(something) the fetch object exists, but the "FETCH" object does not exist.

[MartinDawson]

@joaovmvini Got it. Thanks.

I'll just santise it on my server then in and out of the database instead.