paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-10-23 04:52:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

CVE-2020-9444: Prevent reverse tabnabbing attacks.

Browse Source 
While we could fix this issue by changing the markdown processor,
doing so is not a robust solution, because even a momentary bug in the
markdown processor could allow cached messages that do not follow our
security policy.

This change ensures that even if our markdown processor has bugs that
result in rendered content that does not properly follow our policy of
using rel="noopener noreferrer" on links, we'll still do something
reasonable.

Co-authored-by: Tim Abbott <tabbott@zulipchat.com>
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This commit is contained in:
Anders Kaseorg
2020-02-28 14:59:07 -08:00
committed by Tim Abbott
parent b21117954d
commit c9796ba7f7
21 changed files with 96 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
frontend_tests/casper_lib/common.js
View File

@@ -2,6 +2,8 @@ var util = require("util");
var test_credentials = require('../../var/casper/test_credentials.js').test_credentials;
casper.options.clientScripts.push("frontend_tests/casper_lib/polyfill.js");
function timestamp() {
return new Date().getTime();
}

12
frontend_tests/casper_lib/polyfill.js Normal file
View File

@@ -0,0 +1,12 @@
/* eslint-env browser */
// PhantomJS doesn’t support new DOMParser().parseFromString(…, "text/html").
var real_parseFromString = DOMParser.prototype.parseFromString;
DOMParser.prototype.parseFromString = function (string, type) {
if (type === "text/html") {
var doc = document.implementation.createHTMLDocument("");
doc.documentElement.innerHTML = string;
return doc;
}
return real_parseFromString.apply(this, arguments);
};

3
frontend_tests/node_tests/compose.js
View File

