diff --git a/AGENTS.md b/AGENTS.md index ded3adb..06540eb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -292,7 +292,8 @@ the file. - A string body is written in the alphabet its own `data_coding` names, and one that alphabet cannot carry is refused by the codec — `message_payload` on the same terms as `short_message`. - A GSM 03.38 message declares `data_coding` 0x00, and an inbound 0x01 is still read as GSM. -- Every text field on the wire is latin1, and a character past `U+00FF` is refused. +- Every text field on the wire is latin1, and what the field cannot carry is refused rather than + truncated. ### [The session's life](docs/decisions.md#the-sessions-life) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60d1055..9ee8857 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`, `service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF` in one of those fields is now refused, where it used to go out as its low octet. +- A `U+0000` inside a C-Octet String — `source_addr`, `message_id`, `system_id` and the rest — is + refused. The peer reads such a field to its first NULL, so one sent inside the value shifted every + mandatory field behind it while `command_length` still counted the whole string. An Octet String + carries a NULL as before; its length octet is what ends it. ## 0.5.0 diff --git a/README.md b/README.md index 8c95f4b..3fad325 100644 --- a/README.md +++ b/README.md @@ -289,7 +289,8 @@ await session.sendSms({ **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` goes out and -comes back as its own octets; a character past `U+00FF` is refused rather than sent truncated. +comes back as its own octets; a character past `U+00FF`, or a `U+0000` that would end the field +early, is refused rather than sent truncated. **Encoding.** diff --git a/docs/decisions.md b/docs/decisions.md index 16066c4..70748b0 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -478,8 +478,9 @@ rule and an index of the titles below. concatenation reference rather than on `data_coding` — so a receipt or a segment that crossed the change reads exactly as it did. -- **Every text field on the wire is latin1, and a character past `U+00FF` is refused.** Maintainer's - call, 2026-09-21, from the 0.6.0 correctness list: the codec read these fields with +- **Every text field on the wire is latin1, and what the field cannot carry is refused rather than + truncated.** Maintainer's call, 2026-09-21, from the 0.6.0 correctness list: the codec read these + fields with `toString('ascii')`, which masks bit 7, and wrote them with `write(text, 'ascii')`, which does not, so an inbound `source_addr` of `Kaffeé` reached the application as `Kaffei` and `objToPdu(pduToObj(x))` was idempotent for none of `source_addr`, `destination_addr`, `system_id`, @@ -494,7 +495,13 @@ rule and an index of the titles below. alphabet that cannot carry a message body is already handled. Rejected: reading latin1 and leaving the write spelled ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the upper half on send to stay strict to 3.4's ASCII, which would drop exactly the traffic the read - keeps. + keeps. The security pass over this chunk found the field's other end open the same way: a caller's + own `U+0000` was written verbatim, and since the peer reads a C-Octet String to its first NULL, + every mandatory field behind it shifted under a `command_length` that had counted the whole string + — so an application forwarding a customer's sender id could have a PDU rewritten from inside one. + `wantCstringText()` refuses that where `wantText()` refuses the upper end. Rejected: refusing NULL + in every text field, which would buy one spelling by taking a legitimate octet away from the + length-prefixed Octet String, whose length octet is what ends it. ## The session's life diff --git a/src/defs/types.ts b/src/defs/types.ts index e7589d4..24c0b4b 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -94,10 +94,30 @@ function pastLatin1(text: string): { err: Error } | undefined { } function wantText(value: ParamValue): Result<{ text: string }> { - if (typeof value === 'number') return { text: value.toString() }; - if (typeof value !== 'string') return { err: new Error(`Expected a string, got ${typeof value}`) }; + if (typeof value !== 'number' && typeof value !== 'string') { + return { err: new Error(`Expected a string, got ${typeof value}`) }; + } - return pastLatin1(value) ?? { text: value }; + const text = String(value); + + return pastLatin1(text) ?? { text }; +} + +/** A C-Octet String ends at its first NULL, so one inside the value truncates the field on the peer. */ +function wantCstringText(value: ParamValue): Result<{ text: string }> { + const { err, text } = wantText(value); + + if (err) return { err }; + + const index = text.indexOf('\u0000'); + + if (index === -1) return { text }; + + return { + err: new Error( + `U+0000 at index ${String(index)} would end the C-Octet String there, ${String(text.length - index - 1)} characters early`, + ), + }; } function wantBytes(value: ParamValue): Result<{ bytes: Buffer }> { @@ -302,12 +322,12 @@ export const cstring: WireType = { default: '', read: readCstring, size(value) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : { size: text.length + 1 }; }, write(value, buffer, offset) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : writeCstring(text, buffer, offset); }, @@ -555,12 +575,12 @@ export const tlv = { return { bytesRead: length, value: buf.toString('latin1', offset, end) }; }, size(value: ParamValue) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : { size: text.length + 1 }; }, write(value: ParamValue, buf: Buffer, offset: number) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : writeCstring(text, buf, offset); }, diff --git a/test/pdu.test.ts b/test/pdu.test.ts index 8d8a296..1981dab 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -145,6 +145,18 @@ describe('parsing real PDUs', () => { assert.equal(decode(pdu).params.source_addr, 'Kaffeé'); assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); }); + + // The peer reads source_addr to the first NULL and every mandatory field behind it shifts, so an + // application forwarding a customer's sender id could have a PDU rewritten under it. + test('refuses an address carrying its own terminator', () => { + const smuggled = objToPdu({ + cmdName: 'submit_sm', + params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' }, + }); + + assert.ok(smuggled.err instanceof Error); + assert.equal(smuggled.buffer, undefined); + }); }); describe('encoding submit_sm', () => { diff --git a/test/types.test.ts b/test/types.test.ts index 1da0051..d755ea4 100644 --- a/test/types.test.ts +++ b/test/types.test.ts @@ -83,6 +83,14 @@ describe('string (Octet String)', () => { assert.ok(types.string.size('一').err instanceof Error); assert.ok(types.string.write('一', Buffer.alloc(4), 0).err instanceof Error); }); + + // Its length octet is what ends it, so unlike a C-Octet String it carries a NULL like any other. + test('carries a NULL octet, which its length octet already bounds', () => { + const target = Buffer.alloc(4); + + assert.deepEqual(types.string.write('a\u0000b', target, 0), {}); + assert.deepEqual(target, Buffer.from([3, 0x61, 0x00, 0x62])); + }); }); describe('cstring (C-Octet String)', () => { @@ -128,6 +136,14 @@ describe('cstring (C-Octet String)', () => { assert.ok(types.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); }); + // The field ends at its first NULL, so writing one smuggles a field boundary into the peer's + // parse: every mandatory field behind it shifts, under a command_length that counted the whole + // string. + test('refuses a NULL of its own rather than ending the field early', () => { + assert.ok(types.cstring.size('46701113311\u0000EVIL').err instanceof Error); + assert.ok(types.cstring.write('46701113311\u0000EVIL', Buffer.alloc(17), 0).err instanceof Error); + }); + test('refuses a string with no terminator rather than running off the end', () => { assert.ok(types.cstring.read(Buffer.from('abcd'), 0).err instanceof Error); }); @@ -193,6 +209,7 @@ describe('text TLVs', () => { assert.ok(types.tlv.cstring.size('一').err instanceof Error); assert.ok(types.tlv.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); + assert.ok(types.tlv.cstring.write('a\u0000b', Buffer.alloc(4), 0).err instanceof Error); }); });