From d88422fc54ce53d4209b6e9a9b5dc3e9cf84bf24 Mon Sep 17 00:00:00 2001 From: gvergnaud Date: Thu, 9 Jan 2025 15:40:05 -0500 Subject: [PATCH] security: add update size limits to prevent DOS attacks --- src/utils/encoding.js | 5 +++ src/utils/limits.js | 48 ++++++++++++++++++++++ src/utils/updates.js | 5 +++ tests/index.js | 3 +- tests/limits.tests.js | 93 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 src/utils/limits.js create mode 100644 tests/limits.tests.js diff --git a/src/utils/encoding.js b/src/utils/encoding.js index b195ccc3..4e59002e 100644 --- a/src/utils/encoding.js +++ b/src/utils/encoding.js @@ -45,6 +45,7 @@ import * as binary from 'lib0/binary' import * as map from 'lib0/map' import * as math from 'lib0/math' import * as array from 'lib0/array' +import { assertMaxGCLength, assertMaxSkipLength, assertMaxStructs, assertMaxUpdates } from './limits.js' /** * @param {UpdateEncoderV1 | UpdateEncoderV2} encoder @@ -115,8 +116,10 @@ export const readClientsStructRefs = (decoder, doc) => { */ const clientRefs = map.create() const numOfStateUpdates = decoding.readVarUint(decoder.restDecoder) + assertMaxUpdates(numOfStateUpdates) for (let i = 0; i < numOfStateUpdates; i++) { const numberOfStructs = decoding.readVarUint(decoder.restDecoder) + assertMaxStructs(numberOfStructs) /** * @type {Array} */ @@ -130,6 +133,7 @@ export const readClientsStructRefs = (decoder, doc) => { switch (binary.BITS5 & info) { case 0: { // GC const len = decoder.readLen() + assertMaxGCLength(len) refs[i] = new GC(createID(client, clock), len) clock += len break @@ -137,6 +141,7 @@ export const readClientsStructRefs = (decoder, doc) => { case 10: { // Skip Struct (nothing to apply) // @todo we could reduce the amount of checks by adding Skip struct to clientRefs so we know that something is missing. const len = decoding.readVarUint(decoder.restDecoder) + assertMaxSkipLength(len) refs[i] = new Skip(createID(client, clock), len) clock += len break diff --git a/src/utils/limits.js b/src/utils/limits.js new file mode 100644 index 00000000..877d873d --- /dev/null +++ b/src/utils/limits.js @@ -0,0 +1,48 @@ +export const MAX_STRUCTS = 100_000 +export const MAX_UPDATES = 100_000 +export const MAX_GC_LENGTH = 100_000 +export const MAX_SKIP_LENGTH = 100_000 + +/** + * @param {number} numOfStateUpdates + */ +export function assertMaxUpdates (numOfStateUpdates) { + if (numOfStateUpdates > MAX_UPDATES) { + throw new Error( + `This update exceeds the maximum number of updates. ${numOfStateUpdates} > ${MAX_UPDATES}` + ) + } +} + +/** + * @param {number} numberOfStructs + */ +export function assertMaxStructs (numberOfStructs) { + if (numberOfStructs > MAX_STRUCTS) { + throw new Error( + `This update exceeds the maximum number of structs. ${numberOfStructs} > ${MAX_STRUCTS}` + ) + } +} + +/** + * @param {number} len + */ +export function assertMaxSkipLength (len) { + if (len > MAX_SKIP_LENGTH) { + throw new Error( + `This skip length exceeds the limit. ${len} > ${MAX_SKIP_LENGTH}` + ) + } +} + +/** + * @param {number} len + */ +export function assertMaxGCLength (len) { + if (len > MAX_GC_LENGTH) { + throw new Error( + `This garbage collection update's length exceeds the limit. ${len} > ${MAX_GC_LENGTH}` + ) + } +} diff --git a/src/utils/updates.js b/src/utils/updates.js index fc40cd57..f93752b2 100644 --- a/src/utils/updates.js +++ b/src/utils/updates.js @@ -36,21 +36,25 @@ import { YXmlElement, YXmlHook } from '../internals.js' +import { assertMaxGCLength, assertMaxSkipLength, assertMaxStructs, assertMaxUpdates } from './limits.js' /** * @param {UpdateDecoderV1 | UpdateDecoderV2} decoder */ function * lazyStructReaderGenerator (decoder) { const numOfStateUpdates = decoding.readVarUint(decoder.restDecoder) + assertMaxUpdates(numOfStateUpdates) for (let i = 0; i < numOfStateUpdates; i++) { const numberOfStructs = decoding.readVarUint(decoder.restDecoder) const client = decoder.readClient() let clock = decoding.readVarUint(decoder.restDecoder) + assertMaxStructs(numberOfStructs) for (let i = 0; i < numberOfStructs; i++) { const info = decoder.readInfo() // @todo use switch instead of ifs if (info === 10) { const len = decoding.readVarUint(decoder.restDecoder) + assertMaxSkipLength(len) yield new Skip(createID(client, clock), len) clock += len } else if ((binary.BITS5 & info) !== 0) { @@ -74,6 +78,7 @@ function * lazyStructReaderGenerator (decoder) { clock += struct.length } else { const len = decoder.readLen() + assertMaxGCLength(len) yield new GC(createID(client, clock), len) clock += len } diff --git a/tests/index.js b/tests/index.js index fd9c08e7..8c6d820c 100644 --- a/tests/index.js +++ b/tests/index.js @@ -11,6 +11,7 @@ import * as doc from './doc.tests.js' import * as snapshot from './snapshot.tests.js' import * as updates from './updates.tests.js' import * as relativePositions from './relativePositions.tests.js' +import * as limits from './limits.tests.js' import { runTests } from 'lib0/testing' import { isBrowser, isNode } from 'lib0/environment' @@ -25,7 +26,7 @@ if (isBrowser) { * @type {any} */ const tests = { - doc, map, array, text, xml, encoding, undoredo, compatibility, snapshot, updates, relativePositions + doc, map, array, text, xml, encoding, undoredo, compatibility, snapshot, updates, relativePositions, limits } const run = async () => { diff --git a/tests/limits.tests.js b/tests/limits.tests.js new file mode 100644 index 00000000..5f1d60aa --- /dev/null +++ b/tests/limits.tests.js @@ -0,0 +1,93 @@ +import * as t from 'lib0/testing' +import * as Y from '../src/index.js' + +/** + * @param {() => void} f function that should throw + * @returns {boolean} + */ +const shouldThrow = (f) => { + try { + f() + return false + } catch (/** @type {any} */ e) { + console.log('Error thrown:', e?.message) + return true + } +} + +/** + * @param {t.TestCase} _tc + */ +export const testShouldntMergeUpdatesWithTooManyStructs = (_tc) => { + // Binary with 4398046511101 structs. + const buf = new Uint8Array([ + 0, 1, 0, 0, 0, 0, 1, 0, 1, 0, 1, 2, 0, 0, 22, 2, 0, 0, 1, 253, 255, 255, + 255, 255, 127, 0, 0 + ]) + + t.assert(shouldThrow(() => { + const update = Y.encodeStateAsUpdateV2(new Y.Doc()) + Y.mergeUpdatesV2([update, buf]) + })) +} + +/** + * @param {t.TestCase} _tc + */ +export const testShouldntMergeUpdatesWithTooManyUpdatesV1 = (_tc) => { + // Binary with 265828 updates. + const buf = new Uint8Array([ + 228, 156, 16, 0, 5, 255, 255, 5, 0, 1, 0, 0, 0, 1, 1, 0, 0, 0, 237, + 0, 0, 0, 1, 1, 0, 254, 184, 194, 233, 173, 135, 217, 18, 0, 0, 1, 1, + 255, 237, 246 + ]) + const update = Y.encodeStateAsUpdate(new Y.Doc()) + t.assert(shouldThrow(() => { + Y.mergeUpdates([update, buf]) + })) +} + +/** + * @param {t.TestCase} _tc + */ +export const testShouldntMergeUpdatesWithTooManyUpdatesV2 = (_tc) => { + // Binary with 7658324286 updates. + const buf = new Uint8Array([ + 228, 149, 0, 0, 1, 24, 0, 0, 1, 0, 1, 0, 0, 1, 0, 0, 237, 1, 190, + 130, 227, 195, 28, 1, 2, 228, 149, 0, 0, 1, 24, 0, 0, 1, 24, 0, 0, + 1, 0, 1, 237, 0 + ]) + const update = Y.encodeStateAsUpdateV2(new Y.Doc()) + + t.assert(shouldThrow(() => { + Y.mergeUpdatesV2([update, buf]) + })) +} + +/** + * @param {t.TestCase} _tc + */ +export const testShouldntAcceptTooLargeGCItems = (_tc) => { + // Update with a GC item of length 553253. + const buf = new Uint8Array([ + 143, 1, 128, 0, 0, 0, 1, 170, 1, 0, 1, 2, 0, 0, 22, 2, 0, 229, 196, + 67, 20, 231, 166, 139, 147, 174, 181, 253, 93, 232, 38, 154, 138, + 89, 0, 49, 213, 15, 18, 1, 48, 0, 0, 0 + ]) + const update = Y.encodeStateAsUpdateV2(new Y.Doc()) + t.assert(shouldThrow(() => { + Y.mergeUpdatesV2([update, buf]) + })) +} + +export const testShouldntApplyUpdatesOverLimit = () => { + // Binary with 267854847 structs in one update. + const buf = new Uint8Array([ + 0, 1, 35, 0, 0, 0, 1, 2, 129, 0, 0, 16, 0, 199, 220, 0, 196, 122, 128, + 0, 65, 171, 234, 214, 0, 1, 0, 0, 1, 0, 0, 132, 0, 0, 16, 255, 199, 220, + 255, 0, 0, 0 + ]) + t.assert(shouldThrow(() => { + Y.applyUpdateV2(new Y.Doc(), buf) + })) +}