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 in data-target attribute #20184

Closed
lpilorz opened this issue Jun 27, 2016 · 43 comments
Closed

XSS in data-target attribute #20184

lpilorz opened this issue Jun 27, 2016 · 43 comments
Labels
js v3 v4

Comments

@lpilorz
Copy link

@lpilorz lpilorz commented Jun 27, 2016

The data-target attribute is vulnerable to Cross-Site Scripting attacks:

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.2.4/jquery.min.js"></script>
<script src="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/js/bootstrap.min.js"></script>
<button data-toggle="collapse" data-target="<img src=x onerror=alert(0)>">Test</button>
@cvrebert cvrebert added js v3 labels Jun 27, 2016
@lpilorz
Copy link
Author

@lpilorz lpilorz commented Jun 28, 2016

I guess the fix for this specific issue could be changing getTargetFromTrigger function:

return $(target)

to:

return $(document.querySelector(target))

but it seems the same problem is present in other places too.

Here is another example:

<a href="<img src=x onerror=alert(0)>" data-dismiss="alert">Test</a>
@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Jun 29, 2016

@cvrebert: how do you think we should address this?

@cvrebert
Copy link
Collaborator

@cvrebert cvrebert commented Jun 29, 2016

return $(document.querySelector(target))

I don't think that's a viable option compatibility-wise. Bootstrap 3 supports IE8, and its CSS selector support differs from jQuery's: http://caniuse.com/#feat=queryselector

Based on https://bugs.jquery.com/ticket/11290, I guess we could try something like:

if (/\s*</.test(target)) {
  return $()
}

@dmethvin Sorry to trouble you, but does this sound like a good workaround?

(It's a shame jQuery doesn't have an "only interpret this as a selector, never as HTML" API.)

@dmethvin
Copy link
Contributor

@dmethvin dmethvin commented Jun 29, 2016

You could use $(document).find(target) instead, that won't be interpreted as HTML.

How is this cross-site? Where does the HTML come from and how does the attacker control it?

@lpilorz
Copy link
Author

@lpilorz lpilorz commented Jun 29, 2016

This was found in an application where data-target was based on user input and only passed through standard HTML entities encoding. There is no reason why data-target should interpret HTML so while not impacting many applications it should be fixed in my opinion.

@cvrebert cvrebert added this to the v3.3.7 milestone Jul 12, 2016
@cvrebert cvrebert modified the milestones: v3.3.7, v3.3.8 Jul 25, 2016
@mdo
Copy link
Member

@mdo mdo commented Sep 5, 2016

Bootstrap 3 is no longer being officially developed or supported.

All work has moved onto our next major release, v4. As such, this issue or pull request is being closed as a "won't fix." For additional help and support, we recommend utilizing our community resources. Thanks for your understanding, and see you on the other side of v4!

<3,
@mdo and team

@mdo mdo closed this Sep 5, 2016
@mdo mdo modified the milestone: v3.3.8 Sep 6, 2016
@skokkanthi
Copy link

@skokkanthi skokkanthi commented Jan 17, 2017

Dear @mdo @cvrebert is this issue fixed in V4? Please let me know. If yes, can you please give me the commit related to this.

pvdlg added a commit to pvdlg/bootstrap that referenced this issue Jan 17, 2017
pvdlg added a commit to pvdlg/bootstrap that referenced this issue Jan 27, 2017
* collapse-multiple-target:
  Use $(document).find(selector) to avoid case in twbs#20184
  Muti-target support for collapse plugin
  make getTargets to always return a JQuery to avoid calling JQuery on the same element further down
  Add a dropdown test case for twbs#21328
  Simplify targets.length test
  Simplify null check when possible
  Rework getSelectorFromElement to not rely on regex

# Conflicts:
#	js/src/alert.js
#	js/src/dropdown.js
#	js/tests/unit/collapse.js
@meeque
Copy link
Contributor

@meeque meeque commented Aug 22, 2017

I know this one's closed, but what's the status for v4? I can see that it is still affected by this vulnerability:

https://github.com/twbs/bootstrap/blob/v4-dev/js/src/util.js#L120

I recommend to mitigate it by applying @dmethvin's proposal. (Also see comits on @vanduynslagerp's fork.)

Should I prepare a pull request? Or should I open a new issue for v4?

@XhmikosR XhmikosR reopened this Aug 22, 2017
@Johann-S
Copy link
Member

@Johann-S Johann-S commented Aug 22, 2017

Yes you can make a PR @meeque for this issue which is still present 👍

@Johann-S Johann-S added the v4 label Aug 22, 2017
@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Aug 22, 2017

You could make a PR against v3-dev branch too if you want.

@meeque
Copy link
Contributor

@meeque meeque commented Aug 25, 2017

Let's start off with a test case: http://jsbin.com/qalekeroke/edit?html,output

meeque added a commit to meeque/bootstrap that referenced this issue Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet