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.

Sign up for GitHub

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

Jump to bottom

Cross Site Scripting #564

Closed
Skileau opened this issue Sep 3, 2024 · 17 comments
Closed

Cross Site Scripting #564

Skileau opened this issue Sep 3, 2024 · 17 comments

Comments

@Skileau
Copy link

Skileau commented Sep 3, 2024

Describe the bug
I have found a Cross Site Scripting in the library, where should I submit the PoC?
@brianvoe
Copy link
Owner

brianvoe commented Sep 3, 2024

here

@Skileau
Copy link
Author

Skileau commented Sep 4, 2024

Because of the use of innerHTML here instead of innerText, it is possible to inject HTML which is interpreted:
https://github.com/brianvoe/slim-select/blob/master/src/slim-select/select.ts#L345

I used the following HTML page as an example:

<html>
  <head>
    <script src="https://unpkg.com/slim-select@latest/dist/slimselect.min.js"></script>
    <link href="https://unpkg.com/slim-select@latest/dist/slimselect.css" rel="stylesheet"></link>
  </head>
  <body>
    <select id="selectElement">
      <option>&lt;IMG SRC=X ONERROR=alert(2)&gt;&lt;/IMG&gt;</option>
      <option>&#x3c;&#x69;&#x6d;&#x67;&#x20;&#x73;&#x72;&#x63;&#x3d;&#x78;&#x20;&#x6f;&#x6e;&#x65;&#x72;&#x72;&#x6f;&#x72;&#x3d;&#x61;&#x6c;&#x65;&#x72;&#x74;&#x28;&#x31;&#x29;&#x3e;&#x3c;&#x2f;&#x69;&#x6d;&#x67;&#x3e;</option>
      <option>Option 3</option>
    </select>
    <script>
      new SlimSelect({
        select: '#selectElement'
      })
    </script>
  </body>
</html>

It is important to note that the payload is HTML encoded.

Changing the selected option on the page leads to the execution of JavaScript:
image

@brianvoe
Copy link
Owner

brianvoe commented Sep 4, 2024

Sorry i just dont see this as a slim select issue.

@brianvoe brianvoe closed this as completed Sep 4, 2024
@brianvoe
Copy link
Owner

@Skileau You told another company about this?

@Skileau
Copy link
Author

Skileau commented Sep 23, 2024

Yes, I reported it during a pentest as it's my job and the usual process, and reached out to you so we can secure this issue upstream.

@brianvoe
Copy link
Owner

Your letting someone submit XSS attacks through your input fields, save them into the database, serve them up to another user, inject it into a ui select dropdown and want to say the ui select dropdown is the issue?

@brianvoe
Copy link
Owner

If you have a fix for this that doesn't effect how things currently work. Submit a pr

@Skileau
Copy link
Author

Skileau commented Sep 24, 2024

Is there a specific reason why you are using innerHTML rather than innerText here ?
https://github.com/brianvoe/slim-select/blob/master/src/slim-select/select.ts#L377C14-L377C23

@brianvoe
Copy link
Owner

Nope, dont care what value it is as long as it works. If you have a fix for this that doesn't effect how things currently work. Submit a pr

@Skileau
Copy link
Author

Skileau commented Sep 27, 2024 •
edited
Loading

I am not a developer so I would not be able to provide you with a fix and confirm that it would not break anything, sorry.
I can simply confirm you that the part of the code that I exploited to get an XSS is this specific innerHTML:
https://github.com/brianvoe/slim-select/blob/master/src/slim-select/select.ts#L377C14-L377C23

You can try to replace it with an innerText and build the application on your side to make sure that it works well.
As previously said, here is the index.html that I used for a small PoC:

<html>
  <head>
    <script src="https://unpkg.com/slim-select@latest/dist/slimselect.min.js"></script>
    <link href="https://unpkg.com/slim-select@latest/dist/slimselect.css" rel="stylesheet"></link>
  </head>
  <body>
    <select id="selectElement">
      <option>&lt;IMG SRC=X ONERROR=alert(1)&gt;&lt;/IMG&gt;</option>
      <option>Option 2</option>
      <option>Option 3</option>
    </select>
    <script>
new SlimSelect({
  select: '#selectElement',
})
    </script>
  </body>
</html>

As long as once you change the selection and it does not trigger the alert(1), then the reported vulnerability can be considered as fixed.

Also notice that this example is provided as a PoC but I exploited this vulnerability in a global application during a security assessment to elevate my privileges to super admin as I was able to inject a malicious payload into one of the list. Even though it was their choice to let users create new options, I think that this is a frequent use case and it cannot be ignored ...
Especially as there doesn't seem to be any real point in converting the option's innerText into HTML, so the fix is unlikely to break any feature.

@brianvoe
Copy link
Owner

"I am not a developer" - ok thanks have a good one

@shelbyc shelbyc mentioned this issue Oct 8, 2024
Merged
@Zerotask
Copy link

Zerotask commented Oct 9, 2024 •
edited
Loading

Dependabot also warned us about a potential risk for slimselect:
https://nvd.nist.gov/vuln/detail/CVE-2024-9440

Slim Select 2.0 versions through 2.9.0 are affected by a potential cross-site scripting vulnerability. In select.ts:createOption(), the text variable from the user-provided Options object is assigned to an innerHTML without sanitation. Software that depends on this library to dynamically generate lists using unsanitized user-provided input may be vulnerable to cross-site scripting, resulting in attacker executed JavaScript. At this time, no patch is available.

@Shoplifter
Copy link

I can't see any reason why innerText should not be used here instead of innerHTML.
In contrast: In an option tag only text is permitted anyways
see: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/option#technical_summary

@brianvoe
Copy link
Owner

brianvoe commented Oct 9, 2024

update the code and add a test. submit a pr and ill get it in.

@Shoplifter Shoplifter mentioned this issue Oct 11, 2024
Merged
@Shoplifter
Copy link

Shoplifter commented Oct 11, 2024 •
edited
Loading

Hi @brianvoe
I just submitted a PR for that. Don't know what you mean with "add a test"
I think the update should be already covered by all the tests that use the select.createOption method

UPDATE: the original PR did not pass the test suite. I changed innerText to textContent.
Also added a test for this and now all tests pass.

brianvoe added a commit that referenced this issue Oct 11, 2024
@brianvoe
Merge pull request #572 from Shoplifter/master
f8534f2 
patch for #564: Use textContent instead of innerHTML for setting an option's text
@brianvoe
Copy link
Owner

v2.9.2

@frenkel
Copy link
Contributor

frenkel commented Oct 16, 2024

Thanks for fixing this @Shoplifter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@frenkel @Shoplifter @brianvoe @Zerotask @Skileau