diff --git a/AGENTS.md b/AGENTS.md index 0b7ae06..a3af124 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -401,6 +401,24 @@ Grouped by what each one constrains. keeps that traffic and `PendingRequests.nextSeqNr()`, the only thing that invents one, is what holds our own sends inside the spec. +- **The optional parameters run to `command_length` exactly, and the only slack tolerated is one + NULL octet where a peer padded `short_message`.** Maintainer's call, 2026-09-06, from the + Java-client interoperability phase: accepting any parse that merely did not error answered + `ESME_ROK` to a `deliver_sm` whose three trailing octets were never read, dropping the + `receipted_message_id` that makes a receipt a receipt + ([interop-tests/findings/05-java-clients.md](interop-tests/findings/05-java-clients.md)). Goal 2 + settles it against goal 3: octets this codec cannot name are a PDU it did not read, so a region + that does not end on `command_length` — the padded read included — is refused with the `tlvs` + reason and `ESME_RINVTLVSTREAM` a truncated TLV value already gets. What the rule costs is paid + once, in `readCstring()`: a trailing C-Octet String a peer left out entirely consumes no octet, + where reporting the terminator it never sent puts every later offset past the declared end and + refuses a bind, and every bodyless response, that used to parse. That composes, so a run of them + at the tail all read empty — `outbind` is the only command with two, and an absent field and an + empty one say the same thing, so goal 2 is not at stake even there. Rejected: keeping the tolerance + for the one to three trailing octets too few to hold a TLV header, which no researched peer sends + and which cannot be told apart from the truncated tail this fixes. Rejected: refusing it as + `body`/`ESME_RINVCMDLEN`, which names the mandatory fields — the part the peer got right. + - **`smsIdFormat` names a notation per place, and normalisation never reaches inside a `-` id.** An SMSC may answer `submit_sm_resp` in hex and write the receipt's `id:` in decimal, so one transform over both sides cannot make them equal. `submitResp` covers the `receipted_message_id` diff --git a/interop-tests/findings/05-java-clients.md b/interop-tests/findings/05-java-clients.md index be2dd59..32d6ecd 100644 --- a/interop-tests/findings/05-java-clients.md +++ b/interop-tests/findings/05-java-clients.md @@ -136,19 +136,25 @@ plain Python socket, no Java involved): ``` This is a `deliver_sm` (`source_addr` `raw-from`, `destination_addr` `raw-to`, body "truncated tlv -probe silent") followed by `00 1d 00 c8` - tag `0x001D`, declared length 200, zero value octets. +probe") followed by `00 1d 00 c8` - tag `0x001D`, declared length 200, zero value octets. Our server answers `command_status 0x00000000` (`ESME_ROK`) and delivers the text as `sms`. Appending 4 more arbitrary octets to the same tail (8 total, still declaring length 200) correctly triggers `ESME_RINVTLVSTREAM` instead - `jsmpp.test.ts`'s "a deliver_sm with a truncated TLV stream" test uses that 8-octet form deliberately, to test the *documented* refusal path rather than this adjacent bug. Reproduced with jsmpp's own driver too: -`jsmpp.test.ts`, "defect: a 4-octet truncated TLV tail is silently accepted rather than refused" - -same bytes, over a `net.Socket` opened directly against `server()` (not through jsmpp's typed API, -which cannot build this shape at all). Severity: low - a narrow boundary condition (exactly 4 +`jsmpp.test.ts`, "a deliver_sm ending in a bare TLV header gets ESME_RINVTLVSTREAM, and reaches no +listener" - the same shape, over a `net.Socket` opened directly against `server()` (not through +jsmpp's typed API, which cannot build it at all). Severity: low - a narrow boundary condition (exactly 4 trailing octets, no value) rather than a general TLV-validation gap, but it is a hole in the fix target 1 otherwise closed, silently dropping a TLV the peer meant to send instead of losing (and counting) the one malformed PDU. +Fixed in [#87](https://github.com/larvit/larvitsmpp/pull/87): the optional parameters now have to end +on `command_length`, so this PDU is refused `ESME_RINVTLVSTREAM` like the sibling case. The hex above +is asserted octet for octet by `test/pdu.test.ts`, "refuses a bare TLV header the same way it refuses +a truncated value"; the same shape over a socket is `jsmpp.test.ts`, "a deliver_sm ending in a bare +TLV header gets ESME_RINVTLVSTREAM, and reaches no listener". + ## Peer quirks - **jsmpp's `session.getInterfaceVersion()` echoes what the driver declared, not what the earlier diff --git a/interop-tests/jsmpp.test.ts b/interop-tests/jsmpp.test.ts index 063a025..1fdea20 100644 --- a/interop-tests/jsmpp.test.ts +++ b/interop-tests/jsmpp.test.ts @@ -4,6 +4,7 @@ import test, { after, describe } from 'node:test'; import type { Session } from '../src/session.ts'; import type { Sms } from '../src/sms.ts'; import { PduRefusedError } from '../src/index.ts'; +import { bareTlvHeader, pduBytes } from '../test/raw-pdus.ts'; import { server } from '../src/server.ts'; const JSMPP_HOST = process.env.JSMPP_HOST ?? 'jsmpp:8080'; @@ -264,62 +265,48 @@ describe('S3 - known-but-unhandled and malformed commands (targets 1, 6)', () => assert.equal(linkResponse.cmdStatusHex, '0x0'); }); - // Not reachable through jsmpp's own typed API at all (it cannot construct wire garbage) - this is - // a raw fixture opened directly against our server(), discovered while building the reproducers - // above. Kept here rather than in test/ because it was found during, and belongs beside, this - // phase's malformed-PDU work. - test('defect: a 4-octet truncated TLV tail is silently accepted rather than refused', async () => { + // Not reachable through jsmpp's own typed API at all (it cannot construct wire garbage), so this + // is a raw fixture opened directly against our server(). + test('a deliver_sm ending in a bare TLV header gets ESME_RINVTLVSTREAM, and reaches no listener', async t => { await waitForSessions(1); - function cstring(value: string): Buffer { - return Buffer.concat([Buffer.from(value, 'latin1'), Buffer.from([0])]); - } - - const msg = Buffer.from('truncated tlv probe silent', 'latin1'); - const body = Buffer.concat([ - cstring(''), Buffer.from([0, 0]), cstring('raw2-from'), - Buffer.from([0, 0]), cstring('raw2-to'), - Buffer.from([0, 0, 0]), cstring(''), cstring(''), - Buffer.from([0, 0, 0, 0, msg.length]), msg, - // A bare 4-octet TLV header (tag 0x001D, declared length 200) with zero value octets. - Buffer.from([0x00, 0x1D, 0x00, 0xC8]), - ]); - const header = Buffer.alloc(16); - - header.writeUInt32BE(16 + body.length, 0); - header.writeUInt32BE(0x00000005, 4); - header.writeUInt32BE(0, 8); - header.writeUInt32BE(777, 12); - const sock = net.connect(SMPP_PORT, '127.0.0.1'); + t.after(() => { sock.destroy(); }); await new Promise(resolve => { sock.once('connect', () => { resolve(); }); }); - const bindBody = Buffer.concat([cstring('rawverify'), cstring('pw'), cstring(''), Buffer.from([0x34, 0, 0]), cstring('')]); - const bindHeader = Buffer.alloc(16); - - bindHeader.writeUInt32BE(16 + bindBody.length, 0); - bindHeader.writeUInt32BE(0x00000009, 4); - bindHeader.writeUInt32BE(0, 8); - bindHeader.writeUInt32BE(1, 12); - sock.write(Buffer.concat([bindHeader, bindBody])); + sock.write(pduBytes({ + cmdName: 'bind_transceiver', + params: { interface_version: 0x34, password: 'pw', system_id: 'rawverify' }, + seqNr: 1, + })); await new Promise(resolve => { sock.once('data', () => { resolve(); }); }); const responsePromise = new Promise(resolve => { sock.once('data', data => { resolve(data); }); }); - sock.write(Buffer.concat([header, body])); + sock.write(bareTlvHeader({ + cmdName: 'deliver_sm', + params: { + destination_addr: 'raw2-to', + short_message: 'truncated tlv probe silent', + source_addr: 'raw2-from', + }, + seqNr: 777, + })); const response = await responsePromise; - const cmdStatus = response.readUInt32BE(8); - // Defect: this should be ESME_RINVTLVSTREAM (0xC0); the codec's trailing-NUL retry instead - // treats the 4 leftover octets as unparsed slack and accepts the PDU as ESME_ROK. - assert.equal(cmdStatus, 0x00000000); + assert.equal(response.readUInt32BE(4), 0x80000005); + assert.equal(response.readUInt32BE(8), 0x000000C0); + assert.equal(response.readUInt32BE(12), 777); - const arrived = await waitForSms('truncated tlv probe silent'); + const refused = await waitFor(() => allSessionErrors.find(e => e.err instanceof PduRefusedError + && e.err.header.seqNr === 777)); - assert.equal(arrived.from, 'raw2-from'); - sock.destroy(); + assert.ok(refused); + assert.ok(refused.err instanceof PduRefusedError); + assert.equal(refused.err.reason, 'tlvs'); + assert.equal(allSms.some(entry => entry.sms.message === 'truncated tlv probe silent'), false); }); }); diff --git a/src/defs/commands.ts b/src/defs/commands.ts index 2850dc7..900a789 100644 --- a/src/defs/commands.ts +++ b/src/defs/commands.ts @@ -19,7 +19,8 @@ const bindParams = { /** * Key order inside each `params` object is the order the fields appear on the wire. Reordering - * them corrupts every PDU of that command. + * them corrupts every PDU of that command, and moving a field after `short_message` also stops the + * codec skipping the NULL octet some peers append to it. */ const specs = { alert_notification: { diff --git a/src/defs/types.ts b/src/defs/types.ts index 6959e67..9f8a323 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -146,7 +146,6 @@ function wantUnsuccessSmes(value: ParamValue): Result<{ smes: UnsuccessSme[] }> } function readCstring(buffer: Buffer, offset: number): Result<{ bytesRead: number; value: string }> { - // An offset at the end exactly is an absent trailing field, which real peers do send. if (outOfRange(buffer, offset, 0)) { return { err: new Error( @@ -155,6 +154,9 @@ function readCstring(buffer: Buffer, offset: number): Result<{ bytesRead: number }; } + // An offset at the end exactly is an absent trailing field, which real peers do send. + if (offset === buffer.length) return { bytesRead: 0, value: '' }; + let length = 0; while (buffer[offset + length]) { diff --git a/src/pdu.ts b/src/pdu.ts index a23454b..842c35c 100644 --- a/src/pdu.ts +++ b/src/pdu.ts @@ -245,19 +245,15 @@ export function objToPdu(obj: PduObjectInput): Result< ); } -function parseTlvs( - pdu: Buffer, - start: number, - cmdLength: number, -): Result<{ offset: number; tlvs: Record }> { +function parseTlvs(pdu: Buffer, start: number): Result<{ offset: number; tlvs: Record }> { const tlvs: Record = {}; let offset = start; - while (offset + 4 <= cmdLength) { + while (offset + 4 <= pdu.length) { const tagId = pdu.readUInt16BE(offset); const tagLength = pdu.readUInt16BE(offset + 2); - if (offset + 4 + tagLength > cmdLength) { + if (offset + 4 + tagLength > pdu.length) { return { err: new Error(`TLV ${String(tagId)} runs past the end of the PDU`) }; } @@ -281,9 +277,9 @@ function parseTlvs( function readParams( cmdName: CommandName, pdu: Buffer, - trailingNull: boolean, -): Result<{ offset: number; params: Record }> { +): Result<{ lastParam: string | undefined; offset: number; params: Record }> { const params: Record = {}; + let lastParam: string | undefined; let offset = 16; for (const [name, type] of Object.entries(cmds[cmdName]?.params ?? {})) { @@ -293,13 +289,39 @@ function readParams( return { err: new Error(`Parameter "${name}" of "${cmdName}": ${read.err.message}`) }; } + lastParam = name; params[name] = read.value; offset += read.bytesRead; - - if (name === 'short_message' && trailingNull) offset++; } - return { offset, params }; + return { lastParam, offset, params }; +} + +/** + * SMPP 3.4 4.3: the optional parameters run to command_length exactly, so an octet left over is a + * TLV stream this codec could not read rather than slack to drop. + */ +function readOptionalParams( + pdu: Buffer, + start: number, + afterShortMessage: boolean, +): Result<{ tlvs: Record }> { + const plain = parseTlvs(pdu, start); + + if (!plain.err && plain.offset === pdu.length) return { tlvs: plain.tlvs }; + + // Some peers append a NULL octet after short_message; that octet, and no other, is skipped. + if (afterShortMessage && pdu[start] === 0) { + const padded = parseTlvs(pdu, start + 1); + + if (!padded.err && padded.offset === pdu.length) return { tlvs: padded.tlvs }; + } + + return { + err: plain.err ?? new Error( + `${String(pdu.length - plain.offset)} octets are left over after the optional parameters`, + ), + }; } function headerOf(pdu: Buffer): PduHeader { @@ -314,7 +336,7 @@ function headerOf(pdu: Buffer): PduHeader { }; } -function parseOnce(pdu: Buffer, trailingNull: boolean): Result<{ aligned: boolean; pduObj: PduObject }> { +function parsePdu(pdu: Buffer): Result<{ pduObj: PduObject }> { const header = headerOf(pdu); const { cmdId, cmdLength, cmdName, cmdStatusId, seqNr } = header; @@ -322,20 +344,20 @@ function parseOnce(pdu: Buffer, trailingNull: boolean): Result<{ aligned: boolea return { err: new PduRefusedError(header, 'command', new Error('Unknown PDU command id')) }; } + const declared = pdu.subarray(0, cmdLength); // SMPP 3.4 4.4.2 and friends: a response with a non-zero status carries no body at all. const read = cmdStatusId !== 0 && cmdLength === 16 - ? { offset: 16, params: {} } - : readParams(cmdName, pdu, trailingNull); + ? { lastParam: undefined, offset: 16, params: {} } + : readParams(cmdName, declared); if (read.err) return { err: new PduRefusedError(header, 'body', read.err) }; - const parsed = parseTlvs(pdu, read.offset, cmdLength); - - if (parsed.err) return { err: new PduRefusedError(header, 'tlvs', parsed.err) }; - const params = read.params; const message = params.short_message; const octets = Buffer.isBuffer(message) ? message : undefined; + const parsed = readOptionalParams(declared, read.offset, read.lastParam === 'short_message'); + + if (parsed.err) return { err: new PduRefusedError(header, 'tlvs', parsed.err) }; // A message carrying a UDH stays a buffer; the session needs the header intact to reassemble. if (octets && !hasUdh(paramNumber(params.esm_class, 0))) { @@ -343,7 +365,6 @@ function parseOnce(pdu: Buffer, trailingNull: boolean): Result<{ aligned: boolea } return { - aligned: parsed.offset === cmdLength, pduObj: { cmdId, cmdLength, @@ -380,18 +401,7 @@ export function pduToObj(pdu: Buffer): Result<{ pduObj: PduObject }> { if (framing.err) return { err: framing.err }; - const plain = parseOnce(pdu, false); - - if (!plain.err && plain.aligned) return { pduObj: plain.pduObj }; - - // Some peers append a NULL octet after short_message; allow for it before giving up. - const padded = parseOnce(pdu, true); - - if (!padded.err && padded.aligned) return { pduObj: padded.pduObj }; - if (!plain.err) return { pduObj: plain.pduObj }; - if (!padded.err) return { pduObj: padded.pduObj }; - - return { err: plain.err }; + return parsePdu(pdu); } /** diff --git a/test/pdu.test.ts b/test/pdu.test.ts index 3ffb7f3..8b60776 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -87,6 +87,46 @@ describe('parsing real PDUs', () => { assert.equal(pduObj.params.short_message, 'test'); assert.equal(pduObj.cmdLength, 59); }); + + // SMPP 3.4 4.6.2 leaves deliver_sm_resp's message_id unused and peers send the response with no + // body at all, so the last C-Octet String of a PDU is one a peer may leave out entirely. + test('reads a PDU whose trailing C-Octet String was left out altogether', () => { + const bind = encode({ + cmdName: 'bind_transceiver', + params: { password: 'secret08', system_id: 'SMPP3TEST' }, + seqNr: 1, + }); + const withoutRange = bind.subarray(0, bind.length - 1); + + withoutRange.writeUInt32BE(withoutRange.length, 0); + + const bound = decode(withoutRange); + + assert.equal(bound.params.system_id, 'SMPP3TEST'); + assert.equal(bound.params.address_range, ''); + assert.equal(decode(Buffer.from('00000010800000050000000000000007', 'hex')).params.message_id, ''); + assert.equal(decode(Buffer.from('00000010800000040000000000000007', 'hex')).params.message_id, ''); + }); + + // The padded read skips one NULL octet and no more: the body's own last octet is 0x00 here, and + // the optional parameters behind the padding still have to be read. + test('reads past that NULL octet to the optional parameters behind it', () => { + const padded = Buffer.concat([ + encode({ + cmdName: 'deliver_sm', + params: { destination_addr: '46709771337', short_message: 'hej 一', source_addr: '46701113311' }, + seqNr: 41, + }), + Buffer.from('000427000102', 'hex'), + ]); + + padded.writeUInt32BE(padded.length, 0); + + const pduObj = decode(padded); + + assert.equal(pduObj.params.short_message, 'hej 一'); + assert.equal(pduObj.tlvs.message_state?.tagValue, 2); + }); }); describe('encoding submit_sm', () => { @@ -532,6 +572,37 @@ describe('malformed input', () => { }); }); + // interop-tests/findings/05-java-clients.md's reproducer, octet for octet. + test('refuses a bare TLV header the same way it refuses a truncated value', () => { + const refused = pduToObj(Buffer.from( + '000000460000000500000000000000630000007261772d66726f6d0000007261772d746f0000000000000000000013' + + '7472756e636174656420746c762070726f6265001d00c8', + 'hex', + )).err; + + assert.ok(refused instanceof PduRefusedError); + assert.equal(refused.reason, 'tlvs'); + assert.equal(refused.header.seqNr, 99); + assert.deepEqual(refusalAnswer(refused), { + cmdName: 'deliver_sm_resp', + cmdStatus: 'ESME_RINVTLVSTREAM', + }); + }); + + test('refuses octets left over after the optional parameters', () => { + const slack = Buffer.concat([ + encode({ cmdName: 'deliver_sm', params: { short_message: 'hello' }, seqNr: 13 }), + Buffer.from('4142', 'hex'), + ]); + + slack.writeUInt32BE(slack.length, 0); + + const refused = pduToObj(slack).err; + + assert.ok(refused instanceof PduRefusedError); + assert.equal(refused.reason, 'tlvs'); + }); + test('answers a command with no response of its own with generic_nack', () => { const outbind = encode({ cmdName: 'outbind', params: { system_id: 'smsc' }, seqNr: 12 }); // Both C-Octet Strings lose their terminator, so system_id runs off the end of the PDU. diff --git a/test/raw-pdus.ts b/test/raw-pdus.ts index 90b316f..46ae6f4 100644 --- a/test/raw-pdus.ts +++ b/test/raw-pdus.ts @@ -38,3 +38,12 @@ export function truncatedTlv(input: PduObjectInput): Buffer { return appended; } + +/** The same, ending in a bare TLV header: a tag, a declared length, and no value octets at all. */ +export function bareTlvHeader(input: PduObjectInput): Buffer { + const appended = Buffer.concat([pduBytes(input), Buffer.from('001d00c8', 'hex')]); + + appended.writeUInt32BE(appended.length, 0); + + return appended; +} diff --git a/test/session-error.test.ts b/test/session-error.test.ts index 7496f17..af9b3b9 100644 --- a/test/session-error.test.ts +++ b/test/session-error.test.ts @@ -3,8 +3,8 @@ import test, { describe } from 'node:test'; import type { PduHeader, Session } from '../src/index.ts'; import type { TestContext } from 'node:test'; import { PduRefusedError, client, server } from '../src/index.ts'; +import { bareTlvHeader, shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts'; import { closeAfter } from './teardown.ts'; -import { shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts'; const receipt = { destination_addr: '46709771337', @@ -85,11 +85,12 @@ describe('telling a refused PDU from a failed session', () => { peer.sock.write(withUnknownCmdId({ cmdName: 'enquire_link', seqNr: 9 })); peer.sock.write(shortened({ cmdName: 'deliver_sm', params: receipt, seqNr: 6 }, 3)); peer.sock.write(truncatedTlv({ cmdName: 'deliver_sm', params: receipt, seqNr: 7 })); + peer.sock.write(bareTlvHeader({ cmdName: 'deliver_sm', params: receipt, seqNr: 8 })); - assert.ok(await waitFor(() => seen.length === 3), 'one sessionError per refused PDU'); + assert.ok(await waitFor(() => seen.length === 4), 'one sessionError per refused PDU'); assert.deepEqual( seen.map(err => (err instanceof PduRefusedError ? err.reason : err.message)), - ['command', 'body', 'tlvs'], + ['command', 'body', 'tlvs', 'tlvs'], ); }); diff --git a/test/session.test.ts b/test/session.test.ts index d50d6a9..35c5da1 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -19,7 +19,7 @@ import { PduRefusedError } from '../src/pdu-refusal.ts'; import { isCommand, objToPdu, pduReturn, pduToObj } from '../src/pdu.ts'; import { paramText } from '../src/defs/types.ts'; import { server } from '../src/server.ts'; -import { pduBytes, shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts'; +import { bareTlvHeader, pduBytes, shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts'; import { silentLog } from '../src/log.ts'; import { splitMessage } from '../src/message.ts'; @@ -1568,6 +1568,24 @@ describe('a PDU the codec cannot read', () => { assert.equal((await answerTo(peer)).cmdName, 'enquire_link_resp'); }); + test('answers a deliver_sm ending in a bare TLV header with ESME_RINVTLVSTREAM', async t => { + const { peer, session } = await bound(t); + let reports = 0; + + session.on('dlr', () => { reports++; }); + peer.writeRaw(bareTlvHeader({ cmdName: 'deliver_sm', params: receipt, seqNr: 55 })); + + const answered = await answerTo(peer); + + assert.equal(answered.cmdName, 'deliver_sm_resp'); + assert.equal(answered.cmdStatus, 'ESME_RINVTLVSTREAM'); + assert.equal(answered.seqNr, 55); + assert.equal(reports, 0, 'a PDU whose optional parameters were never read is no report'); + + peer.writeRaw(pduBytes({ cmdName: 'enquire_link', seqNr: 79 })); + assert.equal((await answerTo(peer)).cmdName, 'enquire_link_resp'); + }); + test('answers a deliver_sm whose body is shorter than it declares with ESME_RINVCMDLEN', async t => { const { peer } = await bound(t); diff --git a/test/types.test.ts b/test/types.test.ts index 516d71a..4d8f0a4 100644 --- a/test/types.test.ts +++ b/test/types.test.ts @@ -108,8 +108,9 @@ describe('cstring (C-Octet String)', () => { assert.ok(types.cstring.read(encoded, encoded.length + 1).err instanceof Error); assert.ok(types.cstring.read(encoded, -1).err instanceof Error); - // At the end exactly the field is absent, not corrupt: peers truncate a NULL-only body. - assert.deepEqual(types.cstring.read(encoded, encoded.length), { bytesRead: 1, value: '' }); + // At the end exactly the field is absent, not corrupt, and consumes no octet: counting one + // puts every later offset past the declared end. + assert.deepEqual(types.cstring.read(encoded, encoded.length), { bytesRead: 0, value: '' }); }); }); @@ -187,6 +188,12 @@ describe('dest_address_array', () => { bytesRead: 13, value: expected, }); + + // A structure that ran out counts the octets that were there, never the terminator that was not. + assert.deepEqual(types.dest_address_array.read(Buffer.from([0x01, 0x01, 0x00, 0x00]), 0), { + bytesRead: 4, + value: [{ dest_addr_npi: 0, dest_addr_ton: 0, destination_addr: '' }], + }); }); test('sizes every dest_address structure', () => {