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

[Security] Stored XSS in main page #1358

Closed
edoardottt opened this issue Jan 28, 2023 · 10 comments · Fixed by #1359
Closed

[Security] Stored XSS in main page #1358

edoardottt opened this issue Jan 28, 2023 · 10 comments · Fixed by #1359
Assignees
Labels
bug Something isn't working security

Comments

@edoardottt
Copy link

edoardottt commented Jan 28, 2023

Describe the bug
It's possible to inject arbitrary Javascript code in the main page of changedetection.io. This can result in a stored cross site scripting attack. Since in /settings#api it's exposed the plaintext API Key, the attacker can read also the api key with an XSS attack.

Version
I'm using v0.39.20.4, but I'm sure other version could be affected as well.

To Reproduce

Steps to reproduce the behavior:

  1. Go to the main page.
  2. Under Add a new change detection watch add as URL javascript:alert(document.domain)
  3. Click Watch
  4. A new row is added under the websites watched
  5. Click CTRL+ click with mouse on the link taking to a new tab
  6. Javascript payload is being executed.

Reproduce the vulnerability with https://changedetection.io/share/LpbICKx5Rbca

Expected behavior
javascript protocol should be blocked like file:// for security reasons.

Screenshots
Screenshot from 2023-01-28 23-10-19

Desktop (please complete the following information):

  • OS: Linux edoardottt 5.19.0-29-generic
  • Browser: Chrome Version 109.0.5414.119 (Official Build) (64-bit)
  • Changedetetion.io Version: v0.39.20.4
@dgtlmoon
Copy link
Owner

Hey thanks! yeah interesting, If i just click on the link, I get about:blank#blocked but if I ctrl+click it will run the JS

@edoardottt
Copy link
Author

edoardottt commented Jan 28, 2023

Yeah I know. That's why I specified that thing. It's a security measure taken by the modern browsers, but if you use an older one it will execute normally. (Even if I normally click it with ctrl pressed 🤧)

Can you give some info on the versions affected? You are the one knowing the entire codebase :P

@dgtlmoon
Copy link
Owner

Hmm i'm wondering if a better future-proof fix is to only allow ^http(s)+/ links by default

@edoardottt
Copy link
Author

Strongly agreed.

@dgtlmoon
Copy link
Owner

dgtlmoon commented Jan 28, 2023

@edoardottt whats your thoughts on #1359 ? I guess this is kinda mitigated by the fact that you'de need to ctrl+click on it

@dgtlmoon dgtlmoon added bug Something isn't working security and removed triage labels Jan 28, 2023
@dgtlmoon
Copy link
Owner

dgtlmoon commented Jan 29, 2023

@edoardottt this is further mitigated by the 'share' receiving server now checking for 'javascript:' , so it wont be possible to share that kind of URL, so you'de have to paste that URL in (which is possible I guess)

https://changedetection.io/share/LpbICKx5Rbca was deleted from the share database

@edoardottt
Copy link
Author

I can confirm the share feature is behaving correctly handling javascript protocol (imo it was the best option to deliver the attack payload). For the other mitigations it seems okay 👍

@edoardottt
Copy link
Author

For the sake of completeness, this attack can be carried out also using the Import feature with this payload:

{
   "client":{
      "local":1
   },
   "data":[
      {
         "name":"javascript:alert(document.domain)",
         "uri":"javascript:alert(document.domain)",
         "config":"{\"selections\":[]}",
         "tags":[]
      }
   ]
}

but now it looks fixed

@edoardottt
Copy link
Author

edoardottt commented Jan 29, 2023

Another idea, since in http://BASE_URL/settings#api we have plaintext API Key, the attacker can read also the api key with a XSS attack.

@dgtlmoon
Copy link
Owner

go easy on the emoticons :) yeah but that should be resolved on that PR, I'll merge it in and make a release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
None yet
2 participants