From 105d6a6998f9fb23795843355da4a7ed1e106a29 Mon Sep 17 00:00:00 2001 From: Aria Buckles Date: Wed, 20 May 2020 19:23:23 -0700 Subject: [PATCH] XSS: Fix sanitizeUrl vbscript/data xss I believe this fixes https://www.npmjs.com/advisories/1219 if `options.disableParsingRawHTML` is set. NOTE: This does not handle script elements, etc., that may be rendered when `options.disableParsingRawHTML` is not enabled. We might be able to use something like [`dompurify`](https://github.com/cure53/DOMPurify) to solve that case? According to https://owasp.org/www-community/xss-filter-evasion-cheatsheet , the dangerous `javascript:` protocol can contain some whitespace characters and still be vulnerable, and sometimes when used in conjunction with images, some other special characters like ` or < before the javascript: protocol can also leave a url vulnerable. This change re-adds the sanitiation logic removed in 9c6c782c , and also adds the vbscript/data handling from github.com/Khan/simple-markdown/pull/63 Test plan: Add tests and run `npm test` --- index.compiler.spec.js | 216 ++++++++++++++++++++++++++++++++++------- index.js | 7 +- 2 files changed, 187 insertions(+), 36 deletions(-) diff --git a/index.compiler.spec.js b/index.compiler.spec.js index bbb43de4..11863df5 100644 --- a/index.compiler.spec.js +++ b/index.compiler.spec.js @@ -925,6 +925,48 @@ describe('links', () => { expect(console.warn).toHaveBeenCalled(); }); + it('should sanitize markdown links containing JS expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + render(compiler('![foo](javascript:doSomethingBad)')); + + expect(root.innerHTML).toMatchInlineSnapshot(` + +foo + +`); + + expect(console.warn).toHaveBeenCalled(); + }); + + it('should sanitize markdown links containing Data expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + render(compiler('[foo](data:doSomethingBad)')); + expect(root.innerHTML).toMatchInlineSnapshot(` + + + foo + + +`); + expect(console.warn).toHaveBeenCalled(); + }); + + it('should sanitize markdown links containing VBScript expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + render(compiler('[foo](vbScript:doSomethingBad)')); + expect(root.innerHTML).toMatchInlineSnapshot(` + + + foo + + +`); + expect(console.warn).toHaveBeenCalled(); + }); + it('should sanitize markdown links containing encoded JS expressions', () => { jest.spyOn(console, 'warn').mockImplementation(() => {}); @@ -957,6 +999,60 @@ describe('links', () => { expect(console.warn).toHaveBeenCalled(); }); + it('should sanitize markdown links containing padded encoded vscript expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + render(compiler('[foo]( VBScript%3AdoSomethingBad)')); + + expect(root.innerHTML).toMatchInlineSnapshot(` + + + foo + + +`); + expect(console.warn).toHaveBeenCalled(); + }); + + it('should sanitize markdown images containing padded encoded vscript expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + render(compiler('![foo]( VBScript%3AdoSomethingBad)')); + expect(root.innerHTML).toMatchInlineSnapshot(` + +foo + +`); + expect(console.warn).toHaveBeenCalled(); + }); + + it('should sanitize markdown links containing padded encoded data expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + render(compiler('[foo](` + foo + + +`); + expect(console.warn).toHaveBeenCalled(); + }); + + it('should sanitize markdown images containing padded encoded data expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + render(compiler('![foo](` + +`); + expect(console.warn).toHaveBeenCalled(); + }); + it('should sanitize markdown links containing invalid characters', () => { jest.spyOn(console, 'warn').mockImplementation(() => {}); @@ -973,20 +1069,65 @@ describe('links', () => { }); it('should sanitize html links containing JS expressions', () => { - jest.spyOn(console, 'warn').mockImplementation(() => {}); + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + render(compiler('foo')); + + expect(root.innerHTML).toMatchInlineSnapshot(` + + + foo + + +`); - render(compiler('foo')); + expect(console.warn).toHaveBeenCalled(); + }); - expect(root.innerHTML).toMatchInlineSnapshot(` + it('should sanitize html links containing encoded, prefixed data expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + render(compiler('foo')); + expect(root.innerHTML).toMatchInlineSnapshot(` foo `); + expect(console.warn).toHaveBeenCalled(); + }); + + it('should sanitize html images containing encoded, prefixed JS expressions', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + // TODO: something is off on parsing here, because this prints: + // console.error("Warning: Unknown prop `javascript:alert` on tag"...) + // Which it shouldn't + render(compiler(' + + \`('alertstr')" + + +`); + expect(console.warn).toHaveBeenCalled(); + }); + + it('should sanitize html images containing weird parsing src=s', () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + render(compiler('`')); + expect(root.innerHTML).toMatchInlineSnapshot(` + + + + \` + + +`); + expect(console.warn).toHaveBeenCalled(); + }); it('should handle a link with a URL in the text', () => { render( @@ -1582,14 +1723,14 @@ describe('GFM tables', () => { it('#241 should not ignore the first cell when its contents is empty', () => { render( - compiler( - [ - '| Foo | Bar | Baz |', - '| --- | --- | --- |', - '| | 2 | 3 |', - '| | 5 | 6 |', - ].join('\n') - ) + compiler( + [ + '| Foo | Bar | Baz |', + '| --- | --- | --- |', + '| | 2 | 3 |', + '| | 5 | 6 |', + ].join('\n') + ) ); expect(root.innerHTML).toMatchInlineSnapshot(` @@ -1780,7 +1921,6 @@ describe('GFM tables', () => { `); }); - }); describe('arbitrary HTML', () => { @@ -1976,10 +2116,12 @@ describe('arbitrary HTML', () => { }); it('throws out multiline HTML comments', () => { - render(compiler(`Foo\n`)); +comment -->`) + ); expect(root.innerHTML).toMatchInlineSnapshot(` @@ -2800,7 +2942,7 @@ fun main() { `); }); - it('should not fail with lots of \\n in the middle of the text', () => { + it('should not fail with lots of \\n in the middle of the text', () => { render( compiler( 'Text\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\ntext', @@ -2825,12 +2967,9 @@ fun main() { it('should not render html if disableParsingRawHTML is true', () => { render( - compiler( - 'Text with html inside', - { - disableParsingRawHTML: true - } - ) + compiler('Text with html inside', { + disableParsingRawHTML: true, + }) ); expect(root.innerHTML).toMatchInlineSnapshot(` @@ -2843,12 +2982,9 @@ fun main() { it('should render html if disableParsingRawHTML is false', () => { render( - compiler( - 'Text with html inside', - { - disableParsingRawHTML: false - } - ) + compiler('Text with html inside', { + disableParsingRawHTML: false, + }) ); expect(root.innerHTML).toMatchInlineSnapshot(` @@ -2976,7 +3112,15 @@ describe('footnotes', () => { }); it('should handle complex references', () => { - render(compiler(['foo[^referencé heré 123] bar', '', '[^referencé heré 123]: Baz baz'].join('\n'))); + render( + compiler( + [ + 'foo[^referencé heré 123] bar', + '', + '[^referencé heré 123]: Baz baz', + ].join('\n') + ) + ); expect(root.innerHTML).toMatchInlineSnapshot(` @@ -3002,7 +3146,13 @@ describe('footnotes', () => { }); it('should handle conversion of multiple references into links', () => { - render(compiler(['foo[^abc] bar. baz[^def]', '', '[^abc]: Baz baz', '[^def]: Def'].join('\n'))); + render( + compiler( + ['foo[^abc] bar. baz[^def]', '', '[^abc]: Baz baz', '[^def]: Def'].join( + '\n' + ) + ) + ); expect(root.innerHTML).toMatchInlineSnapshot(` @@ -3410,7 +3560,7 @@ describe('overrides', () => { render( compiler('Hello.\n\n', { overrides: { p: { component: FakeParagraph } }, - options: { disableParsingRawHTML: true } + options: { disableParsingRawHTML: true }, }) ); @@ -3424,7 +3574,7 @@ describe('overrides', () => { render( compiler('Hello.\n\nI am a fake span', { overrides: { FakeSpan }, - options: { disableParsingRawHTML: true } + options: { disableParsingRawHTML: true }, }) ); diff --git a/index.js b/index.js index 788ea79a..d012c3bc 100644 --- a/index.js +++ b/index.js @@ -586,12 +586,13 @@ function reactFor(outputFunc) { function sanitizeUrl(url) { try { - const decoded = decodeURIComponent(url); + const decoded = decodeURIComponent(url) + .replace(/[^A-Za-z0-9/:]/g, ''); - if (decoded.match(/^\s*javascript:/i)) { + if (decoded.match(/^\s*(javascript|vbscript|data):/i)) { if (process.env.NODE_ENV !== 'production') { console.warn( - 'Anchor URL contains an unsafe JavaScript expression, it will not be rendered.', + 'Anchor URL contains an unsafe JavaScript/VBScript/data expression, it will not be rendered.', decoded ); }