Fix cipher upload (#346)

* Upload correct data array

* Require BufferArray Encryption for upload to server

The CipherArrayBuffer tiny type is only created by CryptoService
and required by all upload methods

* Add test for attachment upload encryption

spec/common/services/cipher.service.spec.ts

@@ -0,0 +1,61 @@
+import { Arg, Substitute, SubstituteOf } from '@fluffy-spoon/substitute';
+
+import { ApiService } from '../../../src/abstractions/api.service';
+import { CryptoService } from '../../../src/abstractions/crypto.service';
+import { FileUploadService } from '../../../src/abstractions/fileUpload.service';
+import { I18nService } from '../../../src/abstractions/i18n.service';
+import { SearchService } from '../../../src/abstractions/search.service';
+import { SettingsService } from '../../../src/abstractions/settings.service';
+import { StorageService } from '../../../src/abstractions/storage.service';
+import { UserService } from '../../../src/abstractions/user.service';
+import { Utils } from '../../../src/misc/utils';
+import { Cipher } from '../../../src/models/domain/cipher';
+import { CipherArrayBuffer } from '../../../src/models/domain/cipherArrayBuffer';
+import { CipherString } from '../../../src/models/domain/cipherString';
+import { SymmetricCryptoKey } from '../../../src/models/domain/symmetricCryptoKey';
+
+import { CipherService } from '../../../src/services/cipher.service';
+
+const ENCRYPTED_TEXT = 'This data has been encrypted';
+const ENCRYPTED_BYTES = new CipherArrayBuffer(Utils.fromUtf8ToArray(ENCRYPTED_TEXT).buffer);
+
+describe('Cipher Service', () => {
+    let cryptoService: SubstituteOf<CryptoService>;
+    let userService: SubstituteOf<UserService>;
+    let settingsService: SubstituteOf<SettingsService>;
+    let apiService: SubstituteOf<ApiService>;
+    let fileUploadService: SubstituteOf<FileUploadService>;
+    let storageService: SubstituteOf<StorageService>;
+    let i18nService: SubstituteOf<I18nService>;
+    let searchService: SubstituteOf<SearchService>;
+
+    let cipherService: CipherService;
+
+    beforeEach(() => {
+        cryptoService = Substitute.for<CryptoService>();
+        userService = Substitute.for<UserService>();
+        settingsService = Substitute.for<SettingsService>();
+        apiService = Substitute.for<ApiService>();
+        fileUploadService = Substitute.for<FileUploadService>();
+        storageService = Substitute.for<StorageService>();
+        i18nService = Substitute.for<I18nService>();
+        searchService = Substitute.for<SearchService>();
+
+        cryptoService.encryptToBytes(Arg.any(), Arg.any()).resolves(ENCRYPTED_BYTES);
+        cryptoService.encrypt(Arg.any(), Arg.any()).resolves(new CipherString(ENCRYPTED_TEXT));
+
+        cipherService = new CipherService(cryptoService, userService, settingsService, apiService, fileUploadService,
+            storageService, i18nService, () => searchService);
+    });
+
+    it('attachments upload encrypted file contents', async () => {
+        const key = new SymmetricCryptoKey(new Uint8Array(32).buffer);
+        const fileName = 'filename';
+        const fileData = new Uint8Array(10).buffer;
+        cryptoService.getOrgKey(Arg.any()).resolves(new SymmetricCryptoKey(new Uint8Array(32).buffer));
+
+        await cipherService.saveAttachmentRawWithServer(new Cipher(), fileName, fileData);
+
+        fileUploadService.received(1).uploadCipherAttachment(Arg.any(), Arg.any(), fileName, ENCRYPTED_BYTES);
+    });
+});

src/abstractions/crypto.service.ts

@@ -1,3 +1,4 @@
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
 import { CipherString } from '../models/domain/cipherString';
 import { SymmetricCryptoKey } from '../models/domain/symmetricCryptoKey';
 
@@ -40,7 +41,7 @@ export abstract class CryptoService {
     makeEncKey: (key: SymmetricCryptoKey) => Promise<[SymmetricCryptoKey, CipherString]>;
     remakeEncKey: (key: SymmetricCryptoKey, encKey?: SymmetricCryptoKey) => Promise<[SymmetricCryptoKey, CipherString]>;
     encrypt: (plainValue: string | ArrayBuffer, key?: SymmetricCryptoKey) => Promise<CipherString>;
-    encryptToBytes: (plainValue: ArrayBuffer, key?: SymmetricCryptoKey) => Promise<ArrayBuffer>;
+    encryptToBytes: (plainValue: ArrayBuffer, key?: SymmetricCryptoKey) => Promise<CipherArrayBuffer>;
     rsaEncrypt: (data: ArrayBuffer, publicKey?: ArrayBuffer) => Promise<CipherString>;
     rsaDecrypt: (encValue: string) => Promise<ArrayBuffer>;
     decryptToBytes: (cipherString: CipherString, key?: SymmetricCryptoKey) => Promise<ArrayBuffer>;

src/abstractions/fileUpload.service.ts

@@ -1,10 +1,11 @@
 import { CipherString } from '../models/domain';
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
 import { AttachmentUploadDataResponse } from '../models/response/attachmentUploadDataResponse';
 import { SendFileUploadDataResponse } from '../models/response/sendFileUploadDataResponse';
 
 export abstract class FileUploadService {
     uploadSendFile: (uploadData: SendFileUploadDataResponse, fileName: CipherString,
-        encryptedFileData: ArrayBuffer) => Promise<any>;
+        encryptedFileData: CipherArrayBuffer) => Promise<any>;
     uploadCipherAttachment: (admin: boolean, uploadData: AttachmentUploadDataResponse, fileName: string,
-        encryptedFileData: ArrayBuffer) => Promise<any>;
+        encryptedFileData: CipherArrayBuffer) => Promise<any>;
 }

