Skip to content

Commit

Permalink
Fix Node encryption of ArrayBuffer plaintext
Browse files Browse the repository at this point in the history
ArrayBuffer does not have a property named `length`, and also
Buffer.concat does not accept an argument of type ArrayBuffer.

The createTestMessage stuff introduced in the tests is a result of
wanting to be able to perform multiple tests relating to the same
original data without one test affecting another, given two things:

1. I want to be able to explicitly manipulate the message inside my test
   case (e.g. to change its `data` property to an ArrayBuffer).

2. Message.encode appears to have the side effect of manipulating the
   `data` property of the message that it is passed. This doesn’t feel
   intentional (and indeed is quite surprising), but I haven’t
   investigated it here.

Resolves #1281.
  • Loading branch information
lawrence-forooghian committed May 18, 2023
1 parent 54a9fc5 commit 5592d04
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 18 deletions.
7 changes: 5 additions & 2 deletions src/platform/nodejs/lib/util/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,13 @@ var Crypto = (function () {

CBCCipher.prototype.encrypt = function (plaintext, callback) {
Logger.logAction(Logger.LOG_MICRO, 'CBCCipher.encrypt()', '');
var plaintextLength = plaintext.length,
var plaintextBuffer = Platform.BufferUtils.toBuffer(plaintext);
var plaintextLength = plaintextBuffer.length,
paddedLength = getPaddedLength(plaintextLength),
iv = this.getIv();
var cipherOut = this.encryptCipher.update(Buffer.concat([plaintext, pkcs5Padding[paddedLength - plaintextLength]]));
var cipherOut = this.encryptCipher.update(
Buffer.concat([plaintextBuffer, pkcs5Padding[paddedLength - plaintextLength]])
);
var ciphertext = Buffer.concat([iv, toBuffer(cipherOut)]);
return callback(null, ciphertext);
};
Expand Down
71 changes: 55 additions & 16 deletions test/realtime/crypto.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,13 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
var item = testData.items[i];

/* read messages from test data and decode (ie remove any base64 encoding). */
var testMessage = Message.fromEncoded(item.encoded);
var createTestMessage = function () {
return Message.fromEncoded(item.encoded);
};
var encryptedMessage = Message.fromEncoded(item.encrypted);
/* reset channel cipher, to ensure it uses the given iv */
channel.setOptions({ cipher: { key: key, iv: iv } });
fixtureTest(channel.channelOptions, testMessage, encryptedMessage, item.msgpack);
fixtureTest(channel.channelOptions, createTestMessage, encryptedMessage, item.msgpack);
}
} catch (err) {
closeAndFinish(done, realtime, err);
Expand All @@ -85,6 +87,33 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
});
}

/**
* Helper for testing that we are able to encrypt data of various buffer-like types.
*
* If the message created by `createTestMessage` has data of buffer-like type, then this will call `fixtureTest`
* with a function that creates that message, and if the data is not already an `ArrayBuffer` then it will also call
* `fixtureTest` with a function that creates a version of that message whose data is instead represented as an
* `ArrayBuffer`.
*
* If the message does not have data of buffer-like type, it will simply call `fixtureTest` with `createTestMessage`.
*/
function testEachPlaintextVariant(createTestMessage, fixtureTest) {
// Test with plaintext of type same as that of createTestMessage().data
fixtureTest(createTestMessage);

// If createTestMessage().data is buffer-like but not an ArrayBuffer, convert plaintext to ArrayBuffer and test
var testMessage = createTestMessage();
if (BufferUtils.isBuffer(testMessage.data) && !(testMessage.data instanceof ArrayBuffer)) {
var createTestMessageWithArrayBufferData = function () {
var testMessageWithArrayBufferData = createTestMessage();
testMessageWithArrayBufferData.data = BufferUtils.toArrayBuffer(testMessageWithArrayBufferData.data);
return testMessageWithArrayBufferData;
};

fixtureTest(createTestMessageWithArrayBufferData);
}
}

describe('realtime/crypto', function () {
this.timeout(60 * 1000);

Expand Down Expand Up @@ -204,11 +233,14 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
'crypto-data-128.json',
'encrypt_message_128',
2,
function (channelOpts, testMessage, encryptedMessage) {
/* encrypt plaintext message; encode() also to handle data that is not already string or buffer */
Message.encode(testMessage, channelOpts, function () {
/* compare */
testMessageEquality(done, testMessage, encryptedMessage);
function (channelOpts, createTestMessage, encryptedMessage) {
testEachPlaintextVariant(createTestMessage, function (createTestMessage) {
var testMessage = createTestMessage();
/* encrypt plaintext message; encode() also to handle data that is not already string or buffer */
Message.encode(testMessage, channelOpts, function () {
/* compare */
testMessageEquality(done, testMessage, encryptedMessage);
});
});
}
);
Expand All @@ -220,11 +252,14 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
'crypto-data-256.json',
'encrypt_message_256',
2,
function (channelOpts, testMessage, encryptedMessage) {
/* encrypt plaintext message; encode() also to handle data that is not already string or buffer */
Message.encode(testMessage, channelOpts, function () {
/* compare */
testMessageEquality(done, testMessage, encryptedMessage);
function (channelOpts, createTestMessage, encryptedMessage) {
testEachPlaintextVariant(createTestMessage, function (createTestMessage) {
var testMessage = createTestMessage();
/* encrypt plaintext message; encode() also to handle data that is not already string or buffer */
Message.encode(testMessage, channelOpts, function () {
/* compare */
testMessageEquality(done, testMessage, encryptedMessage);
});
});
}
);
Expand All @@ -236,7 +271,8 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
'crypto-data-128.json',
'decrypt_message_128',
2,
function (channelOpts, testMessage, encryptedMessage) {
function (channelOpts, createTestMessage, encryptedMessage) {
var testMessage = createTestMessage();
/* decrypt encrypted message; decode() also to handle data that is not string or buffer */
Message.decode(encryptedMessage, channelOpts);
/* compare */
Expand All @@ -251,7 +287,8 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
'crypto-data-256.json',
'decrypt_message_256',
2,
function (channelOpts, testMessage, encryptedMessage) {
function (channelOpts, createTestMessage, encryptedMessage) {
var testMessage = createTestMessage();
/* decrypt encrypted message; decode() also to handle data that is not string or buffer */
Message.decode(encryptedMessage, channelOpts);
/* compare */
Expand Down Expand Up @@ -292,7 +329,8 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
'crypto-data-128.json',
'msgpack_128',
2,
function (channelOpts, testMessage, encryptedMessage, msgpackEncodedMessage) {
function (channelOpts, createTestMessage, encryptedMessage, msgpackEncodedMessage) {
var testMessage = createTestMessage();
Message.encode(testMessage, channelOpts, function () {
var msgpackFromEncoded = msgpack.encode(testMessage);
var msgpackFromEncrypted = msgpack.encode(encryptedMessage);
Expand Down Expand Up @@ -325,7 +363,8 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
'crypto-data-256.json',
'msgpack_256',
2,
function (channelOpts, testMessage, encryptedMessage, msgpackEncodedMessage) {
function (channelOpts, createTestMessage, encryptedMessage, msgpackEncodedMessage) {
var testMessage = createTestMessage();
Message.encode(testMessage, channelOpts, function () {
var msgpackFromEncoded = msgpack.encode(testMessage);
var msgpackFromEncrypted = msgpack.encode(encryptedMessage);
Expand Down

0 comments on commit 5592d04

Please sign in to comment.