diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-26 10:42:58 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-26 10:42:58 +0300 |
commit | ffb237a45b3232d0deebcbfbbc5daf3515038dfd (patch) | |
tree | 9a182c778662fcfcd73b900f658f4a1e5c981055 | |
parent | 00e0757c9057f4c6240cd5f334527bd6088ceb92 (diff) | |
parent | 9f3f0059042350b076032894ad77f60fcd801a5a (diff) |
Merge branch 'security-katex-dos-12-1' into '12-1-stable'
Enforce max chars and max render time in markdown math
See merge request gitlab/gitlabhq!3287
-rw-r--r-- | app/assets/javascripts/behaviors/markdown/render_math.js | 146 | ||||
-rw-r--r-- | changelogs/unreleased/security-katex-dos-12-1.yml | 5 | ||||
-rw-r--r-- | locale/gitlab.pot | 9 | ||||
-rw-r--r-- | spec/features/markdown/math_spec.rb | 6 |
4 files changed, 143 insertions, 23 deletions
diff --git a/app/assets/javascripts/behaviors/markdown/render_math.js b/app/assets/javascripts/behaviors/markdown/render_math.js index a68936d79e2..53867b3096b 100644 --- a/app/assets/javascripts/behaviors/markdown/render_math.js +++ b/app/assets/javascripts/behaviors/markdown/render_math.js @@ -1,6 +1,5 @@ -import $ from 'jquery'; -import { __ } from '~/locale'; import flash from '~/flash'; +import { s__, sprintf } from '~/locale'; // Renders math using KaTeX in any element with the // `js-render-math` class @@ -10,21 +9,131 @@ import flash from '~/flash'; // <code class="js-render-math"></div> // -// Loop over all math elements and render math -function renderWithKaTeX(elements, katex) { - elements.each(function katexElementsLoop() { - const mathNode = $('<span></span>'); - const $this = $(this); - - const display = $this.attr('data-math-style') === 'display'; - try { - katex.render($this.text(), mathNode.get(0), { displayMode: display, throwOnError: false }); - mathNode.insertAfter($this); - $this.remove(); - } catch (err) { - throw err; +const MAX_MATH_CHARS = 1000; +const MAX_RENDER_TIME_MS = 2000; + +// These messages might be used with inline errors in the future. Keep them around. For now, we will +// display a single error message using flash(). + +// const CHAR_LIMIT_EXCEEDED_MSG = sprintf( +// s__( +// 'math|The following math is too long. For performance reasons, math blocks are limited to %{maxChars} characters. Try splitting up this block, or include an image instead.', +// ), +// { maxChars: MAX_MATH_CHARS }, +// ); +// const RENDER_TIME_EXCEEDED_MSG = s__( +// "math|The math in this entry is taking too long to render. Any math below this point won't be shown. Consider splitting it among multiple entries.", +// ); + +const RENDER_FLASH_MSG = sprintf( + s__( + 'math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead.', + ), + { maxChars: MAX_MATH_CHARS }, +); + +// Wait for the browser to reflow the layout. Reflowing SVG takes time. +// This has to wrap the inner function, otherwise IE/Edge throw "invalid calling object". +const waitForReflow = fn => { + window.requestAnimationFrame(fn); +}; + +/** + * Renders math blocks sequentially while protecting against DoS attacks. Math blocks have a maximum character limit of MAX_MATH_CHARS. If rendering math takes longer than MAX_RENDER_TIME_MS, all subsequent math blocks are skipped and an error message is shown. + */ +class SafeMathRenderer { + /* + How this works: + + The performance bottleneck in rendering math is in the browser trying to reflow the generated SVG. + During this time, the JS is blocked and the page becomes unresponsive. + We want to render math blocks one by one until a certain time is exceeded, after which we stop + rendering subsequent math blocks, to protect against DoS. However, browsers do reflowing in an + asynchronous task, so we can't time it synchronously. + + SafeMathRenderer essentially does the following: + 1. Replaces all math blocks with placeholders so that they're not mistakenly rendered twice. + 2. Places each placeholder element in a queue. + 3. Renders the element at the head of the queue and waits for reflow. + 4. After reflow, gets the elapsed time since step 3 and repeats step 3 until the queue is empty. + */ + queue = []; + totalMS = 0; + + constructor(elements, katex) { + this.elements = elements; + this.katex = katex; + + this.renderElement = this.renderElement.bind(this); + this.render = this.render.bind(this); + } + + renderElement() { + if (!this.queue.length) { + return; } - }); + + const el = this.queue.shift(); + const text = el.textContent; + + el.removeAttribute('style'); + + if (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS) { + if (!this.flashShown) { + flash(RENDER_FLASH_MSG); + this.flashShown = true; + } + + // Show unrendered math code + const codeElement = document.createElement('pre'); + codeElement.className = 'code'; + codeElement.textContent = el.textContent; + el.parentNode.replaceChild(codeElement, el); + + // Render the next math + this.renderElement(); + } else { + this.startTime = Date.now(); + + try { + el.innerHTML = this.katex.renderToString(text, { + displayMode: el.getAttribute('data-math-style') === 'display', + throwOnError: true, + maxSize: 20, + maxExpand: 20, + }); + } catch { + // Don't show a flash for now because it would override an existing flash message + el.textContent = s__('math|There was an error rendering this math block'); + // el.style.color = '#d00'; + el.className = 'katex-error'; + } + + // Give the browser time to reflow the svg + waitForReflow(() => { + const deltaTime = Date.now() - this.startTime; + this.totalMS += deltaTime; + + this.renderElement(); + }); + } + } + + render() { + // Replace math blocks with a placeholder so they aren't rendered twice + this.elements.forEach(el => { + const placeholder = document.createElement('span'); + placeholder.style.display = 'none'; + placeholder.setAttribute('data-math-style', el.getAttribute('data-math-style')); + placeholder.textContent = el.textContent; + el.parentNode.replaceChild(placeholder, el); + this.queue.push(placeholder); + }); + + // If we wait for the browser thread to settle down a bit, math rendering becomes 5-10x faster + // and less prone to timeouts. + setTimeout(this.renderElement, 400); + } } export default function renderMath($els) { @@ -34,7 +143,8 @@ export default function renderMath($els) { import(/* webpackChunkName: 'katex' */ 'katex/dist/katex.min.css'), ]) .then(([katex]) => { - renderWithKaTeX($els, katex); + const renderer = new SafeMathRenderer($els.get(), katex); + renderer.render(); }) - .catch(() => flash(__('An error occurred while rendering KaTeX'))); + .catch(() => {}); } diff --git a/changelogs/unreleased/security-katex-dos-12-1.yml b/changelogs/unreleased/security-katex-dos-12-1.yml new file mode 100644 index 00000000000..df803a5eafd --- /dev/null +++ b/changelogs/unreleased/security-katex-dos-12-1.yml @@ -0,0 +1,5 @@ +--- +title: Enforce max chars and max render time in markdown math +merge_request: +author: +type: security diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a5e55585480..d4d2045c49f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1055,9 +1055,6 @@ msgstr "" msgid "An error occurred while parsing recent searches" msgstr "" -msgid "An error occurred while rendering KaTeX" -msgstr "" - msgid "An error occurred while rendering preview broadcast message" msgstr "" @@ -12890,6 +12887,12 @@ msgstr "" msgid "manual" msgstr "" +msgid "math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead." +msgstr "" + +msgid "math|There was an error rendering this math block" +msgstr "" + msgid "merge request" msgid_plural "merge requests" msgstr[0] "" diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb index 16ad0d456be..776da128a47 100644 --- a/spec/features/markdown/math_spec.rb +++ b/spec/features/markdown/math_spec.rb @@ -32,7 +32,9 @@ describe 'Math rendering', :js do visit project_issue_path(project, issue) - expect(page).to have_selector('.katex-error', text: "\href{javascript:alert('xss');}{xss}") - expect(page).to have_selector('.katex-html a', text: 'Gitlab') + page.within '.description > .md' do + expect(page).to have_selector('.katex-error') + expect(page).to have_selector('.katex-html a', text: 'Gitlab') + end end end |