Security: Replace Unicode order overrides in attachment names

As a user, when I receive a file attachment, I want to have confidence that the filename I see in the Signal Desktop app is the same as it will be on disk. To prevent user confusion when receiving files with Unicode order override characters, e.g. `test<LTRO>fig.exe` appearing as `testexe.gif`, we replace all occurrences of order overrides (`U+202D` and `U+202E`) with `U+FFFD`. **Changes** - [x] Bump `Attachment` `schemaVersion` to 2. - [x] Replace all Unicode order overrides in `attachment.filename`: `Attachment.replaceUnicodeOrderOverrides`. - [x] Add tests for existing `Attachment.upgradeSchema` - [x] Add tests for existing `Attachment.withSchemaVersion` - [x] Add tests for `Attachment.replaceUnicodeOrderOverrides` positives. - [x] Add `testcheck` generative property-based testing library (based on QuickCheck) to ensure valid filenames are preserved. --- commit 855bdbc7e647e44f73b9e1f5e6d64f734c61169a Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 13:02:01 2018 -0500 Log error stack in case of error commit 6e053ed66aee136f186568fa88aacd4814b2ab07 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 12:30:28 2018 -0500 Improve `upgradeStep` error handling commit 8c226a2523b701cb578b2137832c3eaf3475bb2b Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 12:30:08 2018 -0500 Check for expected version before upgrade Prevents out of order upgrade steps. commit 28b0675591e782169128f75429b7bab2a22307fa Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 12:29:52 2018 -0500 Reject invalid attachments commit 41f4f457dae9416dae66dc2fa2079483d1f127a9 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 12:29:36 2018 -0500 Fix upgrade pipeline order commit 3935629e91c49b8d96c1e02bd37b1b31d1180720 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 12:28:25 2018 -0500 Avoid `_.isPlainObject` Attachments are deserialized from a protocol buffer and can have a non-plain-object constructor. commit 39f6e7f622ff4885e2ccafa354e0edb5864c55d8 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 12:19:07 2018 -0500 Define basic attachment validity commit adcf7e3243cd90866cc35990c558ff7829019037 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Thu Feb 22 12:18:54 2018 -0500 Add tests for attachment upgrade pipeline commit 82fc4644d7e654eea9f348518b086497be2b0cb4 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Wed Feb 21 12:20:24 2018 -0500 Favor `async` / `await` over `then` commit 8fe49e3c40e78ced0b8f2eb0b678f4bae842855d Author: Daniel Gasienica <daniel@gasienica.ch> Date: Wed Feb 21 12:19:59 2018 -0500 Add `eslint-more` plugin This will enable us to disallow `then` in favor of `async` / `await`. commit 020beefb25f508ae96cf3fc099599fbbca98802b Author: Daniel Gasienica <daniel@gasienica.ch> Date: Wed Feb 21 11:31:49 2018 -0500 Remove unnecessary `async` modifiers commit 177090c5f5ad9836f0ca0a5c2f298779519e3692 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Wed Feb 21 11:30:55 2018 -0500 Document `operator-linebreak` ESLint rule commit 25622b7c59291cb672ae057c47e7327a564cca40 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Wed Feb 21 11:14:15 2018 -0500 Prefix internal function with `_` commit 6aa3cf5098df71e9b710064739ec49d74f81b7bf Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 19:00:07 2018 -0500 Replace all Unicode order override occurrences commit fd6e23b0a519bce3c12c5b9ac676bcd198034fed Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:48:41 2018 -0500 Whitelist `testcheck` `check` and `gen` globals commit 400bae9fac5078821813bc0ca17a5d7a72900161 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:46:57 2018 -0500 🎨 Fix lint errors commit da53d3960aa7aa36b7cc1fcff414c9e929c0d9fc Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:42:42 2018 -0500 Add tests for `Attachment.withSchemaVersion` commit ec203444239d9e3c443ba88cab7ef4672151072d Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:42:17 2018 -0500 Add test for `Attachment.upgradeSchema` commit 4540d5bdf7a4279f49d2e4c6ee03f47b93df46bf Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:05:29 2018 -0500 Rename `setSchemaVersion` --> `withSchemaVersion` Put the schema version first for better readability. commit e379cf919feda31d1fa96d406c30fd38e159a11d Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:03:22 2018 -0500 Add filename sanitization to upgrade pipeline commit 1e344a0d15926fc3e17be20cd90bfa882b65f337 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:01:55 2018 -0500 Test that we preserve non-suspicious filenames commit a2452bfc98f93f82bed48b438757af2e66a6af82 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 17:00:56 2018 -0500 Add `testcheck` dependency Allows for generative property-based testing similar to Haskell’s QuickCheck. See: https://medium.com/javascript-inside/f91432247c27 commit ceb5bfd2484a77689fdb8e9edd18d4a7b093a486 Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 16:15:33 2018 -0500 Replace Unicode order override characters Prevents users from being tricked into clicking a file named `testexe.fig` that appears as `testexe.gif` due to a Unicode order override character. See: - http://unicode.org/reports/tr36/#Bidirectional_Text_Spoofing - https://krebsonsecurity.com/2011/09/right-to-left-override-aids-email-attacks/ commit bc605afb1c6af3a5ebc31a4c1523ff170eb96ffe Author: Daniel Gasienica <daniel@gasienica.ch> Date: Fri Feb 16 16:12:29 2018 -0500 Remove `CURRENT_PROCESS_VERSION` Reintroduce this whenever we need it. We currently only deal with schema version numbers within this module.
2018-02-22 13:21:53 -05:00 · 2018-02-22 13:21:53 -05:00 · a1ac810343
commit a1ac810343
parent 06a16baaa5
10 changed files with 409 additions and 27 deletions
--- a/js/modules/types/attachment.js
+++ b/js/modules/types/attachment.js
@ -1,18 +1,25 @@
+const isFunction = require('lodash/isFunction');
+const isNumber = require('lodash/isNumber');
+const isString = require('lodash/isString');
+const isUndefined = require('lodash/isUndefined');
+
 const MIME = require('./mime');
 const { arrayBufferToBlob, blobToArrayBuffer, dataURLToBlob } = require('blob-util');
 const { autoOrientImage } = require('../auto_orient_image');

