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

Emoticons re-arm escaped HTML. #192

Closed
cburschka opened this issue May 4, 2015 · 1 comment
Closed

Emoticons re-arm escaped HTML. #192

cburschka opened this issue May 4, 2015 · 1 comment
Assignees
Milestone

Comments

@cburschka
Copy link
Owner

Reproduce

Enter:

This is a <code>Test</code>.

Get

This is a <code>Test</code>.

Enter:

This is a <code>Test</code>. :P

Get

This is a Test. razz

Analysis

This is a somewhat devious side-effect of the jQuery replaceText module, and would probably do well in an Underhanded JavaScript contest.

  1. When jQuery parses the original HTML, any character entities are unescaped inside the text nodes. This is by design - the entities are only intended to armor meta-characters in the serialized HTML code.
  2. Inserting emoticon images via replaceText() requires those images to be entered as HTML which is then evaluated.
  3. jquery.replaceText evaluates the WHOLE text node, including any (unescaped, but not evaluated) HTML code that may already be present.

Solution

Any replaceText(search, replace, textonly) call that does not set textonly to true MUST first be preceded by a replaceText() call that escapes '<' and '&', and then followed by a third replaceText() call that DOES set textonly to true while unescaping '<' and '&'. This is the only way to guarantee that only the newly added HTML code will be evaluated.

Security considerations

It appears possible to execute arbitrary Javascript. This is bad.

@cburschka cburschka self-assigned this May 4, 2015
@cburschka cburschka added this to the cadence 1.8 milestone May 4, 2015
@cburschka
Copy link
Owner Author

The following patch closes this security flaw:

diff --git a/js/core/visual.js b/js/core/visual.js
index c8969c9..a426e4d 100644
--- a/js/core/visual.js
+++ b/js/core/visual.js
@@ -261,13 +261,15 @@ visual = {
             + config.markup.emoticons[set].codes[code]
             + '" title="' + code + '" alt="' + code + '" /><span class="emote-alt">' + code + '</span>';
     }
-    jq.add('*', jq).not('code, code *, a, a *').replaceText(this.emoticonRegex, function() {
+    jq.add('*', jq).not('code, code *, a, a *')
+      .replaceText(/[<&]/g, function(a) { return {'&': '&amp;', '<':'&lt;'}[a]; })
+      .replaceText(this.emoticonRegex, function() {
       for (var i = 1; i < Math.min(arguments.length-2, emoticonSets.length+1); i++) {
         if (arguments[i]) {
           return emoticonImg(emoticonSets[i-1], arguments[i]);
         }
       }
-    });
+    }).replaceText(/&(lt|amp);/g, function(a) { return {'&amp;': '&', '&lt;':'<'}[a]; }, true);
     if (!config.settings.markup.emoticons) {
       jq.find('img.emoticon').css({display:'none'}).next().css({display:'inline'});
     }

cburschka added a commit that referenced this issue May 4, 2015
The emoticon function uses jQuery.replaceText,
which evaluates all unescaped HTML in all text nodes.

To ensure that only the emoticon images are evaluated,
HTML code must first be re-escaped and then un-escaped
without evaluation.

(backported hotfix for 1.7)
cburschka added a commit that referenced this issue May 8, 2015
"Adagio"
========

This update adds several major features and fixes many bugs, including a
security issue.

Critical bugs
-------------

- #192, #196: An arbitrary HTML/Javascript injection vulnerability was removed.
              The emoticon and URL-link processor used the obsolete
              jquery-replacetext library, which implicitly evaluated any escaped
              HTML code in messages as a side effect. The library has been
              removed and replaced with a more robust approach.

Features
--------

- #101: Rooms can be configured via arguments for /create or the newly added
        /configure command. This allows toggling room persistence, logging,
        permissions and other settings.

- #103: Direct messages can be sent and received via the /dmsg command. This
        allows sending a message directly to any JID (including across domains),
        and even to a specific client session identified by its resource.

- #167: Desktop notifications (if the browser and desktop environment support
        them) can be enabled for certain events when the client is in the
        background.

- #178: A list of navigational links can be inserted in the title bar.

- #182: The new /dnd command sets your status to "do not disturb", which turns
        off sounds and notifications, while also showing up as a status icon in
        the roster.

Bugs
----

- #142: All BBCode buttons now have shortcuts supported by most browsers,
        including Chromium-based ones that didn't work previously. These use
        the HTML5 accesskey attribute. The specific key combination may vary
        accross browsers, and is shown in the button tooltips.