@@ -1,3 +1,5 @@
const { JSDOM } = require("jsdom");
set_global('bridge', false);
set_global('blueslip', global.make_zblueslip({
@@ -7,6 +9,7 @@ set_global('blueslip', global.make_zblueslip({
const noop = function () {};
set_global('$', global.make_zjquery());
set_global('DOMParser', new JSDOM().window.DOMParser);
set_global('i18n', global.stub_i18n);
const _navigator = {

3
frontend_tests/node_tests/copy_and_paste.js
View File

@@ -7,8 +7,9 @@ set_global('compose_ui', {});
const { JSDOM } = require("jsdom");
const { window } = new JSDOM('<!DOCTYPE html><p>Hello world</p>');
const { document } = window;
const { DOMParser, document } = window;
set_global('$', require('jquery')(window));
set_global('DOMParser', DOMParser);
set_global('document', document);
const copy_and_paste = zrequire('copy_and_paste');

1
frontend_tests/node_tests/templates.js
View File

@@ -9,6 +9,7 @@ zrequire('stream_edit');
const { JSDOM } = require("jsdom");
const { window } = new JSDOM();
global.$ = require('jquery')(window);
set_global('DOMParser', window.DOMParser);
// When writing these tests, the following command might be helpful:
// ./tools/get-handlebar-vars static/templates/*.hbs

17
frontend_tests/node_tests/util.js
View File

@@ -1,4 +1,7 @@
const { JSDOM } = require("jsdom");
set_global('$', global.make_zjquery());
set_global('DOMParser', new JSDOM().window.DOMParser);
set_global('blueslip', global.make_zblueslip({}));
set_global('document', {});
@@ -301,3 +304,17 @@ run_test('move_array_elements_to_front', () => {
assert(emails_actual[i] === emails_expected[i]);
}
});
run_test("clean_user_content_links", () => {
window.location.href = "http://zulip.zulipdev.com/";
assert.equal(
util.clean_user_content_links(
'<a href="http://example.com">good</a> ' +
'<a href="http://localhost:NNNN">invalid</a> ' +
'<a href="/#fragment" target="_blank">fragment</a>'
),
'<a href="http://example.com" target="_blank" rel="noopener noreferrer">good</a> ' +
'<a>invalid</a> ' +
'<a href="/#fragment">fragment</a>'
);
});

2
static/js/compose.js
View File

@@ -736,7 +736,7 @@ exports.render_and_show_preview = function (preview_spinner, preview_content_box
rendered_preview_html = rendered_content;
}
preview_content_box.html(rendered_preview_html);
preview_content_box.html(util.clean_user_content_links(rendered_preview_html));
if (page_params.emojiset === "text") {
preview_content_box.find(".emoji").replaceWith(function () {
const text = $(this).attr("title");

9
static/js/stream_edit.js
View File

@@ -1,3 +1,4 @@
const util = require("./util");
const render_settings_deactivation_stream_modal = require("../templates/settings/deactivation_stream_modal.hbs");
const render_stream_member_list_entry = require('../templates/stream_member_list_entry.hbs');
const render_subscription_settings = require('../templates/subscription_settings.hbs');
@@ -111,7 +112,9 @@ exports.update_stream_name = function (sub, new_name) {
exports.update_stream_description = function (sub) {
const stream_settings = exports.settings_for_sub(sub);
stream_settings.find('input.description').val(sub.description);
stream_settings.find('.stream-description-editable').html(sub.rendered_description);
stream_settings.find('.stream-description-editable').html(
util.clean_user_content_links(sub.rendered_description)
);
};
exports.invite_user_to_stream = function (user_email, sub, success, failure) {
@@ -458,7 +461,9 @@ exports.change_stream_description = function (e) {
$(".stream_change_property_info"));
},
error: function (xhr) {
sub_settings.find('.stream-description-editable').html(sub.rendered_description);
sub_settings.find('.stream-description-editable').html(
util.clean_user_content_links(sub.rendered_description)
);
ui_report.error(i18n.t("Error"), xhr, $(".stream_change_property_info"));
},
});

3
static/js/subs.js
View File

@@ -1,3 +1,4 @@
const util = require('./util');
const render_subscription = require('../templates/subscription.hbs');
const render_subscription_settings = require('../templates/subscription_settings.hbs');
const render_subscription_table_body = require('../templates/subscription_table_body.hbs');
@@ -162,7 +163,7 @@ exports.update_stream_description = function (sub, description, rendered_descrip
// Update stream row
const sub_row = exports.row_for_stream_id(sub.stream_id);
sub_row.find(".description").html(sub.rendered_description);
sub_row.find(".description").html(util.clean_user_content_links(sub.rendered_description));
// Update stream settings
stream_edit.update_stream_description(sub);

7
static/js/templates.js
View File

@@ -1,3 +1,5 @@
const util = require("./util");
// Below, we register Zulip-specific extensions to the handlebars API.
//
// IMPORTANT: When adding a new handlebars helper, update the
@@ -73,4 +75,9 @@ Handlebars.registerHelper('tr', function (context, options) {
return new Handlebars.SafeString(result);
});
Handlebars.registerHelper(
"rendered_markdown",
content => new Handlebars.SafeString(util.clean_user_content_links(content))
);
window.templates = exports;

32
static/js/util.js
View File

@@ -347,4 +347,36 @@ exports.convert_message_topic = function (message) {
}
};
exports.clean_user_content_links = function (html) {
const content = new DOMParser().parseFromString(html, "text/html").body;
for (const elt of content.getElementsByTagName("a")) {
// Ensure that all external links have target="_blank"
// rel="opener noreferrer". This ensures that external links
// never replace the Zulip webapp while also protecting
// against reverse tabnapping attacks, without relying on the
// correctness of how Zulip's markdown processor generates links.
//
// Fragment links, which we intend to only open within the
// Zulip webapp using our hashchange system, do not require
// these attributes.
let url;
try {
url = new URL(elt.getAttribute("href"), window.location.href);
} catch {
elt.removeAttribute("href");
continue;
}
// We detect URLs that are just fragments by comparing the URL
// against a new URL generated using only the hash.
if (url.hash === "" || url.href !== new URL(url.hash, window.location.href).href) {
elt.setAttribute("target", "_blank");
elt.setAttribute("rel", "noopener noreferrer");
} else {
elt.removeAttribute("target");
}
}
return content.innerHTML;
};
window.util = exports;

2
static/templates/draft.hbs
View File

@@ -36,7 +36,7 @@
<i class="fa fa-trash-o fa-lg delete-draft" aria-hidden="true" data-toggle="tooltip" title="{{t 'Delete draft' }} (Backspace)"></i>
</div>
</div>
<div class="message_content rendered_markdown restore-draft" data-toggle="tooltip" title="{{t 'Restore draft' }}">{{{content}}}</div>
<div class="message_content rendered_markdown restore-draft" data-toggle="tooltip" title="{{t 'Restore draft' }}">{{rendered_markdown content}}</div>
</div>
</div>
</div>

4
static/templates/me_message.hbs
View File

@@ -9,9 +9,7 @@
<i class="zulip-icon bot" aria-label="{{t 'Bot' }}"></i>
{{/if}}
<span class="rendered_markdown status-message auto-select">
{{{ status_message }}}
</span>
<span class="rendered_markdown status-message auto-select">{{rendered_markdown status_message}}</span>
{{#if edited_status_msg}}
{{> edited_notice}}

2
static/templates/message_body.hbs
View File

@@ -36,7 +36,7 @@
</div>
{{#unless status_message}}
<div class="message_content rendered_markdown">{{#if use_match_properties}}{{{msg/match_content}}}{{else}}{{{msg/content}}}{{/if}}</div>
<div class="message_content rendered_markdown">{{#if use_match_properties}}{{rendered_markdown msg/match_content}}{{else}}{{rendered_markdown msg/content}}{{/if}}</div>
{{/unless}}
{{#if edited_in_left_col}}

2
static/templates/message_edit_form.hbs
View File

@@ -24,7 +24,7 @@
<path fill="#777" d="M128 768h256v64H128v-64z m320-384H128v64h320v-64z m128 192V448L384 640l192 192V704h320V576H576z m-288-64H128v64h160v-64zM128 704h160v-64H128v64z m576 64h64v128c-1 18-7 33-19 45s-27 18-45 19H64c-35 0-64-29-64-64V192c0-35 29-64 64-64h192C256 57 313 0 384 0s128 57 128 128h192c35 0 64 29 64 64v320h-64V320H64v576h640V768zM128 256h512c0-35-29-64-64-64h-64c-35 0-64-29-64-64s-29-64-64-64-64 29-64 64-29 64-64 64h-64c-35 0-64 29-64 64z" />
</svg>
</button>
<textarea class="message_edit_content rendered_markdown" maxlength="10000" id="message_edit_content_{{message_id}}">{{content}}</textarea>
<textarea class="message_edit_content" maxlength="10000" id="message_edit_content_{{message_id}}">{{content}}</textarea>
<div class="scrolling_list preview_message_area" id="preview_message_area_{{message_id}}" style="display:none;">
<div id="markdown_preview_spinner_{{message_id}}"></div>
<div id="preview_content_{{message_id}}" class="preview_content rendered_markdown"></div>

2
static/templates/message_edit_history.hbs
View File

@@ -10,7 +10,7 @@
<div class="message_content message_edit_history_content"><p>Topic: <span class="highlight_text_inserted">{{ new_topic }}</span> <span class="highlight_text_deleted">{{ prev_topic }}</span></p></div>
{{/if}}
{{#if body_to_render}}
<div class="message_content rendered_markdown message_edit_history_content">{{{ body_to_render }}}</div>
<div class="message_content rendered_markdown message_edit_history_content">{{rendered_markdown body_to_render}}</div>
{{/if}}
<div class="message_author"><div class="author_details">{{ posted_or_edited }} {{ edited_by }}</div></div>
</div>

2
static/templates/subscription.hbs
View File

@@ -16,7 +16,7 @@
</div>
</div>
<div class="bottom-bar">
<div class="description rendered_markdown" data-no-description="{{t 'No description.'}}">{{{rendered_description}}}</div>
<div class="description rendered_markdown" data-no-description="{{t 'No description.'}}">{{rendered_markdown rendered_description}}</div>
{{#if is_old_stream}}
<div class="stream-message-count" data-toggle="tooltip" title="{{t 'Estimated messages per week' }}">
<i class="fa fa-bar-chart"></i>

2
static/templates/subscription_settings.hbs
View File

@@ -33,7 +33,7 @@
</div>
</div>
<div class="stream-description">
<span class="stream-description-editable editable-section description rendered_markdown" data-no-description="{{t 'No description.' }}">{{{rendered_description}}}</span>
<span class="stream-description-editable editable-section description rendered_markdown" data-no-description="{{t 'No description.' }}">{{rendered_markdown rendered_description}}</span>
{{#if can_change_name_description}}
<span class="editable" data-make-editable=".stream-description-editable"></span>
<span class="checkmark" data-finish-editing=".stream-description-editable">✓</span>

2
static/templates/user_profile_modal.hbs
View File

@@ -55,7 +55,7 @@
<a href={{this.link}} target="_blank" class="value">{{this.value}}</a>
{{else}}
{{#if this.rendered_value}}
<div class="value rendered_markdown">{{{this.rendered_value}}}</div>
<div class="value rendered_markdown">{{rendered_markdown this.rendered_value}}</div>
{{else}}
<div class="value">{{this.value}}</div>
{{/if}}

2
tools/linter_lib/custom_check.py
View File

@@ -136,7 +136,7 @@ js_rules = RuleList(
'exclude': {'frontend_tests/'},
'description': '.includes() is incompatible with Internet Explorer. Use .indexOf() !== -1 instead.'},
{'pattern': '[.]html[(]',
'exclude_pattern': r'''[.]html[(]("|'|render_|html|message.content|sub.rendered_description|i18n.t|rendered_|$|[)]|error_text|widget_elem|[$]error|[$][(]"<p>"[)])''',
'exclude_pattern': r'''\.html\(("|'|render_|html|message\.content|util\.clean_user_content_links|i18n\.t|rendered_|$|\)|error_text|widget_elem|\$error|\$\("<p>"\))''',
'exclude': {'static/js/portico', 'static/js/lightbox.js', 'static/js/ui_report.js',
'static/js/confirm_dialog.js',
'frontend_tests/'},

2
tools/webpack.config.ts
View File

@@ -110,7 +110,7 @@ export default (env?: string): webpack.Configuration[] => {
knownHelpers: ['if', 'unless', 'each', 'with',
// The ones below are defined in static/js/templates.js
'plural', 'eq', 'and', 'or', 'not',
't', 'tr'],
't', 'tr', 'rendered_markdown'],
preventIndent: true,
},
},