Merge remote-tracking branch 'origin/develop' into develop

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,
@@ -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();
 

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";
@@ -72,6 +72,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.
@@ -181,14 +182,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,
@@ -217,6 +218,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

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,
@@ -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();
 

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.
+        let 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;
     });
 }