- #166: The full RGB color selection now works reliably, by adding an "Advanced"
        button to the palette dialog that opens the color picker.

- #186: Disabling inline images now actually works, instead of showing both the
        image and the alt-text.

- #189: Emoticons will no longer render inside hyperlink anchors, which avoids
        rendering 8o or :D in URLs.

Minor fixes
-----------

- #179: Alphabetize /who output.
- #188: Some incorrect labels in the settings form have been fixed.
- #190: Open all links in new tab.
- #191: The text color RGB code is in monospace; the sliders have a set width.
- #195: JS libraries were updated (strophe, buzz, jquery and moment)
- #197: The codes in the help sidebar now line-wrap.
- #198: Some unused images have been removed, one image was renamed.
- #202: The stderr output of `which` is now suppressed.
- #203: The room title is no longer run through Strophe.unescapeNode
- #205: The onclick event for users is now attached instead of injected in HTML.
- #206: Private messages can now be sent to nicks that contain backslashes.
- #207: Pressing Escape or Cancel on the URL prompt no longer inserts [url][/url].
- #209: Make the recipient clickable on outgoing /msg and /dmsg whispers.
- #210: Some control flow refactoring.
cburschka added a commit to calref/cadence that referenced this issue May 9, 2015
"Adagio"
========

This update adds several major features and fixes many bugs, including a
security issue.

Critical bugs
-------------

- cburschka#192, cburschka#196: An arbitrary HTML/Javascript injection vulnerability was removed.
              The emoticon and URL-link processor used the obsolete
              jquery-replacetext library, which implicitly evaluated any escaped
              HTML code in messages as a side effect. The library has been
              removed and replaced with a more robust approach.

Features
--------

- cburschka#101: Rooms can be configured via arguments for /create or the newly added
        /configure command. This allows toggling room persistence, logging,
        permissions and other settings.

- cburschka#103: Direct messages can be sent and received via the /dmsg command. This
        allows sending a message directly to any JID (including across domains),
        and even to a specific client session identified by its resource.

- cburschka#167: Desktop notifications (if the browser and desktop environment support
        them) can be enabled for certain events when the client is in the
        background.

- cburschka#178: A list of navigational links can be inserted in the title bar.

- cburschka#180: Six new pony emotes have been added to the pack:
        :fluttersmile:, :ididntlisten:, :rdmad:, :shutup:, :symadder: :tavizero:

- cburschka#182: The new /dnd command sets your status to "do not disturb", which turns
        off sounds and notifications, while also showing up as a status icon in
        the roster.

Bugs
----

- cburschka#142: All BBCode buttons now have shortcuts supported by most browsers,
        including Chromium-based ones that didn't work previously. These use
        the HTML5 accesskey attribute. The specific key combination may vary
        accross browsers, and is shown in the button tooltips.

- cburschka#166: The full RGB color selection now works reliably, by adding an "Advanced"
        button to the palette dialog that opens the color picker.

- cburschka#186: Disabling inline images now actually works, instead of showing both the
        image and the alt-text.

- cburschka#189: Emoticons will no longer render inside hyperlink anchors, which avoids
        rendering 8o or :D in URLs.

Minor fixes
-----------

- cburschka#179: Alphabetize /who output.
- cburschka#188: Some incorrect labels in the settings form have been fixed.
- cburschka#190: Open all links in new tab.
- cburschka#191: The text color RGB code is in monospace; the sliders have a set width.
- cburschka#195: JS libraries were updated (strophe, buzz, jquery and moment)
- cburschka#197: The codes in the help sidebar now line-wrap.
- cburschka#198: Some unused images have been removed, one image was renamed.
- cburschka#202: The stderr output of `which` is now suppressed.
- cburschka#203: The room title is no longer run through Strophe.unescapeNode
- cburschka#205: The onclick event for users is now attached instead of injected in HTML.
- cburschka#206: Private messages can now be sent to nicks that contain backslashes.
- cburschka#207: Pressing Escape or Cancel on the URL prompt no longer inserts [url][/url].
- cburschka#209: Make the recipient clickable on outgoing /msg and /dmsg whispers.
- cburschka#210: Some control flow refactoring.

Conflicts:
	VERSION
	css/alt/dash/bolt.png
	img/lagging.png
	img/typing.png
	js/core/visual.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant