ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Don’t load sanitize-html in the browser 

This approach solves the same xss vulnerability, but by doing it a way
we can guarantee that what we are trying to .html() onto the page is
safe (and not just arbitrary user content) instead of having to load the
_huge_ sanitize-html npm package and all of it’s deps
(like postcss, source-map, htmlparser2, buffer, and a bunch of others)
On the frontend so we can use it to sanitize all the untrusted
data-mathml content.

closes: CORE-2748
refs COMMS-1846

Test Plan:
* Go to the calendar, click on a day to add an event, and click on the
  "More Options" button.

* Switch to the html editor, and add the following:

  <img class='equation_image'
  data-mathml='<img src=x onerror=prompt(document.cookie); />'>

* Save the event. Go back to the calendar and click on the newly saved
  event, and notice that you don't get XSS'd

* Create another event in the calendar, and this time add the
  following via the html editor:

  <img class="equation_image" title="(-\infty,\infty)"
  src="/equation_images/(-%255Cinfty%252C%255Cinfty)"
  alt="Infinities: (-\infty,\infty)"
  data-equation-content="(-\infty,\infty)" />

* Save the event, and load it in the calendar. Notice that it still
  properly displays this mathml image properly.

* Test the same calendar event with a screen reader, and notice that
  it is still accessible

Change-Id: I5fc1e17ecfc8576dfeaffacff8efcb08873bdd5e
Reviewed-on: https://gerrit.instructure.com/188609
Tested-by: Jenkins
Reviewed-by: Landon Gilbert-Bland <lbland@instructure.com>
QA-Review: Landon Gilbert-Bland <lbland@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This commit is contained in:
Ryan Shaw 2019-04-05 14:14:33 -06:00
parent eac5a15d71
commit e28bb762a9
8 changed files with 25 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
app/coffeescripts/str/apiUserContent.js
View File

@ -19,46 +19,17 @@ import $ from 'jquery'
import _ from 'underscore'
import htmlEscape from 'str/htmlEscape'
import I18n from 'i18n!user_content'
import mathMLTagNames from 'mathml-tag-names'
import sanitizeHtml from 'sanitize-html'
// From https://developer.mozilla.org/en-US/docs/Web/MathML/Attribute
const MATHML_ATTRIBUTES = [
'accent', 'accentunder', 'actiontype', 'align', 'alignmentscope', 'altimg', 'altimg-width',
'altimg-height', 'altimg-valign', 'alttext', 'bevelled', 'charalign', 'close', 'columnalign',
'columnlines', 'columnspacing', 'columnspan', 'columnwidth', 'crossout', 'decimalpoint',
'denomalign', 'depth', 'dir', 'display', 'displaystyle', 'edge', 'equalcolumns', 'equalrows',
'fence', 'form', 'frame', 'framespacing', 'groupalign', 'height', 'id', 'indentalign',
'indentalignfirst', 'indentalignlast', 'indentshift', 'indentshiftfirst', 'indentshiftlast',
'indenttarget', 'infixlinebreakstyle', 'largeop', 'length', 'linebreak', 'linebreakmultchar',
'linebreakstyle', 'lineleading', 'linethickness', 'location', 'longdivstyle', 'lspace',
'lquote', 'mathbackground', 'mathcolor', 'mathsize', 'mathvariant', 'maxsize', 'minlabelspacing',
'minsize', 'movablelimits', 'notation', 'numalign', 'open', 'overflow', 'position', 'rowalign',
'rowlines', 'rowspacing', 'rowspan', 'rspace', 'rquote', 'scriptlevel', 'scriptminsize',
'Starting', 'scriptsizemultiplier', 'selection', 'separator', 'separators', 'shift', 'side',
'src', 'stackalign', 'stretchy', 'subscriptshift', 'supscriptshift', 'symmetric', 'voffset',
'width', 'xmlns'
]
function sanitizeMathmlHtml(html) {
const opts = {
allowedTags: mathMLTagNames,
allowedAttributes: {
'*': MATHML_ATTRIBUTES
}
}
return sanitizeHtml(html, opts)
}
const apiUserContent = {
/*
xsslint safeString.identifier mathml
xsslint safeString.identifier sanitized
*/
translateMathmlForScreenreaders ($equationImage) {
const sanitized = sanitizeMathmlHtml($equationImage.data('mathml'))
const mathml = $('<div/>').html(sanitized).html()
// note, it is safe to treat the x-canvaslms-safe-mathml as html because it
// only ever gets put there by us (in Api::Html::Content::apply_mathml).
// Any user content that gets sent to the server will have the
// x-canvaslms-safe-mathml attribute stripped out.
const mathml = $('<div/>').html($equationImage.attr('x-canvaslms-safe-mathml')).html()
const mathmlSpan = $('<span class="hidden-readable"></span>')
mathmlSpan.html(mathml)
return mathmlSpan
@ -148,7 +119,7 @@ const apiUserContent = {
$dummy.find('img.equation_image').each((index, equationImage) => {
const $equationImage = $(equationImage)
const mathmlSpan = apiUserContent.translateMathmlForScreenreaders($equationImage)
$equationImage.removeAttr('data-mathml')
$equationImage.removeAttr('x-canvaslms-safe-mathml')
$equationImage.after(mathmlSpan)
})
}

7
lib/api/html/content.rb
View File

@ -168,7 +168,12 @@ module Api
mathml = UserContent.latex_to_mathml(equation)
return if mathml.blank?
node['data-mathml'] = mathml
# NOTE: we use "x-canvaslms-safe-mathml" instead of just "data-mathml"
# because canvas_sanitize will strip it out on the way in but it won't
# strip out data-mathml. This means we can gaurentee that there is never
# user input in x-canvaslms-safe-mathml and we can safely pass it to
# $el.html() in translateMathmlForScreenreaders in the js in the frontend
node['x-canvaslms-safe-mathml'] = mathml
end
end
end

2
package.json
View File

@ -81,7 +81,6 @@
"jquery.elastic": "1.0.0",
"location-origin": "^1.1.4",
"lodash": "^4.16.4",
"mathml-tag-names": "^2.1.0",
"md5": "^2.2.1",
"mediaelement": "https://github.com/instructure/mediaelement.git#master",
"moment": "^2.10.6",
@ -106,7 +105,6 @@
"redux": "^4.0.1",
"redux-actions": "^2.6.4",
"redux-thunk": "^2.3.0",
"sanitize-html": "^1.20.0",
"spin.js": "2.3.2",
"swfobject": "^2.2.1",
"tablesorter": "^2.28.5",

8
spec/apis/html/content_spec.rb
View File

@ -31,9 +31,9 @@ describe Api::Html::Content, type: :request do
expect(@node['alt']).to eql(@latex)
end
it "sets data-mathml" do
it "sets x-canvaslms-safe-mathml" do
Api::Html::Content.apply_mathml(@node)
expect(@node['data-mathml']).to eql(Ritex::Parser.new.parse(@latex))
expect(@node['x-canvaslms-safe-mathml']).to eql(Ritex::Parser.new.parse(@latex))
end
end
@ -52,9 +52,9 @@ describe Api::Html::Content, type: :request do
expect(@node['alt']).to eql(@latex)
end
it "doesn't set data-mathml" do
it "doesn't set x-canvaslms-safe-mathml" do
Api::Html::Content.apply_mathml(@node)
expect(@node['data-mathml']).to be_nil
expect(@node['x-canvaslms-safe-mathml']).to be_nil
end
end
end

10
spec/coffeescripts/str/apiUserContentSpec.js
View File

@ -23,8 +23,8 @@ let mathml_html
QUnit.module('apiUserContent.convert', {
setup() {
mathml_html ="<div><ul>\n" +
"<li><img class=\"equation_image\" data-mathml=\"&lt;math xmlns=&quot;http://www.w3.org/1998/Math/MathML&quot; display=&quot;inline&quot;&gt;&lt;mi&gt;i&lt;/mi&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;mi&gt;t&lt;/mi&gt;&lt;mi&gt;f&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;mo&gt;/&lt;/mo&gt;&lt;mi&gt;g&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;/math&gt;\"></li>\n" +
"<li><img class=\"equation_image\" data-mathml='&lt;math xmlns=\"http://www.w3.org/1998/Math/MathML\" display=\"inline\"&gt;&lt;mo lspace=\"thinmathspace\" rspace=\"thinmathspace\"&gt;&amp;Sum;&lt;/mo&gt;&lt;mn&gt;1&lt;/mn&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;/math&gt;'></li>\n" +
"<li><img class=\"equation_image\" x-canvaslms-safe-mathml=\"&lt;math xmlns=&quot;http://www.w3.org/1998/Math/MathML&quot; display=&quot;inline&quot;&gt;&lt;mi&gt;i&lt;/mi&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;mi&gt;t&lt;/mi&gt;&lt;mi&gt;f&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;mo&gt;/&lt;/mo&gt;&lt;mi&gt;g&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;/math&gt;\"></li>\n" +
"<li><img class=\"equation_image\" x-canvaslms-safe-mathml='&lt;math xmlns=\"http://www.w3.org/1998/Math/MathML\" display=\"inline\"&gt;&lt;mo lspace=\"thinmathspace\" rspace=\"thinmathspace\"&gt;&amp;Sum;&lt;/mo&gt;&lt;mn&gt;1&lt;/mn&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;/math&gt;'></li>\n" +
"<li><img class=\"nothing_special\"></li>\n"+
"</ul></div>"
}
@ -35,12 +35,6 @@ test('moves mathml into a screenreader element', () => {
ok(output.includes('<span class="hidden-readable"><math '))
})
test('prevents XSS on data-mathml content', () => {
const xss_mathml = "<img class='equation_image' data-mathml='<img src=x onerror=prompt(document.cookie); />'>"
const output = apiUserContent.convert(xss_mathml)
ok(!output.includes('onerror'))
})
test('mathml need not be screenreadered if editing content (this would start an update loop)', () => {
const output = apiUserContent.convert(mathml_html, {forEditing: true})
ok(!output.includes('<span class="hidden-readable"><math '))

4
spec/lib/api/html/content_spec.rb
View File

@ -82,8 +82,8 @@ module Api
url_helper = double(rewrite_api_urls: nil)
html = Content.new(string).rewritten_html(url_helper)
expected = "<div><ul>\n"\
"<li><img class=\"equation_image\" data-equation-content=\"\int f(x)/g(x)\" data-mathml=\"&lt;math xmlns=&quot;http://www.w3.org/1998/Math/MathML&quot; display=&quot;inline&quot;&gt;&lt;mi&gt;i&lt;/mi&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;mi&gt;t&lt;/mi&gt;&lt;mi&gt;f&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;mo&gt;/&lt;/mo&gt;&lt;mi&gt;g&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;/math&gt;\"></li>\n"\
"<li><img class=\"equation_image\" data-equation-content=\"\\sum 1..n\" data-mathml='&lt;math xmlns=\"http://www.w3.org/1998/Math/MathML\" display=\"inline\"&gt;&lt;mo lspace=\"thinmathspace\" rspace=\"thinmathspace\"&gt;&amp;Sum;&lt;/mo&gt;&lt;mn&gt;1&lt;/mn&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;/math&gt;'></li>\n"\
"<li><img class=\"equation_image\" data-equation-content=\"\int f(x)/g(x)\" x-canvaslms-safe-mathml=\"&lt;math xmlns=&quot;http://www.w3.org/1998/Math/MathML&quot; display=&quot;inline&quot;&gt;&lt;mi&gt;i&lt;/mi&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;mi&gt;t&lt;/mi&gt;&lt;mi&gt;f&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;mo&gt;/&lt;/mo&gt;&lt;mi&gt;g&lt;/mi&gt;&lt;mo stretchy='false'&gt;(&lt;/mo&gt;&lt;mi&gt;x&lt;/mi&gt;&lt;mo stretchy='false'&gt;)&lt;/mo&gt;&lt;/math&gt;\"></li>\n"\
"<li><img class=\"equation_image\" data-equation-content=\"\\sum 1..n\" x-canvaslms-safe-mathml='&lt;math xmlns=\"http://www.w3.org/1998/Math/MathML\" display=\"inline\"&gt;&lt;mo lspace=\"thinmathspace\" rspace=\"thinmathspace\"&gt;&amp;Sum;&lt;/mo&gt;&lt;mn&gt;1&lt;/mn&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mo&gt;.&lt;/mo&gt;&lt;mi&gt;n&lt;/mi&gt;&lt;/math&gt;'></li>\n"\
"<li><img class=\"nothing_special\"></li>\n</ul></div>"
expect(html).to eq(expected)
end

2
spec/models/content_migration/course_copy_quizzes_spec.rb
View File

@ -541,7 +541,7 @@ equation: <img class="equation_image" title="Log_216" src="/equation_images/Log_
qtext = <<-HTML.strip
equation: <p>
<img class="equation_image" title="\\sum" src="/equation_images/%255Csum"
alt="LaTeX: \\sum" data-equation-content="\\sum" data-mathml="&lt;math xmlns=&quot;http://www.w3.org/1998/Math/MathML&quot;&gt;
alt="LaTeX: \\sum" data-equation-content="\\sum" x-canvaslms-safe-mathml="&lt;math xmlns=&quot;http://www.w3.org/1998/Math/MathML&quot;&gt;
&lt;mo&gt;&amp;#x2211;&lt;!-- &sum; --&gt;&lt;/mo&gt;&lt;/math&gt;" />
</p>
HTML

40
yarn.lock
View File

@ -11102,7 +11102,7 @@ lodash.camelcase@^4.3.0:
resolved "https://registry.yarnpkg.com/lodash.camelcase/-/lodash.camelcase-4.3.0.tgz#b28aa6288a2b9fc651035c7711f65ab6190331a6"
integrity sha1-soqmKIorn8ZRA1x3EfZathkDMaY=
lodash.clonedeep@^4.3.2, lodash.clonedeep@^4.5.0:
lodash.clonedeep@^4.3.2:
version "4.5.0"
resolved "https://registry.yarnpkg.com/lodash.clonedeep/-/lodash.clonedeep-4.5.0.tgz#e23f3f9c4f8fbdde872529c1071857a086e5ccef"
integrity sha1-4j8/nE+Pvd6HJSnBBxhXoIblzO8=
@ -11129,11 +11129,6 @@ lodash.escape@^4.0.1:
resolved "https://registry.yarnpkg.com/lodash.escape/-/lodash.escape-4.0.1.tgz#c9044690c21e04294beaa517712fded1fa88de98"
integrity sha1-yQRGkMIeBClL6qUXcS/e0fqI3pg=
lodash.escaperegexp@^4.1.2:
version "4.1.2"
resolved "https://registry.yarnpkg.com/lodash.escaperegexp/-/lodash.escaperegexp-4.1.2.tgz#64762c48618082518ac3df4ccf5d5886dae20347"
integrity sha1-ZHYsSGGAglGKw99Mz11YhtriA0c=
lodash.flattendeep@^4.4.0:
version "4.4.0"
resolved "https://registry.yarnpkg.com/lodash.flattendeep/-/lodash.flattendeep-4.4.0.tgz#fb030917f86a3134e5bc9bec0d69e0013ddfedb2"
@ -11169,11 +11164,6 @@ lodash.isplainobject@^4.0.6:
resolved "https://registry.yarnpkg.com/lodash.isplainobject/-/lodash.isplainobject-4.0.6.tgz#7c526a52d89b45c45cc690b88163be0497f550cb"
integrity sha1-fFJqUtibRcRcxpC4gWO+BJf1UMs=
lodash.isstring@^4.0.1:
version "4.0.1"
resolved "https://registry.yarnpkg.com/lodash.isstring/-/lodash.isstring-4.0.1.tgz#d527dfb5456eca7cc9bb95d5daeaf88ba54a5451"
integrity sha1-1SfftUVuynzJu5XV2ur4i6VKVFE=
lodash.kebabcase@^4.1.1:
version "4.1.1"
resolved "https://registry.yarnpkg.com/lodash.kebabcase/-/lodash.kebabcase-4.1.1.tgz#8489b1cb0d29ff88195cceca448ff6d6cc295c36"
@ -11198,7 +11188,7 @@ lodash.merge@^4.6.0:
resolved "https://registry.yarnpkg.com/lodash.merge/-/lodash.merge-4.6.1.tgz#adc25d9cb99b9391c59624f379fbba60d7111d54"
integrity sha512-AOYza4+Hf5z1/0Hztxpm2/xiPZgi/cjMqdnKTUWTBSKchJlxXXuUSxCCl8rJlf4g6yww/j6mA8nC8Hw/EZWxKQ==
lodash.mergewith@^4.6.0, lodash.mergewith@^4.6.1:
lodash.mergewith@^4.6.0:
version "4.6.1"
resolved "https://registry.yarnpkg.com/lodash.mergewith/-/lodash.mergewith-4.6.1.tgz#639057e726c3afbdb3e7d42741caa8d6e4335927"
integrity sha512-eWw5r+PYICtEBgrBE5hhlT6aAa75f411bgDz/ZL2KZqYV03USvucsxcHUIlGTDTECs1eunpI7HOV7U+WLDvNdQ==
@ -11565,7 +11555,7 @@ math-random@^1.0.1:
resolved "https://registry.yarnpkg.com/math-random/-/math-random-1.0.1.tgz#8b3aac588b8a66e4975e3cdea67f7bb329601fac"
integrity sha1-izqsWIuKZuSXXjzepn97sylgH6w=
mathml-tag-names@^2.0.1, mathml-tag-names@^2.1.0:
mathml-tag-names@^2.0.1:
version "2.1.0"
resolved "https://registry.yarnpkg.com/mathml-tag-names/-/mathml-tag-names-2.1.0.tgz#490b70e062ee24636536e3d9481e333733d00f2c"
integrity sha512-3Zs9P/0zzwTob2pdgT0CHZuMbnSUSp8MB1bddfm+HDmnFWHGT4jvEZRf+2RuPoa+cjdn/z25SEt5gFTqdhvJAg==
@ -15389,22 +15379,6 @@ sane@^4.0.3:
minimist "^1.1.1"
walker "~1.0.5"
sanitize-html@^1.20.0:
version "1.20.0"
resolved "https://registry.yarnpkg.com/sanitize-html/-/sanitize-html-1.20.0.tgz#9a602beb1c9faf960fb31f9890f61911cc4d9156"
integrity sha512-BpxXkBoAG+uKCHjoXFmox6kCSYpnulABoGcZ/R3QyY9ndXbIM5S94eOr1IqnzTG8TnbmXaxWoDDzKC5eJv7fEQ==
dependencies:
chalk "^2.4.1"
htmlparser2 "^3.10.0"
lodash.clonedeep "^4.5.0"
lodash.escaperegexp "^4.1.2"
lodash.isplainobject "^4.0.6"
lodash.isstring "^4.0.1"
lodash.mergewith "^4.6.1"
postcss "^7.0.5"
srcset "^1.0.0"
xtend "^4.0.1"
sass-direction@^1:
version "1.2.0"
resolved "https://registry.yarnpkg.com/sass-direction/-/sass-direction-1.2.0.tgz#84be38ed3876e4847c9924376d434203763bc26e"
@ -16117,14 +16091,6 @@ sprintf-js@~1.0.2, sprintf-js@~1.0.3:
resolved "https://registry.yarnpkg.com/sprintf-js/-/sprintf-js-1.0.3.tgz#04e6926f662895354f3dd015203633b857297e2c"
integrity sha1-BOaSb2YolTVPPdAVIDYzuFcpfiw=
srcset@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/srcset/-/srcset-1.0.0.tgz#a5669de12b42f3b1d5e83ed03c71046fc48f41ef"
integrity sha1-pWad4StC87HV6D7QPHEEb8SPQe8=
dependencies:
array-uniq "^1.0.2"
number-is-nan "^1.0.0"
sshpk@^1.7.0:
version "1.16.0"
resolved "https://registry.yarnpkg.com/sshpk/-/sshpk-1.16.0.tgz#1d4963a2fbffe58050aa9084ca20be81741c07de"