diff --git a/CHANGELOG.md b/CHANGELOG.md index 77f326f..01f74d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,9 @@ - An `alert_notification` or an `outbind` from the peer is logged and left unanswered, as SMPP 3.4 gives neither a response. Each one used to emit `sessionError`, `"alert_notification" has no response command`. +- `maxOctets` charges every TLV a held segment carries for the memory it keeps, empty ones + included. A peer could hold megabytes per segment beyond the cap by sending thousands of empty + TLVs, which it counted as nothing. - `server()` refuses a `maxOctets` below 1 or not a whole number, `Infinity` included, like its other limits. `server({ maxOctets: 0 })` used to start and then refuse every multipart message. - `callback_num`, `callback_num_atag`, `callback_num_pres_ind`, `broadcast_area_identifier` and diff --git a/src/reassembly.ts b/src/reassembly.ts index a40eccc..00d0409 100644 --- a/src/reassembly.ts +++ b/src/reassembly.ts @@ -73,8 +73,8 @@ function detach(pduObj: PduObject): PduObject { return { ...pduObj, params, shortMessageOctets: octets, tlvs }; } -// Roughly the heap a listed occurrence costs beyond its value, so a PDU of empty repeats is not free. -const listedTlvOverhead = 200; +// Roughly the heap a TLV or listed occurrence costs beyond its value, so a PDU of empty ones is not free. +const tlvObjectOverhead = 200; // A cstring param arrives as a string, and source_addr alone can carry most of a 1 MiB PDU. function sizeOf(value: ParamValue): number { @@ -93,7 +93,7 @@ function octetsOf(pduObj: PduObject): number { for (const tlv of Object.values(pduObj.tlvs)) { const listed = Array.isArray(tlv.tagValue) ? tlv.tagValue.length : 0; - octets += tlvOctets(tlv.tagValue) + listed * listedTlvOverhead; + octets += tlvOctets(tlv.tagValue) + (1 + listed) * tlvObjectOverhead; } return octets; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 3456adf..4add97b 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1805,7 +1805,7 @@ describe('reassembly bounds', () => { assert.deepEqual(lost, [], 'the peer holds the only segment there was, so nothing was lost'); }); - // The two addresses are 22 octets, so only the 14 the TLV carries can overrun a cap of 30. + // The two addresses and the TLV's object are 222 octets, so only the 14 it carries can overrun a cap of 230. test('counts a body carried in message_payload against the octet cap', () => { function collectPayload(maxOctets: number): Collected { const reassembler = new Reassembler({ @@ -1820,12 +1820,12 @@ describe('reassembly bounds', () => { return collectPdu(reassembler, payloadSegment(9, 1, 2)); } - assert.equal(collectPayload(30).kept, false, 'a TLV body the cap cannot hold is refused, not dropped later'); - assert.equal(collectPayload(40).kept, true); + assert.equal(collectPayload(230).kept, false, 'a TLV body the cap cannot hold is refused, not dropped later'); + assert.equal(collectPayload(240).kept, true); }); - test('counts every occurrence of a repeatable TLV against the octet cap, empty ones included', () => { - function collectCallbacks(maxOctets: number, tagValue: Buffer[]): Collected { + test('counts every TLV and every occurrence of a repeatable one against the octet cap, empty ones included', () => { + function collectTlvs(maxOctets: number, tlvs: PduObject['tlvs']): Collected { const reassembler = new Reassembler({ log: silentLog, max: 10, @@ -1834,11 +1834,17 @@ describe('reassembly bounds', () => { onLost: () => undefined, timeout: 60_000, }); - const carried = segment(9, 1, 2); - return collectPdu(reassembler, { ...carried, tlvs: { callback_num: { tagId: 0x0381, tagName: 'callback_num', tagValue } } }); + return collectPdu(reassembler, { ...segment(9, 1, 2), tlvs }); } + function collectCallbacks(maxOctets: number, tagValue: Buffer[]): Collected { + return collectTlvs(maxOctets, { callback_num: { tagId: 0x0381, tagName: 'callback_num', tagValue } }); + } + const unknownTags = Object.fromEntries(Array.from({ length: 10_000 }, (_, i) => { + const tagId = 0x4000 + i; + return [String(tagId), { tagId, tagName: undefined, tagValue: Buffer.alloc(0) }]; + })); const numbers = [Buffer.alloc(10_000, 0x31), Buffer.alloc(10_000, 0x32)]; assert.equal(collectCallbacks(20_000, numbers).kept, false); @@ -1848,6 +1854,7 @@ describe('reassembly bounds', () => { false, 'an empty occurrence still holds an object', ); + assert.equal(collectTlvs(30_000, unknownTags).kept, false, 'an empty tag still holds an object'); }); // The segments before it were answered ESME_ROK, so dropping those is not the same as refusing one. @@ -1921,9 +1928,9 @@ describe('reassembly bounds', () => { assert.equal(counted.size, 1, 'the second group evicted the first, as a UDH group would'); counted.clear(); - // A sar_* segment is the two 11-octet addresses plus an 8-octet body, with no UDH to carry. - assert.equal(collectSar(capped(20), 3, 1, 2).kept, false); - assert.equal(collectSar(capped(30), 3, 1, 2).kept, true); + // A sar_* segment is the two 11-octet addresses, an 8-octet body and three 200-octet TLV objects. + assert.equal(collectSar(capped(620), 3, 1, 2).kept, false); + assert.equal(collectSar(capped(630), 3, 1, 2).kept, true); }); // Nothing else says a message the peer has already been answered for was thrown away. diff --git a/todo.md b/todo.md index 7b83a5f..843a501 100644 --- a/todo.md +++ b/todo.md @@ -6,14 +6,6 @@ hard rules first — they constrain every item below. This is a working file that sets its own rules. The documentation conventions in AGENTS.md do not govern it, and nothing here is a source anything else may cite. -## Security - -- [ ] **Charge a held segment's TLVs for the objects they keep, not only their value octets.** A - peer sending segments that carry thousands of distinct unknown tags with empty values makes - this library hold megabytes of heap per segment that `maxOctets` counts as nothing, up to - 255 segments per group. Repeatable tags are already charged per occurrence. From the stability - review of #25. - ## Status The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18