Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XSS Vulnerability in ScenesController.js #3367

Closed
Frige1 opened this issue Jul 1, 2019 · 23 comments
Closed

XSS Vulnerability in ScenesController.js #3367

Frige1 opened this issue Jul 1, 2019 · 23 comments

Comments

@Frige1
Copy link

Frige1 commented Jul 1, 2019

Description

The ScenesController.js is vulnerable against XSS. By adding a scene with some scripts in the scene name will lead to a XSS.
Obviously this is thankful for an attacker if he is privilege to adding or edit scenes. The attacker can execute arbitrary code.

Affected Version

Current release 4.10717

Steps to Reproduce

  1. Clone and compile it. I follow the instructions from the wiki: https://www.domoticz.com/wiki/Raspberry_Pi_-_Build_Domoticz_from_source#Raspberry_Pi_-_Build_Domoticz_from_source
  2. Run domoticz ./domoticz
  3. Open http://localhost:8080/#/Scenes in your browser.
  4. Click button: Add Scene
  5. Have fun to implement arbitrary scripts or even test it with <script>alert("XSS");</script>
    XSS-Vul

Fix

Some escape() is used in the ScenesController.js, but this XSS is still present. Escaping the item.name fix the XSS. Furthermore use encodeURI() instead of escape(). The escape() function was deprecated in JavaScript v1.5.

XSS-Fix

See PR #3368

@gizmocuz
Copy link
Contributor

gizmocuz commented Jul 3, 2019

Thanks for the PR.
Your current PR causes all scene names that includes for example spaces to be displayed with %20 in the name
Also names containing non-latin characters to display wrongly.

It also seems not possible to change a name to a non-latin name

Is there another solution ?

@Frige1
Copy link
Author

Frige1 commented Jul 4, 2019

Well, you're right. My focus was to (quick) fix the XSS-Vulnerability.

Since now, i used encodeURI(), which escape not that much chars like encodeURIComponent(). Especially because (as you mentioned) some chars will be displayed as escaped chars.

Browsing on OWASP sites, i found a library for special XSS escaping:
https://www.owasp.org/index.php/Category:OWASP_Encoding_Project

Unfortunately I have not enough free-time space, to fix the XSS with propper presentation for the item.name . Maybe on a later point in time.

I really recommend to you / the community to rewrite the hole mechanism of adding or let's say at least of displaying scenes in a safe way with escaping + good presentation for the name.

@gizmocuz
Copy link
Contributor

gizmocuz commented Jul 5, 2019

@Frige1 , sure, but as this is a open source project, it would be great if you could contribute with a patch that don't break functionality

@Frige1
Copy link
Author

Frige1 commented Jul 5, 2019

Yeah you're right.

I try to find some time till Sunday/Monday.

@gizmocuz
Copy link
Contributor

gizmocuz commented Jul 5, 2019

Thanks , much appreciated !

@Frige1
Copy link
Author

Frige1 commented Jul 8, 2019

I tried to understand the software better. Unfortunately, I cannot find any code documentation, so I had to reverse. Correct me, if there is some code documentation.