-// Increment this everytime we change how attachments are upgraded. This allows us to
-// retroactively upgrade existing attachments. As we add more upgrade steps, we could
-// design a pipeline that does this incrementally, e.g. from version 0 (unknown) -> 1,
-// 1 --> 2, etc., similar to how we do database migrations:
-const CURRENT_PROCESS_VERSION = 1;
+// Increment this version number every time we change how attachments are upgraded. This
+// will allow us to retroactively upgrade existing attachments. As we add more upgrade
+// steps, we could design a pipeline that does this incrementally, e.g. from
+// version 0 / unknown -> 1, 1 --> 2, etc., similar to how we do database migrations:
+exports.CURRENT_SCHEMA_VERSION = 2;

 // Schema version history
 //
 // Version 1
 //   - Auto-orient JPEG attachments using EXIF `Orientation` data
 //   - Add `schemaVersion` property
+// Version 2
+//   - Sanitize Unicode order override characters

 // // Incoming message attachment fields
 // {
@ -37,34 +44,81 @@ const CURRENT_PROCESS_VERSION = 1;
 //   schemaVersion: integer
 // }

+// Returns true if `rawAttachment` is a valid attachment based on our (limited)
+// criteria. Over time, we can expand this definition to become more narrow:
+exports.isValid = (rawAttachment) => {
+  // NOTE: We cannot use `_.isPlainObject` because `rawAttachment` is
+  // deserialized by protobuf:
+  if (!rawAttachment) {
+    return false;
+  }
+
+  return isString(rawAttachment.contentType) &&
+    isString(rawAttachment.fileName);
+};
+
 // Middleware
 // type UpgradeStep = Attachment -> Promise Attachment

-// UpgradeStep -> SchemaVersion -> UpgradeStep
-const setSchemaVersion = (next, schemaVersion) => async (attachment) => {
-  const isAlreadyUpgraded = attachment.schemaVersion >= schemaVersion;
-  if (isAlreadyUpgraded) {
-    return attachment;
+// SchemaVersion -> UpgradeStep -> UpgradeStep
+exports.withSchemaVersion = (schemaVersion, upgrade) => {
+  if (!isNumber(schemaVersion)) {
+    throw new TypeError('`schemaVersion` must be a number');
+  }
+  if (!isFunction(upgrade)) {
+    throw new TypeError('`upgrade` must be a function');
  }

-  let upgradedAttachment;
-  try {
-    upgradedAttachment = await next(attachment);
-  } catch (error) {
-    console.error('Attachment.setSchemaVersion: error:', error);
-    upgradedAttachment = null;
-  }
+  return async (attachment) => {
+    if (!exports.isValid(attachment)) {
+      console.log('Attachment.withSchemaVersion: Invalid input attachment:', attachment);
+      return attachment;
+    }

-  const hasSuccessfullyUpgraded = upgradedAttachment !== null;
-  if (!hasSuccessfullyUpgraded) {
-    return attachment;
-  }
+    const isAlreadyUpgraded = attachment.schemaVersion >= schemaVersion;
+    if (isAlreadyUpgraded) {
+      return attachment;
+    }

-  return Object.assign(
-    {},
-    upgradedAttachment,
-    { schemaVersion }
-  );
+    const expectedVersion = schemaVersion - 1;
+    const isUnversioned = isUndefined(attachment.schemaVersion);
+    const hasExpectedVersion = isUnversioned ||
+      attachment.schemaVersion === expectedVersion;
+    if (!hasExpectedVersion) {
+      console.log(
+        'WARNING: Attachment.withSchemaVersion: Unexpected version:' +
+        ` Expected attachment to have version ${expectedVersion},` +
+        ` but got ${attachment.schemaVersion}.`,
+        attachment
+      );
+      return attachment;
+    }
+
+    let upgradedAttachment;
+    try {
+      upgradedAttachment = await upgrade(attachment);
+    } catch (error) {
+      console.log(
+        'Attachment.withSchemaVersion: error:',
+        error && error.stack ? error.stack : error
+      );
+      return attachment;
+    }
+
+    if (!exports.isValid(upgradedAttachment)) {
+      console.log(
+        'Attachment.withSchemaVersion: Invalid upgraded attachment:',
+        upgradedAttachment
+      );
+      return attachment;
+    }
+
+    return Object.assign(
+      {},
+      upgradedAttachment,
+      { schemaVersion }
+    );
+  };
 };

 // Upgrade steps
@ -93,6 +147,39 @@ const autoOrientJPEG = async (attachment) => {
  return newAttachment;
 };

+const UNICODE_LEFT_TO_RIGHT_OVERRIDE = '\u202D';
+const UNICODE_RIGHT_TO_LEFT_OVERRIDE = '\u202E';
+const UNICODE_REPLACEMENT_CHARACTER = '\uFFFD';
+const INVALID_CHARACTERS_PATTERN = new RegExp(
+  `[${UNICODE_LEFT_TO_RIGHT_OVERRIDE}${UNICODE_RIGHT_TO_LEFT_OVERRIDE}]`,
+  'g'
+);
+// NOTE: Expose synchronous version to do property-based testing using `testcheck`,
+// which currently doesn’t support async testing:
+// https://github.com/leebyron/testcheck-js/issues/45
+exports._replaceUnicodeOrderOverridesSync = (attachment) => {
+  if (!isString(attachment.fileName)) {
+    return attachment;
+  }
+
+  const normalizedFilename = attachment.fileName.replace(
+    INVALID_CHARACTERS_PATTERN,
+    UNICODE_REPLACEMENT_CHARACTER
+  );
+  const newAttachment = Object.assign({}, attachment, {
+    fileName: normalizedFilename,
+  });
+
+  return newAttachment;
+};
+
+exports.replaceUnicodeOrderOverrides = async attachment =>
+  exports._replaceUnicodeOrderOverridesSync(attachment);
+
 // Public API
+const toVersion1 = exports.withSchemaVersion(1, autoOrientJPEG);
+const toVersion2 = exports.withSchemaVersion(2, exports.replaceUnicodeOrderOverrides);
+
 // UpgradeStep
-exports.upgradeSchema = setSchemaVersion(autoOrientJPEG, CURRENT_PROCESS_VERSION);
+exports.upgradeSchema = async attachment =>
+  toVersion2(await toVersion1(attachment));