src/abstractions/send.service.ts

@@ -1,5 +1,6 @@
 import { SendData } from '../models/data/sendData';
 
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
 import { Send } from '../models/domain/send';
 import { SymmetricCryptoKey } from '../models/domain/symmetricCryptoKey';
 
@@ -9,11 +10,11 @@ export abstract class SendService {
     decryptedSendCache: SendView[];
 
     clearCache: () => void;
-    encrypt: (model: SendView, file: File | ArrayBuffer, password: string, key?: SymmetricCryptoKey) => Promise<[Send, ArrayBuffer]>;
+    encrypt: (model: SendView, file: File | ArrayBuffer, password: string, key?: SymmetricCryptoKey) => Promise<[Send, CipherArrayBuffer]>;
     get: (id: string) => Promise<Send>;
     getAll: () => Promise<Send[]>;
     getAllDecrypted: () => Promise<SendView[]>;
-    saveWithServer: (sendData: [Send, ArrayBuffer]) => Promise<any>;
+    saveWithServer: (sendData: [Send, CipherArrayBuffer]) => Promise<any>;
     upsert: (send: SendData | SendData[]) => Promise<any>;
     replace: (sends: { [id: string]: SendData; }) => Promise<any>;
     clear: (userId: string) => Promise<any>;

src/angular/components/send/add-edit.component.ts

@@ -24,6 +24,7 @@ import { SendFileView } from '../../../models/view/sendFileView';
 import { SendTextView } from '../../../models/view/sendTextView';
 import { SendView } from '../../../models/view/sendView';
 
+import { CipherArrayBuffer } from '../../../models/domain/cipherArrayBuffer';
 import { Send } from '../../../models/domain/send';
 
 // TimeOption is used for the dropdown implementation of custom times
@@ -385,7 +386,7 @@ export class AddEditComponent implements OnInit {
         return this.sendService.get(this.sendId);
     }
 
-    protected async encryptSend(file: File): Promise<[Send, ArrayBuffer]> {
+    protected async encryptSend(file: File): Promise<[Send, CipherArrayBuffer]> {
         const sendData = await this.sendService.encrypt(this.send, file, this.password, null);
 
         // Parse dates

src/models/domain/cipherArrayBuffer.ts

@@ -0,0 +1,3 @@
+export class CipherArrayBuffer {
+    constructor(public buffer: ArrayBuffer) { }
+}

src/services/azureFileUpload.service.ts

@@ -2,30 +2,32 @@ import { LogService } from '../abstractions/log.service';
 
 import { Utils } from '../misc/utils';
 
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
+
 const MAX_SINGLE_BLOB_UPLOAD_SIZE = 256 * 1024 * 1024; // 256 MiB
 const MAX_BLOCKS_PER_BLOB = 50000;
 
 export class AzureFileUploadService {
     constructor(private logService: LogService) { }
 
-    async upload(url: string, data: ArrayBuffer, renewalCallback: () => Promise<string>) {
+    async upload(url: string, data: CipherArrayBuffer, renewalCallback: () => Promise<string>) {
-        if (data.byteLength <= MAX_SINGLE_BLOB_UPLOAD_SIZE) {
+        if (data.buffer.byteLength <= MAX_SINGLE_BLOB_UPLOAD_SIZE) {
             return await this.azureUploadBlob(url, data);
         } else {
             return await this.azureUploadBlocks(url, data, renewalCallback);
         }
     }
-    private async azureUploadBlob(url: string, data: ArrayBuffer) {
+    private async azureUploadBlob(url: string, data: CipherArrayBuffer) {
         const urlObject = Utils.getUrl(url);
         const headers = new Headers({
             'x-ms-date': new Date().toUTCString(),
             'x-ms-version': urlObject.searchParams.get('sv'),
-            'Content-Length': data.byteLength.toString(),
+            'Content-Length': data.buffer.byteLength.toString(),
             'x-ms-blob-type': 'BlockBlob',
         });
 
         const request = new Request(url, {
-            body: data,
+            body: data.buffer,
             cache: 'no-store',
             method: 'PUT',
             headers: headers,
@@ -37,11 +39,11 @@ export class AzureFileUploadService {
             throw new Error(`Failed to create Azure blob: ${blobResponse.status}`);
         }
     }
-    private async azureUploadBlocks(url: string, data: ArrayBuffer, renewalCallback: () => Promise<string>) {
+    private async azureUploadBlocks(url: string, data: CipherArrayBuffer, renewalCallback: () => Promise<string>) {
         const baseUrl = Utils.getUrl(url);
         const blockSize = this.getMaxBlockSize(baseUrl.searchParams.get('sv'));
         let blockIndex = 0;
-        const numBlocks = Math.ceil(data.byteLength / blockSize);
+        const numBlocks = Math.ceil(data.buffer.byteLength / blockSize);
         const blocksStaged: string[] = [];
 
         if (numBlocks > MAX_BLOCKS_PER_BLOB) {
@@ -56,7 +58,7 @@ export class AzureFileUploadService {
                 blockUrl.searchParams.append('comp', 'block');
                 blockUrl.searchParams.append('blockid', blockId);
                 const start = blockIndex * blockSize;
-                const blockData = data.slice(start, start + blockSize);
+                const blockData = data.buffer.slice(start, start + blockSize);
                 const blockHeaders = new Headers({
                     'x-ms-date': new Date().toUTCString(),
                     'x-ms-version': blockUrl.searchParams.get('sv'),

src/services/bitwardenFileUpload.service.ts

@@ -1,19 +1,21 @@
 import { ApiService } from '../abstractions/api.service';
 
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
+
 import { Utils } from '../misc/utils';
 
 export class BitwardenFileUploadService
 {
     constructor(private apiService: ApiService) { }
 
-    async upload(encryptedFileName: string, encryptedFileData: ArrayBuffer, apiCall: (fd: FormData) => Promise<any>) {
+    async upload(encryptedFileName: string, encryptedFileData: CipherArrayBuffer, apiCall: (fd: FormData) => Promise<any>) {
         const fd = new FormData();
         try {
-            const blob = new Blob([encryptedFileData], { type: 'application/octet-stream' });
+            const blob = new Blob([encryptedFileData.buffer], { type: 'application/octet-stream' });
             fd.append('data', blob, encryptedFileName);
         } catch (e) {
             if (Utils.isNode && !Utils.isBrowser) {
-                fd.append('data', Buffer.from(encryptedFileData) as any, {
+                fd.append('data', Buffer.from(encryptedFileData.buffer) as any, {
                     filepath: encryptedFileName,
                     contentType: 'application/octet-stream',
                 } as any);

src/services/cipher.service.ts

@@ -7,6 +7,7 @@ import { CipherData } from '../models/data/cipherData';
 import { Attachment } from '../models/domain/attachment';
 import { Card } from '../models/domain/card';
 import { Cipher } from '../models/domain/cipher';
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
 import { CipherString } from '../models/domain/cipherString';
 import Domain from '../models/domain/domainBase';
 import { Field } from '../models/domain/field';
@@ -17,6 +18,7 @@ import { Password } from '../models/domain/password';
 import { SecureNote } from '../models/domain/secureNote';
 import { SymmetricCryptoKey } from '../models/domain/symmetricCryptoKey';
 
+import { AttachmentRequest } from '../models/request/attachmentRequest';
 import { CipherBulkDeleteRequest } from '../models/request/cipherBulkDeleteRequest';
 import { CipherBulkMoveRequest } from '../models/request/cipherBulkMoveRequest';
 import { CipherBulkRestoreRequest } from '../models/request/cipherBulkRestoreRequest';
@@ -51,7 +53,6 @@ import { ConstantsService } from './constants.service';
 
 import { sequentialize } from '../misc/sequentialize';
 import { Utils } from '../misc/utils';
-import { AttachmentRequest } from '../models/request/attachmentRequest';
 
 const Keys = {
     ciphersPrefix: 'ciphers_',
@@ -623,7 +624,7 @@ export class CipherService implements CipherServiceAbstraction {
         const request: AttachmentRequest = {
             key: dataEncKey[1].encryptedString,
             fileName: encFileName.encryptedString,
-            fileSize: encData.byteLength,
+            fileSize: encData.buffer.byteLength,
             adminRequest: admin,
         };
 
@@ -631,7 +632,7 @@ export class CipherService implements CipherServiceAbstraction {
         try {
             const uploadDataResponse = await this.apiService.postCipherAttachment(cipher.id, request);
             response = admin ? uploadDataResponse.cipherMiniResponse : uploadDataResponse.cipherResponse;
-            await this.fileUploadService.uploadCipherAttachment(admin, uploadDataResponse, filename, data);
+            await this.fileUploadService.uploadCipherAttachment(admin, uploadDataResponse, filename, encData);
         } catch (e) {
             if (e instanceof ErrorResponse && (e as ErrorResponse).statusCode === 404 || (e as ErrorResponse).statusCode === 405) {
                 response = await this.legacyServerAttachmentFileUpload(admin, cipher.id, encFileName, encData, dataEncKey[1]);
@@ -655,16 +656,16 @@ export class CipherService implements CipherServiceAbstraction {
      * This method still exists for backward compatibility with old server versions.
      */
     async legacyServerAttachmentFileUpload(admin: boolean, cipherId: string, encFileName: CipherString,
-        encData: ArrayBuffer, key: CipherString) {
+        encData: CipherArrayBuffer, key: CipherString) {
         const fd = new FormData();
         try {
-            const blob = new Blob([encData], { type: 'application/octet-stream' });
+            const blob = new Blob([encData.buffer], { type: 'application/octet-stream' });
             fd.append('key', key.encryptedString);
             fd.append('data', blob, encFileName.encryptedString);
         } catch (e) {
             if (Utils.isNode && !Utils.isBrowser) {
                 fd.append('key', key.encryptedString);
-                fd.append('data', Buffer.from(encData) as any, {
+                fd.append('data', Buffer.from(encData.buffer) as any, {
                     filepath: encFileName.encryptedString,
                     contentType: 'application/octet-stream',
                 } as any);
@@ -970,13 +971,13 @@ export class CipherService implements CipherServiceAbstraction {
 
         const fd = new FormData();
         try {
-            const blob = new Blob([encData], { type: 'application/octet-stream' });
+            const blob = new Blob([encData.buffer], { type: 'application/octet-stream' });
             fd.append('key', dataEncKey[1].encryptedString);
             fd.append('data', blob, encFileName.encryptedString);
         } catch (e) {
             if (Utils.isNode && !Utils.isBrowser) {
                 fd.append('key', dataEncKey[1].encryptedString);
-                fd.append('data', Buffer.from(encData) as any, {
+                fd.append('data', Buffer.from(encData.buffer) as any, {
                     filepath: encFileName.encryptedString,
                     contentType: 'application/octet-stream',
                 } as any);

src/services/crypto.service.ts

@@ -3,6 +3,7 @@ import * as bigInt from 'big-integer';
 import { EncryptionType } from '../enums/encryptionType';
 import { KdfType } from '../enums/kdfType';
 
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
 import { CipherString } from '../models/domain/cipherString';
 import { EncryptedObject } from '../models/domain/encryptedObject';
 import { SymmetricCryptoKey } from '../models/domain/symmetricCryptoKey';
@@ -406,7 +407,7 @@ export class CryptoService implements CryptoServiceAbstraction {
         return new CipherString(encObj.key.encType, data, iv, mac);
     }
 
-    async encryptToBytes(plainValue: ArrayBuffer, key?: SymmetricCryptoKey): Promise<ArrayBuffer> {
+    async encryptToBytes(plainValue: ArrayBuffer, key?: SymmetricCryptoKey): Promise<CipherArrayBuffer> {
         const encValue = await this.aesEncrypt(plainValue, key);
         let macLen = 0;
         if (encValue.mac != null) {
@@ -421,7 +422,7 @@ export class CryptoService implements CryptoServiceAbstraction {
         }
 
         encBytes.set(new Uint8Array(encValue.data), 1 + encValue.iv.byteLength + macLen);
-        return encBytes.buffer;
+        return new CipherArrayBuffer(encBytes.buffer);
     }
 
     async rsaEncrypt(data: ArrayBuffer, publicKey?: ArrayBuffer): Promise<CipherString> {

src/services/fileUpload.service.ts

@@ -4,7 +4,9 @@ import { LogService } from '../abstractions/log.service';
 
 import { FileUploadType } from '../enums/fileUploadType';
 
-import { CipherString } from '../models/domain';
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
+import { CipherString } from '../models/domain/cipherString';
+
 import { AttachmentUploadDataResponse } from '../models/response/attachmentUploadDataResponse';
 import { SendFileUploadDataResponse } from '../models/response/sendFileUploadDataResponse';
 
@@ -20,7 +22,7 @@ export class FileUploadService implements FileUploadServiceAbstraction {
         this.bitwardenFileUploadService = new BitwardenFileUploadService(apiService);
     }
 
-    async uploadSendFile(uploadData: SendFileUploadDataResponse, fileName: CipherString, encryptedFileData: ArrayBuffer) {
+    async uploadSendFile(uploadData: SendFileUploadDataResponse, fileName: CipherString, encryptedFileData: CipherArrayBuffer) {
         try {
             switch (uploadData.fileUploadType) {
                 case FileUploadType.Direct:
@@ -45,7 +47,7 @@ export class FileUploadService implements FileUploadServiceAbstraction {
         }
     }
 
-    async uploadCipherAttachment(admin: boolean, uploadData: AttachmentUploadDataResponse, encryptedFileName: string, encryptedFileData: ArrayBuffer) {
+    async uploadCipherAttachment(admin: boolean, uploadData: AttachmentUploadDataResponse, encryptedFileName: string, encryptedFileData: CipherArrayBuffer) {
         const response = admin ? uploadData.cipherMiniResponse : uploadData.cipherResponse;
         try {
             switch (uploadData.fileUploadType) {

src/services/send.service.ts

@@ -2,8 +2,11 @@ import { SendData } from '../models/data/sendData';
 
 import { SendRequest } from '../models/request/sendRequest';
 
+import { ErrorResponse } from '../models/response/errorResponse';
 import { SendResponse } from '../models/response/sendResponse';
 
+import { CipherArrayBuffer } from '../models/domain/cipherArrayBuffer';
+import { CipherString } from '../models/domain/cipherString';
 import { Send } from '../models/domain/send';
 import { SendFile } from '../models/domain/sendFile';
 import { SendText } from '../models/domain/sendText';
@@ -24,8 +27,6 @@ import { StorageService } from '../abstractions/storage.service';
 import { UserService } from '../abstractions/user.service';
 
 import { Utils } from '../misc/utils';
-import { CipherString } from '../models/domain';
-import { ErrorResponse } from '../models/response';
 
 const Keys = {
     sendsPrefix: 'sends_',
@@ -44,8 +45,8 @@ export class SendService implements SendServiceAbstraction {
     }
 
     async encrypt(model: SendView, file: File | ArrayBuffer, password: string,
-        key?: SymmetricCryptoKey): Promise<[Send, ArrayBuffer]> {
+        key?: SymmetricCryptoKey): Promise<[Send, CipherArrayBuffer]> {
-        let fileData: ArrayBuffer = null;
+        let fileData: CipherArrayBuffer = null;
         const send = new Send();
         send.id = model.id;
         send.type = model.type;
@@ -131,8 +132,8 @@ export class SendService implements SendServiceAbstraction {
         return this.decryptedSendCache;
     }
 
-    async saveWithServer(sendData: [Send, ArrayBuffer]): Promise<any> {
+    async saveWithServer(sendData: [Send, CipherArrayBuffer]): Promise<any> {
-        const request = new SendRequest(sendData[0], sendData[1]?.byteLength);
+        const request = new SendRequest(sendData[0], sendData[1]?.buffer.byteLength);
         let response: SendResponse;
         if (sendData[0].id == null) {
             if (sendData[0].type === SendType.Text) {
@@ -168,17 +169,17 @@ export class SendService implements SendServiceAbstraction {
      * @deprecated Mar 25 2021: This method has been deprecated in favor of direct uploads.
      * This method still exists for backward compatibility with old server versions.
      */
-    async legacyServerSendFileUpload(sendData: [Send, ArrayBuffer], request: SendRequest): Promise<SendResponse>
+    async legacyServerSendFileUpload(sendData: [Send, CipherArrayBuffer], request: SendRequest): Promise<SendResponse>
     {
         const fd = new FormData();
         try {
-            const blob = new Blob([sendData[1]], { type: 'application/octet-stream' });
+            const blob = new Blob([sendData[1].buffer], { type: 'application/octet-stream' });
             fd.append('model', JSON.stringify(request));
             fd.append('data', blob, sendData[0].file.fileName.encryptedString);
         } catch (e) {
             if (Utils.isNode && !Utils.isBrowser) {
                 fd.append('model', JSON.stringify(request));
-                fd.append('data', Buffer.from(sendData[1]) as any, {
+                fd.append('data', Buffer.from(sendData[1].buffer) as any, {
                     filepath: sendData[0].file.fileName.encryptedString,
                     contentType: 'application/octet-stream',
                 } as any);
@@ -256,7 +257,7 @@ export class SendService implements SendServiceAbstraction {
         await this.upsert(data);
     }
 
-    private parseFile(send: Send, file: File, key: SymmetricCryptoKey): Promise<ArrayBuffer> {
+    private parseFile(send: Send, file: File, key: SymmetricCryptoKey): Promise<CipherArrayBuffer> {
         return new Promise((resolve, reject) => {
             const reader = new FileReader();
             reader.readAsArrayBuffer(file);
@@ -276,7 +277,7 @@ export class SendService implements SendServiceAbstraction {
     }
 
     private async encryptFileData(fileName: string, data: ArrayBuffer,
-        key: SymmetricCryptoKey): Promise<[CipherString, ArrayBuffer]> {
+        key: SymmetricCryptoKey): Promise<[CipherString, CipherArrayBuffer]> {
         const encFileName = await this.cryptoService.encrypt(fileName, key);
         const encFileData = await this.cryptoService.encryptToBytes(data, key);
         return [encFileName, encFileData];