I found some decoding methods for JSON and i believe it works like this (correct me please):

  1. encoded Request
  2. decoded via some functions (in JSON- or Lua- or on the websever- helpers?
  3. If the request point to an Insert-DB, the values of the request will saved as decoded values for example "<script>alert("xss")</script>"
  4. If now someone read from the DB it will always be an XSS right?

This is a problem not only for the ScenesController.js, this leads to a problem for the hole Webapp.

You can test it, if you add a scene with the mentioned script. And open "localhost:8080/#/Devices"

Well, I did not check all inputfields of forms, but i guess all inputfields can lead to an XSS, weather they got encoded/escaped or not.

Last but not least:
I could fix the Problem of this specific field item.name with good representation. But this would not fix any other possible XSS in the Webapp.

@gizmocuz
Copy link
Contributor

gizmocuz commented Jul 8, 2019

Reading and writing from and to the database is done in c++, no XSS there

But reading and writing in the GUI is done via JavaScript
There could be the xss

@Frige1
Copy link
Author

Frige1 commented Jul 8, 2019

Obviously, there is no XSS in c++.

So parsing HTML with no escaping/encoding for variables with input from DB will always be an XSS in JS.

@gizmocuz
Copy link
Contributor

gizmocuz commented Jul 8, 2019

Yep we are on the same line

@Frige1
Copy link
Author

Frige1 commented Jul 8, 2019

Oh well, this is a critical problem for the hole webapp.

I see two options so far:

  1. Encode responses or writing from c++ to "JSON-GUI" holders.
  2. Rewrite all *.js controllers, so there is no HTML parsing anymore.

Looking forward for more ideas.

@gizmocuz
Copy link
Contributor

gizmocuz commented Jul 9, 2019

@Frige1 , i think it is best to keep the JSON clean.
This because the JSON output is also used for mobile applications and other developers use this as well.
Then it might be best to update the .js controllers ?

@Frige1
Copy link
Author

Frige1 commented Jul 9, 2019

Keep in mind, this will result in a persistent-XSS vulnerability. The attacker can store and will try spreading scripts in any place he can on the DB. At any time, someone write code for the frontend and miss script handling, will lead again to a XSS.

I prefer escaped data through the frontend over the backend. And mobile applications, let's say android or iOS can decode json data. I cannot test it for iOS, but for android (mainly Java) i can say, there exist decode functions.

@gizmocuz
Copy link
Contributor

I think we should keep je JSON output as it is... who knows what we will add in the feature
But how about this change:

abcd817#diff-d770ada4194786ba884ee2979332f94a

This should prevent XSS attacks in the ScenesController

Basically by Adding/Updating a Scene/Devices/... , you purify the Name with

DOMPurify.sanitize(Name)

@rwaaren
Copy link
Contributor

rwaaren commented Jul 10, 2019

Does this also prevent the same type of script injection in levelnames of a selector device or are more code changes required ?

@gizmocuz
Copy link
Contributor

Okey, on second thought, it might be better to do this indeed in the backend in the add/update xx functions...

@gizmocuz
Copy link
Contributor

@rwaaren , a lot of code changes are required... but I will try to do this in the backend... so when someone enters a malicious name, it will be sanitized
It will be quite some work...

@Frige1
Copy link
Author

Frige1 commented Jul 10, 2019

@gizmocuz I tried this library and it looks good as a fix for this specific controller.js. But as you already mentioned, it should be better to clean up the stored/persistent XSS vulnerability as still exists.

If I get more free time, I will continue my training/learning "angular". Maybe i find enough time to rewrite the frontend from JS controllers, to angular (TypeScript) as frontend. But i can promise nothing.

One of the purpose of angular is the cross-site scripting security model. They view all inputs as untrusted by default. https://angular.io/guide/security

Unless the community still wants to use javascript for the controllers and frontend.

@gizmocuz
Copy link
Contributor

@Frige1 , For testing I added some code to Sanitize text in the C++ code.
It is build in currently for the Add/Update Scene commands in beta 10982

Currently it will strip tags ( between < and > ) containing the following text:

	"script", //noscript/onbeforescriptexecute
	"style",
	"svg",
	"audio",
	"video",
	"head",
	"math",
	"template",
	"form", //formaction
	"input", //oninput
	"onerror",
	"frame", //iframe/noframe/frameset
	"img",
	"marquee",
	"applet",
	"object",
	"embed",
	"math",
	"href",
	"onfocus",
	"onresize",
	"onactivate",
	"onscroll",
	"onwebkittransitionend",
	"onanimationstart",
	"ontoggle",
	"details",
	"table",

for example "Hello<script>bladiebla< / script >" will be stripped down to "Hellobladiebla"

Is it possible you could test this ? After this i will add this check in all add/update functions (lights/utilty etc anything stay stores names in Domoticz via the web interface)

@gizmocuz
Copy link
Contributor

@Frige1 , a lot of GUI code is already in AngularJs and therefore probably not vulnerable

@Frige1
Copy link
Author

Frige1 commented Jul 10, 2019

Yeah i try to test it. Could you refer the commit, so i get an idea of the added code?

@gizmocuz
Copy link
Contributor

gizmocuz commented Jul 12, 2019

@Frige1 , this is the commit:

bc853c7

@gizmocuz
Copy link
Contributor

@Frige1 , and for the remaining: c915110

@gizmocuz
Copy link
Contributor

Closing issue, should be resolved. Please feel free to continue if it's not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants