#359 - fixed unauthorized access of data through dataCid reference in RecordsWrite

Author: thehenrytsai

README.md

@@ -3,11 +3,11 @@
 # Decentralized Web Node (DWN) SDK
 
 Code Coverage
-![Statements](https://img.shields.io/badge/statements-94.74%25-brightgreen.svg?style=flat) ![Branches](https://img.shields.io/badge/branches-94.13%25-brightgreen.svg?style=flat) ![Functions](https://img.shields.io/badge/functions-92.48%25-brightgreen.svg?style=flat) ![Lines](https://img.shields.io/badge/lines-94.74%25-brightgreen.svg?style=flat)
+![Statements](https://img.shields.io/badge/statements-94.69%25-brightgreen.svg?style=flat) ![Branches](https://img.shields.io/badge/branches-94.15%25-brightgreen.svg?style=flat) ![Functions](https://img.shields.io/badge/functions-93.15%25-brightgreen.svg?style=flat) ![Lines](https://img.shields.io/badge/lines-94.69%25-brightgreen.svg?style=flat)
 
 ## Introduction
 
-This repository contains a reference implementation of Decentralized Web Node (DWN) as per the [specification](https://identity.foundation/decentralized-web-node/spec/). This specification is in a draft state and very much so a WIP. For the foreseeable future, a lot of the work on DWN will be split across this repo and the repo that houses the specification, which you can find [here](https://github.com/decentralized-identity/decentralized-web-node). The current goal is to produce a beta implementation by March 2023. This won't include all interfaces described in the DWN spec, but will be enough to begin building applications.
+This repository contains a reference implementation of Decentralized Web Node (DWN) as per the [specification](https://identity.foundation/decentralized-web-node/spec/). This specification is in a draft state and very much so a WIP. For the foreseeable future, a lot of the work on DWN will be split across this repo and the repo that houses the specification, which you can find [here](https://github.com/decentralized-identity/decentralized-web-node). The current implementation does not include all interfaces described in the DWN spec, but is enough to begin building test applications.
 
 This project is used as a dependency by several other projects.
 

src/core/dwn-error.ts

@@ -29,10 +29,10 @@ export enum DwnErrorCode {
   RecordsProtocolsDerivationSchemeMissingProtocol = 'RecordsProtocolsDerivationSchemeMissingProtocol',
   RecordsSchemasDerivationSchemeMissingSchema = 'RecordsSchemasDerivationSchemeMissingSchema',
   RecordsWriteGetEntryIdUndefinedAuthor = 'RecordsWriteGetEntryIdUndefinedAuthor',
+  RecordsWriteMissingDataStream = 'RecordsWriteMissingDataStream',
   RecordsWriteValidateIntegrityEncryptionCidMismatch = 'RecordsWriteValidateIntegrityEncryptionCidMismatch',
   Secp256k1KeyNotValid = 'Secp256k1KeyNotValid',
   StorageControllerDataCidMismatch = 'StorageControllerDataCidMismatch',
-  StorageControllerDataNotFound = 'StorageControllerDataNotFound',
   StorageControllerDataSizeMismatch = 'StorageControllerDataSizeMismatch',
   UrlProtocolNotNormalized = 'UrlProtocolNotNormalized',
   UrlProtocolNotNormalizable = 'UrlProtocolNotNormalizable',

src/interfaces/records/handlers/pruned-initial-records-write.ts

@@ -1,7 +1,7 @@
-import type { BaseMessage } from '../../../core/types.js';
 import type { EventLog } from '../../../event-log/event-log.js';
 import type { Readable } from 'readable-stream';
-import type { DataStore, MessageStore } from '../../../index.js';
+import type { BaseMessage, TimestampedMessage } from '../../../core/types.js';
+import type { DataStore, MessageStore, RecordsWriteMessage } from '../../../index.js';
 
 import { Message } from '../../../core/message.js';
 import { RecordsWriteHandler } from './records-write.js';
@@ -11,11 +11,19 @@ import { RecordsWriteHandler } from './records-write.js';
  * NOTE: This is intended to be ONLY used by sync.
  */
 export class PrunedInitialRecordsWriteHandler extends RecordsWriteHandler {
+  /**
+   * Overriding parent behavior, `undefined` data stream is allowed.
+   */
+  protected validateUndefinedDataStream(
+    _dataStream: Readable | undefined,
+    _newestExistingMessage: TimestampedMessage | undefined,
+    _incomingMessage: RecordsWriteMessage): void { }
+
   /**
    * Stores the given message without storing the associated data.
    * Requires `dataCid` to exist in the `RecordsWrite` message given.
    */
-  public async storeMessage(
+  protected async storeMessage(
     messageStore: MessageStore,
     _dataStore: DataStore,
     eventLog: EventLog,

src/interfaces/records/handlers/records-write.ts

@@ -7,11 +7,11 @@ import type { DataStore, DidResolver, MessageStore } from '../../../index.js';
 
 import { authenticate } from '../../../core/auth.js';
 import { deleteAllOlderMessagesButKeepInitialWrite } from '../records-interface.js';
-import { DwnErrorCode } from '../../../core/dwn-error.js';
-import { DwnInterfaceName } from '../../../core/message.js';
 import { MessageReply } from '../../../core/message-reply.js';
 import { RecordsWrite } from '../messages/records-write.js';
 import { StorageController } from '../../../store/storage-controller.js';
+import { DwnError, DwnErrorCode } from '../../../core/dwn-error.js';
+import { DwnInterfaceName, Message } from '../../../core/message.js';
 
 export class RecordsWriteHandler implements MethodHandler {
 
@@ -56,62 +56,80 @@ export class RecordsWriteHandler implements MethodHandler {
       }
     }
 
-    // find which message is the newest, and if the incoming message is the newest
-    const newestExistingMessage = await RecordsWrite.getNewestMessage(existingMessages);
+    const newestExistingMessage = await Message.getNewestMessage(existingMessages);
 
     let incomingMessageIsNewest = false;
-    let newestMessage;
-    // if incoming message is newest
-    if (newestExistingMessage === undefined || await RecordsWrite.isNewer(message, newestExistingMessage)) {
+    let newestMessage; // keep reference of newest message for pruning later
+    if (newestExistingMessage === undefined || await Message.isNewer(message, newestExistingMessage)) {
       incomingMessageIsNewest = true;
       newestMessage = message;
     } else { // existing message is the same age or newer than the incoming message
       newestMessage = newestExistingMessage;
     }
 
-    // write the incoming message to DB if incoming message is newest
-    let messageReply: MessageReply;
-    if (incomingMessageIsNewest) {
-      const isLatestBaseState = true;
-      const indexes = await constructRecordsWriteIndexes(recordsWrite, isLatestBaseState);
+    if (!incomingMessageIsNewest) {
+      return new MessageReply({
+        status: { code: 409, detail: 'Conflict' }
+      });
+    }
 
-      try {
-        await this.storeMessage(this.messageStore, this.dataStore, this.eventLog, tenant, message, indexes, dataStream);
-      } catch (error) {
-        const e = error as any;
-        if (e.code === DwnErrorCode.StorageControllerDataCidMismatch ||
-            e.code === DwnErrorCode.StorageControllerDataNotFound ||
-            e.code === DwnErrorCode.StorageControllerDataSizeMismatch) {
-          return MessageReply.fromError(error, 400);
-        }
-
-        // else throw
-        throw error;
+    const isLatestBaseState = true;
+    const indexes = await constructRecordsWriteIndexes(recordsWrite, isLatestBaseState);
+
+    try {
+      this.validateUndefinedDataStream(dataStream, newestExistingMessage, message);
+
+      await this.storeMessage(this.messageStore, this.dataStore, this.eventLog, tenant, message, indexes, dataStream);
+    } catch (error) {
+      const e = error as any;
+      if (e.code === DwnErrorCode.StorageControllerDataCidMismatch ||
+          e.code === DwnErrorCode.StorageControllerDataSizeMismatch ||
+          e.code === DwnErrorCode.RecordsWriteMissingDataStream) {
+        return MessageReply.fromError(error, 400);
       }
 
-      messageReply = new MessageReply({
-        status: { code: 202, detail: 'Accepted' }
-      });
-    } else {
-      messageReply = new MessageReply({
-        status: { code: 409, detail: 'Conflict' }
-      });
+      // else throw
+      throw error;
     }
 
+    const messageReply = new MessageReply({
+      status: { code: 202, detail: 'Accepted' }
+    });
+
     // delete all existing messages that are not newest, except for the initial write
     await deleteAllOlderMessagesButKeepInitialWrite(tenant, existingMessages, newestMessage, this.messageStore, this.dataStore, this.eventLog);
 
     return messageReply;
   };
 
+  /**
+   * Further validation if data stream is undefined.
+   * NOTE: if data stream is not be provided but `dataCid` is provided,
+   * then we need to make sure that the existing record state is referencing the same data as the incoming message.
+   * Without this check will lead to unauthorized access of data (https://github.com/TBD54566975/dwn-sdk-js/issues/359)
+   */
+  protected validateUndefinedDataStream(
+    dataStream: _Readable.Readable | undefined,
+    newestExistingMessage: TimestampedMessage | undefined,
+    incomingMessage: RecordsWriteMessage): void {
+    if (dataStream === undefined && incomingMessage.descriptor.dataCid !== undefined) {
+      if (newestExistingMessage?.descriptor.dataCid !== incomingMessage.descriptor.dataCid) {
+        throw new DwnError(
+          DwnErrorCode.RecordsWriteMissingDataStream,
+          'Data stream is not provided.'
+        );
+      }
+    }
+  }
+
   /**
    * Stores the given message and its data in the underlying database(s).
    * NOTE: this method was created to allow a child class to override the default behavior for sync feature to work:
    * ie. allow `RecordsWrite` to be written even if data stream is not provided to handle the case that:
    * a `RecordsDelete` has happened, as a result a DWN would have pruned the data associated with the original write.
    * This approach avoids the need to duplicate the entire handler.
    */
-  public async storeMessage(
+  protected async storeMessage(
     messageStore: MessageStore,
     dataStore: DataStore,
     eventLog: EventLog,

src/store/storage-controller.ts

@@ -18,8 +18,6 @@ export class StorageController {
    * Puts the given message and data in storage.
    * @throws {DwnError} with `DwnErrorCode.StorageControllerDataCidMismatch`
    *                    if the data stream resulted in a data CID that mismatches with `dataCid` in the given message
-   * @throws {DwnError} with `DwnErrorCode.StorageControllerDataNotFound`
-   *                    if `dataCid` in `descriptor` is given, and `dataStream` is not given, and data for the message does not exist already
    * @throws {DwnError} with `DwnErrorCode.StorageControllerDataSizeMismatch`
    *                    if `dataSize` in `descriptor` given mismatches the actual data size
    */
@@ -35,26 +33,18 @@ export class StorageController {
     const messageCid = await Message.getCid(message);
 
     // if `dataCid` is given, it means there is corresponding data associated with this message
-    // but NOTE: it is possible that a data stream is not given in such case, for instance,
-    // a subsequent RecordsWrite that changes the `published` property, but the data hasn't changed,
-    // in this case requiring re-uploading of the data is extremely inefficient so we take care allow omission of data stream
     if (message.descriptor.dataCid !== undefined) {
       let result;
-
       if (dataStream === undefined) {
+        // but NOTE: it is possible that a data stream is not given even when `dataCid` is given, for instance,
+        // a subsequent RecordsWrite that changes the `published` property, but the data hasn't changed,
+        // in this case requiring re-uploading of the data is extremely inefficient so we allow omission of data stream as a utility function,
+        // but this method assumes checks for the appropriate conditions already took place prior to calling this method.
         result = await dataStore.associate(tenant, messageCid, message.descriptor.dataCid);
       } else {
         result = await dataStore.put(tenant, messageCid, message.descriptor.dataCid, dataStream);
       }
 
-      // the message implies that the data is already in the DB, so we check to make sure the data already exist
-      if (!result) {
-        throw new DwnError(
-          DwnErrorCode.StorageControllerDataNotFound,
-          `data with dataCid ${message.descriptor.dataCid} not found in store`
-        );
-      }
-
       // MUST verify that the size of the actual data matches with the given `dataSize`
       // if data size is wrong, delete the data we just stored
       if (message.descriptor.dataSize !== result.dataSize) {

src/utils/array.ts

@@ -1,11 +1,23 @@
-
 /**
- * Asynchronously iterates an {AsyncGenerator} to return all the values in an array.
+ * Array utility methods.
  */
-export async function asyncGeneratorToArray<T>(iterator: AsyncGenerator<T>): Promise<Array<T>> {
-  const array: Array<T> = [ ];
-  for await (const value of iterator) {
-    array.push(value);
+export class ArrayUtility {
+  /**
+   * Returns `true` if content of the two given byte arrays are equal; `false` otherwise.
+   */
+  public static byteArraysEqual(array1: Uint8Array, array2:Uint8Array): boolean {
+    const equal = array1.length === array2.length && array1.every((value, index) => value === array2[index]);
+    return equal;
+  }
+
+  /**
+   * Asynchronously iterates an {AsyncGenerator} to return all the values in an array.
+   */
+  public static async fromAsyncGenerator<T>(iterator: AsyncGenerator<T>): Promise<Array<T>> {
+    const array: Array<T> = [ ];
+    for await (const value of iterator) {
+      array.push(value);
+    }
+    return array;
   }
-  return array;
-}
+}