diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ee8857..3027ec5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ `source_addr` of `Kaffeé` previously reached the application as `Kaffei`, because the codec wrote 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. + in one of those fields is now refused, where it used to go out as its low octet — which for `一`, + `　` and most emoji is `0x00`, ending the field there. - 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 diff --git a/README.md b/README.md index 3fad325..1cc3dc7 100644 --- a/README.md +++ b/README.md @@ -288,9 +288,9 @@ 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`, or a `U+0000` that would end the field -early, is refused rather than sent truncated. +and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` reaches the +peer as its own octets; a character past `U+00FF`, or a `U+0000`, is refused rather than sent +truncated. **Encoding.** diff --git a/docs/decisions.md b/docs/decisions.md index 70748b0..e911f74 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -479,29 +479,22 @@ rule and an index of the titles below. change reads exactly as it did. - **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`, - `message_id`, `service_type` or the cstring TLVs. Goal 3 settles the read: 3.4 calls these fields - ASCII, and an operator routing an alphanumeric sender through the upper half is traffic to keep. - The write is named latin1 to match, which is what makes the round trip idempotent and is already - the octets Node put on the wire, so no peer sees a change. `wantText()` is the single place that - says so — which is why the `dest_address` and `unsuccess_sme` structures write their embedded - addresses through `cstring.write()` rather than reaching past it — and a character past `U+00FF` - is refused there rather than truncated to its low octet: a `size()` that agreed with a `write()` - that dropped half a character is a wrong answer about what went out (goal 2), and it is how an - 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. 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. + truncated.** Maintainer's call, 2026-09-21, the refusals from the security and stability passes on + [#16](https://gitea.larvit.se/larvit/smpp-js/pulls/16). Goal 3 settles the alphabet: 3.4 calls + these fields ASCII, and an operator routing an alphanumeric sender through the upper half is + traffic to keep, so the read is latin1 and the write is named latin1 to match — which is what + makes the round trip idempotent, and is already the octets Node put on the wire, so no peer sees a + change. Goal 2 settles the refusals, both of them a `size()` that would have agreed with a + `write()` that put something else on the wire: a character past `U+00FF` written as its low octet, + and a caller's own `U+0000`, which the peer reads as the end of the field, shifting every mandatory + field behind it under a `command_length` that counted the whole string. `wantText()` and + `wantCstringText()` are the only two places that decide it, which is why the `dest_address` and + `unsuccess_sme` structures write their embedded addresses through `cstring.write()` rather than + reaching past it into `writeCstring()`. 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. Rejected: + refusing `U+0000` 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 24c0b4b..b12dd6a 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -84,7 +84,7 @@ function pastLatin1(text: string): { err: Error } | undefined { if (index === -1) return undefined; - const code = text.charCodeAt(index).toString(16).toUpperCase().padStart(4, '0'); + const code = (text.codePointAt(index) ?? 0).toString(16).toUpperCase().padStart(4, '0'); return { err: new Error( @@ -103,7 +103,6 @@ function wantText(value: ParamValue): Result<{ text: string }> { 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); @@ -114,9 +113,7 @@ function wantCstringText(value: ParamValue): Result<{ text: string }> { 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`, - ), + err: new Error(`U+0000 at index ${String(index)} would end the C-Octet String there`), }; } diff --git a/test/pdu.test.ts b/test/pdu.test.ts index 1981dab..7d44959 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -128,7 +128,6 @@ describe('parsing real PDUs', () => { assert.equal(pduObj.tlvs.message_state?.tagValue, 2); }); - // 'ascii' masks bit 7 on the way in, so this address used to come back Kaffei. test('keeps an alphanumeric sender whole through the wire and back', () => { const pdu = encode({ cmdName: 'deliver_sm', @@ -143,12 +142,9 @@ describe('parsing real PDUs', () => { assert.ok(pdu.includes(Buffer.from('Kaffeé', 'latin1'))); 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', () => { + test('refuses an address the field cannot carry rather than truncating it', () => { const smuggled = objToPdu({ cmdName: 'submit_sm', params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' }, @@ -156,6 +152,7 @@ describe('parsing real PDUs', () => { assert.ok(smuggled.err instanceof Error); assert.equal(smuggled.buffer, undefined); + assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); }); }); diff --git a/test/types.test.ts b/test/types.test.ts index d755ea4..d353335 100644 --- a/test/types.test.ts +++ b/test/types.test.ts @@ -84,7 +84,6 @@ describe('string (Octet String)', () => { 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); @@ -122,8 +121,6 @@ describe('cstring (C-Octet String)', () => { assert.deepEqual(types.cstring.size(123), { size: 4 }); }); - // 'ascii' masks bit 7 on the way in and keeps it on the way out, so an address a peer wrote as - // Kaffeé came back Kaffei and objToPdu(pduToObj(x)) stopped being idempotent. test('carries every latin1 octet, and refuses a character past it', () => { const address = Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9, 0x00]); const target = Buffer.alloc(7); @@ -136,9 +133,6 @@ 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); @@ -252,8 +246,6 @@ describe('buffer', () => { }); describe('paramText()', () => { - // The one reader of a receipt body that arrived with no octets of its own, so masking bit 7 - // here loses the same characters the wire types used to. test('renders a Buffer parameter as the latin1 text its octets spell', () => { assert.equal(paramText(Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9])), 'Kaffeé'); }); @@ -293,6 +285,11 @@ describe('dest_address_array', () => { types.dest_address_array.write(expected, target, 0); assert.deepEqual(target, encoded); + + const smuggled: DestAddress[] = [{ dest_addr_npi: 1, dest_addr_ton: 1, destination_addr: '46\u0000EVIL' }]; + + assert.ok(types.dest_address_array.write(smuggled, Buffer.alloc(16), 0).err instanceof Error); + assert.ok(types.dest_address_array.write([{ dl_name: '一' }], Buffer.alloc(16), 0).err instanceof Error); }); test('refuses a field value the wire cannot hold instead of throwing', () => { @@ -332,6 +329,12 @@ describe('unsuccess_sme_array', () => { types.unsuccess_sme_array.write(expected, target, 0); assert.deepEqual(target, encoded); + + const smuggled: UnsuccessSme[] = [ + { dest_addr_npi: 1, dest_addr_ton: 1, destination_addr: 'a\u0000b', error_status_code: 0 }, + ]; + + assert.ok(types.unsuccess_sme_array.write(smuggled, Buffer.alloc(16), 0).err instanceof Error); }); test('refuses a field value the wire cannot hold instead of throwing', () => { diff --git a/test/unsendable.test.ts b/test/unsendable.test.ts index cdcda44..07f88e5 100644 --- a/test/unsendable.test.ts +++ b/test/unsendable.test.ts @@ -345,6 +345,30 @@ describe('a body the PDU\'s own data_coding cannot carry', () => { }); }); +describe('an address the field cannot carry', () => { + test('refuses it through sendSms(), with nothing reaching the socket', async t => { + const smsc = await dummySmsc(t); + const session = await bindToSmsc(t, smsc.port, { reconnect: false }); + + const refusals: [string, RegExp][] = [ + ['46701113311\u0000EVIL', /U\+0000 at index 11/], + ['Kaffe一', /U\+4E00 at index 5/], + ['😀', /U\+1F600 at index 0/], + ]; + + for (const [sender, names] of refusals) { + const sent = await session.sendSms({ from: sender, message: 'Hello world', to }); + + assert.ok(sent.err instanceof Error, sender); + assert.match(sent.err.message, /source_addr/); + assert.match(sent.err.message, names); + assert.deepEqual(sent.smsIds, [], sender); + } + + assert.deepEqual(smsc.octets, [], 'an address the field cannot carry never reaches the socket'); + }); +}); + describe('a time no peer can read', () => { const invalid = new Date('nope'); diff --git a/todo.md b/todo.md index 473e856..685cf8a 100644 --- a/todo.md +++ b/todo.md @@ -259,6 +259,14 @@ and is also what the panel ranked hardest — two methods, one answer. budget into `ExpiringGroups` as a weighed capacity, and have `trim()` report whether the current group survived. Named by 6 of 9 readers. +- [ ] **Let the two address arrays size a C-Octet String through `cstring.size()`.** + `sizeDestAddresses()` and `sizeUnsuccessSmes()` spell "len + 1" themselves, and each `offset +=` + after a write spells it a third time, so `dest_address_array` and `unsuccess_sme_array` each + know the cost in three places. Route both through `cstring.size()` and advance the offset by + what it returns. That also makes `size()` refuse where `write()` already does, so the error + arrives from the first call rather than the second; today the pair only fails closed because + `writeParams()` and `writeTlvs()` both bail on the write. From the stability review of #16. + - [ ] **Split the two questions `OutgoingRequests.linkDown()` answers.** `Session.drain()` calls it twice for opposite conclusions — "nothing to drain, success" and "the link died under us, failure" — and `outgoing-requests.ts` reads it a third way. Two named predicates. Named by 7