[BUG] "&" gets translated to "&" when creating new company

[eeintech]

Please verify that this bug has NOT been raised before.

• I checked and didn't find similar issue

Describe the bug*

See title

Steps to Reproduce

1. Create new company
2. Use "&" symbol in the name
3. Click submit
4. Name shows "&" in the name instead of "&"

Expected behavior

"&" should stay as-is

Deployment Method

• Docker
• Bare metal

Version Information

InvenTree-Version: 0.9.0 dev
Django Version: 3.2.15
Commit Hash: 964d674
Commit Date: 2022-08-08
Database: postgresql
Debug-Mode: False
Deployed using Docker: True
Active plugins: False

Relevant log output

No response

[matmair]

@eeintech this is a security measure to protect against CSRF/XSS as the symbol can be used in a scripting context. I am afraid it will have to stay that way as changing it would make us vulnerable again.
Ref: GHSA-rm89-9g65-4ffr

[matmair]

Also please add the version information to all bug reports. The demo is a moving target

[eeintech]

I see... not ideal, some company names are using this symbol (and maybe others).

I don't understand how would it make our instances vulnerable for this kind of attack as most (I hope all) are behind firewalls/VPN, can you explain?

A work around would be to display & instead of & using the translation files?
Can an API GET return "&" while POST keep the security fix?

[SchrodingersGat]

@eeintech I get the frustration, the current solution is not ideal, but is a compromise to prevent a malicious user from inserting XSS attack into another users browser.

Data gets sanitized on the server and then again when rendering. This double sanitization step makes the "chain of data" something like:

Black & Decker -> Black & Decker -> Black & Decker

There is probably something that can be done about this, would require a bit of a deep dive to understand all the edge cases. There are multiple places where data gets rendered (in different ways):

• HTML templates
• Via javascript (e.g. API forms)
• tables (using bootstrap-table)

Each would need separate consideration.

[SchrodingersGat]

@matmair I can see the appeal of keeping data "as entered" by the user wherever possible. Having e.g. Black & Decker in the database is wrong and what happens when the user exports data e.g to excel?

I think we should look at augmenting our CleanMixin class somewow:

InvenTree/InvenTree/InvenTree/mixins.py

Line 8 in b0ad326

class CleanMixin():

Interesting thread: mozilla/bleach#192

[SchrodingersGat]

Another issue - the "markdown" notes fields get corrupted if you try to add a "blockquote" > character:

[eeintech]

@SchrodingersGat Thanks for looking into this. What about pre-existing names in 0.7.x instances? Are they all converted for users who upgrade to 0.8.0? Would they create this security issue if not?

Can company added through Admin section be allowed to carry this symbol?

[SchrodingersGat]

I would imagine that (if we can work out a fix here) it would only apply to new fields. Any instances of invalid characters would need to be manually updated.

Can company added through Admin section be allowed to carry this symbol?

Yes, you can adjust it this way. However it will display incorrectly if you ever edit the company again.

[SchrodingersGat]

Useful reference: https://benhoyt.com/writings/dont-sanitize-do-escape/

[eeintech]

Useful reference: https://benhoyt.com/writings/dont-sanitize-do-escape/

This article makes a lot of sense, I would vote to rethink the strategy of sanitizing as it compromises the database data with removing or replacing special chars. Again InvenTree instances are not public facing and I trust the people I work with, I don't see how this solve any security breach if the breach doesn't exist in the first place.

[matmair]

We run a lot of the user input straight into sql queries (through all the Django ORM but the endresult is the same). I discovered this when I reviewed some of the more common attack surfaces.
So escaping is nice but leaves us open to SQL injection and possibly a few cross-server things in the backend too.
As the sanatizing is transparent I do not really see a problem with it. The big enterprise PLM/ERP at work does not even let me use a backtick or ampersand anywhere.

Again InvenTree instances are not public facing

That is not the case. If you know how to it takes only a few minutes. The hardcoded identifier in the base /api path does not help with that

I trust the people I work with, I don't see how this solve any security breach if the breach doesn't exist in the first place.

This is not how modern security works and I hope whoever runs IT/OT sec at your workplace does not think like that. There are always Joiner / Mover in an org that are not really checked. One malicous data entry later you have a permantent shell in the network. Depending on the setup possibly even in the managment plane.
With educational institutions using InvenTree I would be really against removing security features because you trust your coworkers.

The other solution is the just remove the second and third sanitzion layers by marking the strings as safe and adding validations to charfields that stop user from inputting data that will be sanitzied. That is more work but cleaner.
#2789 would solve this issue as proper input validation (before send) is standard.

[eeintech]

So escaping is nice but leaves us open to SQL injection and possibly a few cross-server things in the backend too.

I am a user willing to live with that, what are my options?

All my half-baked arguments before are meant to show that I care about conserving the special chars/symbols and am willing to put our security to risk (this is my own choice and I will not hold InvenTree team responsible for that).
Please tell me how to do it, even if it takes a custom patch to be applied at every update.

Thanks.

[SchrodingersGat]

@eeintech I think it will depend a lot on what we decide on #3503

[matmair]

I am not going to give instruction on how to break essential security measures but maybe some else is willing to.
We can not assume ppl understand the architectural implications of removing XSS protections. You probably do but if the first thing google shows when searching for special characters is an instruction how to break your instances neck ppl will do that asap.

[SchrodingersGat]

am a user willing to live with that

We are not devs willing to support that ;)

[matmair]

@eeintech dont get me wrong, we will find a way to support your case. It will just not be by breaking security.

[eeintech]

@eeintech dont get me wrong, we will find a way to support your case. It will just not be by breaking security.

Great to hear. Btw this is not "my case" but I imagine applies to many users (most of them probably haven't noticed yet), I got word of this in Ki-nTree actions else I would not have stumbled on this anytime soon.

I am not going to give instruction on how to break essential security measures but maybe some else is willing to.
We can not assume ppl understand the architectural implications of removing XSS protections. You probably do but if the first thing google shows when searching for special characters is an instruction how to break your instances neck ppl will do that asap.

I do not understand enough about security to argue, but data integrity is more dear to me than security as I'm an optimistic and believe in people, at least the ones I work with (as we are a tiny company). I know it might be hard to grasp when sitting on the other side...

We are not devs willing to support that ;)

As long as you're not devs willing to compromise the integrity of our databases, I'm fine with that!

[matmair]

The database integrity is fine - there are just additional input rules.

[eeintech]

How is the data integrity fine if you start replacing special chars & or < with 4/5-chars & or < inside parts, company, etc. names? Then it gets way more complicated to find things, scripting, etc.

[eeintech]

On my side #3503 fixes it