Refuse a NULL inside a C-Octet String, which ends the field on the peer
Mirror / push (push) Successful in 4s
Test / lint (pull_request) Successful in 21s
Test / test (18) (pull_request) Successful in 29s
Test / test (20) (pull_request) Successful in 29s
Test / test (22) (pull_request) Successful in 31s
Test / test (24) (pull_request) Successful in 35s
Test / test (26) (pull_request) Successful in 30s

This commit is contained in:
2026-09-21 07:59:08 +02:00
parent 7db242e375
commit c1e0407942
7 changed files with 74 additions and 12 deletions
+2 -1
View File
@@ -292,7 +292,8 @@ the file.
- A string body is written in the alphabet its own `data_coding` names, and one that alphabet cannot - 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`. 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. - 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) ### [The session's life](docs/decisions.md#the-sessions-life)
+4
View File
@@ -11,6 +11,10 @@
the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`, 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` `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.
- 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 ## 0.5.0
+2 -1
View File
@@ -289,7 +289,8 @@ await session.sendSms({
**Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address **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 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.** **Encoding.**
+10 -3
View File
@@ -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 concatenation reference rather than on `data_coding` — so a receipt or a segment that crossed the
change reads exactly as it did. change reads exactly as it did.
- **Every text field on the wire is latin1, and a character past `U+00FF` is refused.** Maintainer's - **Every text field on the wire is latin1, and what the field cannot carry is refused rather than
call, 2026-09-21, from the 0.6.0 correctness list: the codec read these fields with 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, `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 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`, `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 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 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 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 ## The session's life
+27 -7
View File
@@ -94,10 +94,30 @@ function pastLatin1(text: string): { err: Error } | undefined {
} }
function wantText(value: ParamValue): Result<{ text: string }> { function wantText(value: ParamValue): Result<{ text: string }> {
if (typeof value === 'number') return { text: value.toString() }; if (typeof value !== 'number' && typeof value !== 'string') {
if (typeof value !== 'string') return { err: new Error(`Expected a string, got ${typeof value}`) }; 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 }> { function wantBytes(value: ParamValue): Result<{ bytes: Buffer }> {
@@ -302,12 +322,12 @@ export const cstring: WireType<string> = {
default: '', default: '',
read: readCstring, read: readCstring,
size(value) { size(value) {
const { err, text } = wantText(value); const { err, text } = wantCstringText(value);
return err ? { err } : { size: text.length + 1 }; return err ? { err } : { size: text.length + 1 };
}, },
write(value, buffer, offset) { write(value, buffer, offset) {
const { err, text } = wantText(value); const { err, text } = wantCstringText(value);
return err ? { err } : writeCstring(text, buffer, offset); return err ? { err } : writeCstring(text, buffer, offset);
}, },
@@ -555,12 +575,12 @@ export const tlv = {
return { bytesRead: length, value: buf.toString('latin1', offset, end) }; return { bytesRead: length, value: buf.toString('latin1', offset, end) };
}, },
size(value: ParamValue) { size(value: ParamValue) {
const { err, text } = wantText(value); const { err, text } = wantCstringText(value);
return err ? { err } : { size: text.length + 1 }; return err ? { err } : { size: text.length + 1 };
}, },
write(value: ParamValue, buf: Buffer, offset: number) { write(value: ParamValue, buf: Buffer, offset: number) {
const { err, text } = wantText(value); const { err, text } = wantCstringText(value);
return err ? { err } : writeCstring(text, buf, offset); return err ? { err } : writeCstring(text, buf, offset);
}, },
+12
View File
@@ -145,6 +145,18 @@ describe('parsing real PDUs', () => {
assert.equal(decode(pdu).params.source_addr, 'Kaffeé'); assert.equal(decode(pdu).params.source_addr, 'Kaffeé');
assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); 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', () => { describe('encoding submit_sm', () => {
+17
View File
@@ -83,6 +83,14 @@ describe('string (Octet String)', () => {
assert.ok(types.string.size('一').err instanceof Error); assert.ok(types.string.size('一').err instanceof Error);
assert.ok(types.string.write('一', Buffer.alloc(4), 0).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)', () => { 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); 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', () => { 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); 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.size('一').err instanceof Error);
assert.ok(types.tlv.cstring.write('一', Buffer.alloc(4), 0).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);
}); });
}); });