From 9701fd32b7b7cf56a85f53ca3af450e6e81af339 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 29 Apr 2018 03:07:31 +0100 Subject: [PATCH 1/3] switch back to blob urls for rendering e2e attachments Based on @walle303's work at https://github.com/matrix-org/matrix-react-sdk/pull/1820 Deliberately reverts https://github.com/matrix-org/matrix-react-sdk/commit/8f778f54fd10428836c99d08346cf89f038dd0ce Mitigates XSS by whitelisting the mime-types of the attachments so that malicious ones should not be recognised and executed by the browser. --- src/components/views/messages/MAudioBody.js | 4 +- src/components/views/messages/MImageBody.js | 6 +- src/components/views/messages/MVideoBody.js | 6 +- src/utils/DecryptFile.js | 82 ++++++++++++++++----- 4 files changed, 73 insertions(+), 25 deletions(-) diff --git a/src/components/views/messages/MAudioBody.js b/src/components/views/messages/MAudioBody.js index ab53918987..1e9962846c 100644 --- a/src/components/views/messages/MAudioBody.js +++ b/src/components/views/messages/MAudioBody.js @@ -20,7 +20,7 @@ import React from 'react'; import MFileBody from './MFileBody'; import MatrixClientPeg from '../../../MatrixClientPeg'; -import { decryptFile, readBlobAsDataUri } from '../../../utils/DecryptFile'; +import { decryptFile } from '../../../utils/DecryptFile'; import { _t } from '../../../languageHandler'; export default class MAudioBody extends React.Component { @@ -54,7 +54,7 @@ export default class MAudioBody extends React.Component { let decryptedBlob; decryptFile(content.file).then(function(blob) { decryptedBlob = blob; - return readBlobAsDataUri(decryptedBlob); + return URL.createObjectURL(decryptedBlob); }).done((url) => { this.setState({ decryptedUrl: url, diff --git a/src/components/views/messages/MImageBody.js b/src/components/views/messages/MImageBody.js index d5ce533bda..bb36e9b1ef 100644 --- a/src/components/views/messages/MImageBody.js +++ b/src/components/views/messages/MImageBody.js @@ -25,7 +25,7 @@ import ImageUtils from '../../../ImageUtils'; import Modal from '../../../Modal'; import sdk from '../../../index'; import dis from '../../../dispatcher'; -import { decryptFile, readBlobAsDataUri } from '../../../utils/DecryptFile'; +import { decryptFile } from '../../../utils/DecryptFile'; import Promise from 'bluebird'; import { _t } from '../../../languageHandler'; import SettingsStore from "../../../settings/SettingsStore"; @@ -169,14 +169,14 @@ export default class extends React.Component { thumbnailPromise = decryptFile( content.info.thumbnail_file, ).then(function(blob) { - return readBlobAsDataUri(blob); + return URL.createObjectURL(blob); }); } let decryptedBlob; thumbnailPromise.then((thumbnailUrl) => { return decryptFile(content.file).then(function(blob) { decryptedBlob = blob; - return readBlobAsDataUri(blob); + return URL.createObjectURL(blob); }).then((contentUrl) => { this.setState({ decryptedUrl: contentUrl, diff --git a/src/components/views/messages/MVideoBody.js b/src/components/views/messages/MVideoBody.js index 21edbae9a2..345c3f02e2 100644 --- a/src/components/views/messages/MVideoBody.js +++ b/src/components/views/messages/MVideoBody.js @@ -20,7 +20,7 @@ import React from 'react'; import PropTypes from 'prop-types'; import MFileBody from './MFileBody'; import MatrixClientPeg from '../../../MatrixClientPeg'; -import { decryptFile, readBlobAsDataUri } from '../../../utils/DecryptFile'; +import { decryptFile } from '../../../utils/DecryptFile'; import Promise from 'bluebird'; import { _t } from '../../../languageHandler'; import SettingsStore from "../../../settings/SettingsStore"; @@ -94,14 +94,14 @@ module.exports = React.createClass({ thumbnailPromise = decryptFile( content.info.thumbnail_file, ).then(function(blob) { - return readBlobAsDataUri(blob); + return URL.createObjectURL(blob); }); } let decryptedBlob; thumbnailPromise.then((thumbnailUrl) => { return decryptFile(content.file).then(function(blob) { decryptedBlob = blob; - return readBlobAsDataUri(blob); + return URL.createObjectURL(blob); }).then((contentUrl) => { this.setState({ decryptedUrl: contentUrl, diff --git a/src/utils/DecryptFile.js b/src/utils/DecryptFile.js index cb5e407407..f8327832d1 100644 --- a/src/utils/DecryptFile.js +++ b/src/utils/DecryptFile.js @@ -1,5 +1,6 @@ /* Copyright 2016 OpenMarket Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -22,25 +23,62 @@ import 'isomorphic-fetch'; import MatrixClientPeg from '../MatrixClientPeg'; import Promise from 'bluebird'; +// WARNING: We have to be very careful about what mime-types we allow into blobs, +// as for performance reasons these are now rendered via URL.createObjectURL() +// rather than by converting into data: URIs. +// +// This means that the content is rendered using the origin of the script which +// called createObjectURL(), and so if the content contains any scripting then it +// will pose a XSS vulnerability when the browser renders it. This is particularly +// bad if the user right-clicks the URI and pastes it into a new window or tab, +// as the blob will then execute with access to Riot's full JS environment(!) +// +// See https://github.com/matrix-org/matrix-react-sdk/pull/1820#issuecomment-385210647 +// for details. +// +// We mitigate this by only allowing mime-types into blobs which we know don't +// contain any scripting, and instantiate all others as application/octet-stream +// regardless of what mime-type the event claimed. Even if the payload itself +// is some malicious HTML, the fact we instantiate it with a media mimetype or +// application/octet-stream means the browser doesn't try to render it as such. +// +// One interesting edge case is image/svg+xml, which empirically *is* rendered +// correctly if the blob is set to the src attribute of an img tag (for thumbnails) +// *even if the mimetype is application/octet-stream*. However, empirically JS +// in the SVG isn't executed in this scenario, so we seem to be okay. +// +// Tested on Chrome 65 and Firefox 60 +// +// The list below is taken mainly from +// https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats +// N.B. Matrix doesn't currently specify which mimetypes are valid in given +// events, so we pick the ones which HTML5 browsers should be able to display +// +// For the record, mime-types which must NEVER enter this list below include: +// text/html, text/xhtml, image/svg, image/svg+xml, image/pdf, and similar. -/** - * Read blob as a data:// URI. - * @return {Promise} A promise that resolves with the data:// URI. - */ -export function readBlobAsDataUri(file) { - const deferred = Promise.defer(); - const reader = new FileReader(); - reader.onload = function(e) { - deferred.resolve(e.target.result); - }; - reader.onerror = function(e) { - deferred.reject(e); - }; - reader.readAsDataURL(file); - return deferred.promise; +const ALLOWED_BLOB_MIMETYPES = { + 'image/jpeg': true, + 'image/gif': true, + 'image/png': true, + + 'video/mp4': true, + 'video/webm': true, + 'video/ogg': true, + + 'audio/mp4': true, + 'audio/webm': true, + 'audio/aac': true, + 'audio/mpeg': true, + 'audio/ogg': true, + 'audio/wave': true, + 'audio/wav': true, + 'audio/x-wav': true, + 'audio/x-pn-wav': true, + 'audio/flac': true, + 'audio/x-flac': true, } - /** * Decrypt a file attached to a matrix event. * @param file {Object} The json taken from the matrix event. @@ -61,7 +99,17 @@ export function decryptFile(file) { return encrypt.decryptAttachment(responseData, file); }).then(function(dataArray) { // Turn the array into a Blob and give it the correct MIME-type. - const blob = new Blob([dataArray], {type: file.mimetype}); + + // IMPORTANT: we must not allow scriptable mime-types into Blobs otherwise + // they introduce XSS attacks if the Blob URI is viewed directly in the + // browser (e.g. by copying the URI into a new tab or window.) + // See warning at top of file. + const mimetype = file.mimetype ? file.mimetype.split(";")[0].trim() : ''; + if (!ALLOWED_BLOB_MIMETYPES[mimetype]) { + mimetype = 'application/octet-stream'; + } + + const blob = new Blob([dataArray], {type: mimetype}); return blob; }); } From bffd5bb891dfa0a874d28bd24d5ac7ca0ae54eac Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 29 Apr 2018 03:09:17 +0100 Subject: [PATCH 2/3] fix constness --- src/utils/DecryptFile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/DecryptFile.js b/src/utils/DecryptFile.js index f8327832d1..92c2e3644d 100644 --- a/src/utils/DecryptFile.js +++ b/src/utils/DecryptFile.js @@ -104,7 +104,7 @@ export function decryptFile(file) { // they introduce XSS attacks if the Blob URI is viewed directly in the // browser (e.g. by copying the URI into a new tab or window.) // See warning at top of file. - const mimetype = file.mimetype ? file.mimetype.split(";")[0].trim() : ''; + let mimetype = file.mimetype ? file.mimetype.split(";")[0].trim() : ''; if (!ALLOWED_BLOB_MIMETYPES[mimetype]) { mimetype = 'application/octet-stream'; } From 9c5407c21ffa9021ed3b746a278d9da5693a6672 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 29 Apr 2018 03:17:55 +0100 Subject: [PATCH 3/3] revokeObjectURLs --- src/components/views/messages/MAudioBody.js | 6 ++++++ src/components/views/messages/MImageBody.js | 8 ++++++++ src/components/views/messages/MVideoBody.js | 9 +++++++++ 3 files changed, 23 insertions(+) diff --git a/src/components/views/messages/MAudioBody.js b/src/components/views/messages/MAudioBody.js index 1e9962846c..efc6c4a069 100644 --- a/src/components/views/messages/MAudioBody.js +++ b/src/components/views/messages/MAudioBody.js @@ -69,6 +69,12 @@ export default class MAudioBody extends React.Component { } } + componentWillUnmount() { + if (this.state.decryptedUrl) { + URL.revokeObjectURL(this.state.decryptedUrl); + } + } + render() { const content = this.props.mxEvent.getContent(); diff --git a/src/components/views/messages/MImageBody.js b/src/components/views/messages/MImageBody.js index bb36e9b1ef..c22832467e 100644 --- a/src/components/views/messages/MImageBody.js +++ b/src/components/views/messages/MImageBody.js @@ -71,6 +71,7 @@ export default class extends React.Component { this.context.matrixClient.on('sync', this.onClientSync); } + // FIXME: factor this out and aplpy it to MVideoBody and MAudioBody too! onClientSync(syncState, prevState) { if (this.unmounted) return; // Consider the client reconnected if there is no error with syncing. @@ -206,6 +207,13 @@ export default class extends React.Component { dis.unregister(this.dispatcherRef); this.context.matrixClient.removeListener('sync', this.onClientSync); this._afterComponentWillUnmount(); + + if (this.state.decryptedUrl) { + URL.revokeObjectURL(this.state.decryptedUrl); + } + if (this.state.decryptedThumbnailUrl) { + URL.revokeObjectURL(this.state.decryptedThumbnailUrl); + } } // To be overridden by subclasses (e.g. MStickerBody) for further diff --git a/src/components/views/messages/MVideoBody.js b/src/components/views/messages/MVideoBody.js index 345c3f02e2..5365daee03 100644 --- a/src/components/views/messages/MVideoBody.js +++ b/src/components/views/messages/MVideoBody.js @@ -120,6 +120,15 @@ module.exports = React.createClass({ } }, + componentWillUnmount: function() { + if (this.state.decryptedUrl) { + URL.revokeObjectURL(this.state.decryptedUrl); + } + if (this.state.decryptedThumbnailUrl) { + URL.revokeObjectURL(this.state.decryptedThumbnailUrl); + } + }, + render: function() { const content = this.props.mxEvent.getContent();