Skip to content

Commit 45a2b11

Browse files
beaugundersongaoflow
beaugunderson
and
gaoflow
authored
Validate the byte arrays Address6 is given (#217)
Address6.fromByteArray and fromUnsignedByteArray fold whatever they are given into a BigInt and bounds-check only the aggregate, so wrong-length, out-of-range and non-integer input produces a plausible wrong address instead of an error: three bytes give ::1:203, seventeen bytes whose first is zero give ::1, a low byte of 300 gives ::12c, and a byte of 1.5 gives 100::. A 17-byte array shows why the aggregate bound cannot cover this, since a leading zero keeps the magnitude under 2**128. Address4 rejects all four. This reaches consumers. socks calls Address6.fromByteArray at three sites with Array.from(buff.readBuffer(16)), and in parseUDPFrame that buffer is an unvalidated datagram while smart-buffer's readBuffer clamps a short read with Math.min rather than throwing. A SOCKS5 UDP frame carrying four address bytes therefore reports a remote host of 0000:0000:0000:0000:0000:0000:dead:beef. Both methods now require exactly 16 bytes. fromByteArray keeps folding signed bytes to unsigned, so an Int8Array or a Java byte[] still works and nothing that parses today stops parsing; its floor is -128, below which folding would silently produce a value the caller did not mean. fromUnsignedByteArray takes 0 to 255. The length and range checks live in one place that Address4 shares, replacing its copy of the same loop, with its messages unchanged. Address6.fromByteArray accepting signed bytes while Address4.fromByteArray rejects them, and toUnsignedByteArray returning what toByteArray already returns, are differences to settle in a major version rather than here, where the point is to reach the versions socks resolves. A test asserts the major version is below 11 and names what to delete when it is not. The byte array parameters are typed number[] rather than any[], matching Address4. Co-authored-by: gaoflow <[email protected]>
1 parent bac8810 commit 45a2b11

6 files changed

Lines changed: 230 additions & 52 deletions

File tree

‎README.md‎

Lines changed: 40 additions & 40 deletions
Large diffs are not rendered by default.

‎src/common.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,28 @@ export function prefixLengthFromMask(value: bigint, totalBits: number): number {
7979
return firstZero;
8080
}
8181

82+
/**
83+
* Throws `AddressError` unless `bytes` holds exactly `byteCount` integers,
84+
* each from `minimum` to 255. Pass a `minimum` of `-128` where signed bytes
85+
* are accepted and folded to unsigned, and `0` where they are not.
86+
*/
87+
export function assertByteArray(
88+
bytes: number[],
89+
byteCount: number,
90+
family: 'IPv4' | 'IPv6',
91+
minimum: number,
92+
): void {
93+
if (bytes.length !== byteCount) {
94+
throw new AddressError(`${family} addresses require exactly ${byteCount} bytes`);
95+
}
96+
97+
for (let i = 0; i < bytes.length; i++) {
98+
if (!Number.isInteger(bytes[i]) || bytes[i] < minimum || bytes[i] > 255) {
99+
throw new AddressError(`All bytes must be integers between ${minimum} and 255`);
100+
}
101+
}
102+
}
103+
82104
export function numberToPaddedHex(number: number) {
83105
return number.toString(16).padStart(2, '0');
84106
}

‎src/ipv4.ts‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -379,16 +379,7 @@ export class Address4 {
379379
* @returns {Address4}
380380
*/
381381
static fromByteArray(bytes: Array<number>): Address4 {
382-
if (bytes.length !== 4) {
383-
throw new AddressError('IPv4 addresses require exactly 4 bytes');
384-
}
385-
386-
// Validate that all bytes are within valid range (0-255)
387-
for (let i = 0; i < bytes.length; i++) {
388-
if (!Number.isInteger(bytes[i]) || bytes[i] < 0 || bytes[i] > 255) {
389-
throw new AddressError('All bytes must be integers between 0 and 255');
390-
}
391-
}
382+
common.assertByteArray(bytes, 4, 'IPv4', 0);
392383

393384
return this.fromUnsignedByteArray(bytes);
394385
}

‎src/ipv6.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,26 +1155,43 @@ export class Address6 {
11551155
* @returns {Array}
11561156
*/
11571157
toUnsignedByteArray(): number[] {
1158+
// toByteArray() emits 0 to 255, so unsigning it is an identity mapping and
1159+
// the two methods return equal arrays. 11.0.0 keeps one of them and makes
1160+
// this a deprecated alias; test/common-test.ts fails at that version.
11581161
return this.toByteArray().map(unsignByte);
11591162
}
11601163

11611164
/**
11621165
* Convert a byte array to an Address6 object.
11631166
*
1167+
* Accepts unsigned bytes (0 to 255) or signed bytes (-128 to 127, as an
1168+
* `Int8Array` or a Java `byte[]` holds them), folding signed values to their
1169+
* unsigned equivalent. Throws `AddressError` unless given exactly 16
1170+
* integers from -128 to 255.
1171+
*
11641172
* To convert from a Node.js `Buffer`, spread it: `Address6.fromByteArray([...buf])`.
11651173
* @returns {Address6}
11661174
*/
1167-
static fromByteArray(bytes: Array<any>): Address6 {
1175+
static fromByteArray(bytes: Array<number>): Address6 {
1176+
// Address4.fromByteArray takes unsigned bytes only. 11.0.0 aligns this
1177+
// method with it, at which point the -128 floor here, unsignByte, and the
1178+
// mapping below all go; test/common-test.ts fails at that version.
1179+
common.assertByteArray(bytes, 16, 'IPv6', -128);
1180+
11681181
return this.fromUnsignedByteArray(bytes.map(unsignByte));
11691182
}
11701183

11711184
/**
11721185
* Convert an unsigned byte array to an Address6 object.
11731186
*
1187+
* Throws `AddressError` unless given exactly 16 integers from 0 to 255.
1188+
*
11741189
* To convert from a Node.js `Buffer`, spread it: `Address6.fromUnsignedByteArray([...buf])`.
11751190
* @returns {Address6}
11761191
*/
1177-
static fromUnsignedByteArray(bytes: Array<any>): Address6 {
1192+
static fromUnsignedByteArray(bytes: Array<number>): Address6 {
1193+
common.assertByteArray(bytes, 16, 'IPv6', 0);
1194+
11781195
const BYTE_MAX = BigInt('256');
11791196
let result = BigInt('0');
11801197
let multiplier = BigInt('1');

‎test/common-test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,30 @@
11
import * as chai from 'chai';
22
import { testBit } from '../src/common';
33

4+
// eslint-disable-next-line import/extensions
5+
import pkg from '../package.json';
6+
47
const should = chai.should();
58

9+
describe('the byte array API', () => {
10+
it('keeps its signed and unsigned split only until 11.0.0', () => {
11+
// Address6.fromByteArray accepts signed bytes and folds them to unsigned,
12+
// Address4.fromByteArray rejects them, and toUnsignedByteArray returns what
13+
// toByteArray already returns. A major version is where those differences
14+
// can be settled, so this fails there rather than relying on anyone
15+
// remembering to look.
16+
const major = Number(pkg.version.split('.')[0]);
17+
18+
major.should.be.below(
19+
11,
20+
'ip-address is at 11.x, so collapse the byte array API: make Address6.fromByteArray ' +
21+
'reject anything outside 0-255 as Address4.fromByteArray does, delete unsignByte and ' +
22+
'the -128 floor from src/ipv6.ts, and reduce fromUnsignedByteArray and ' +
23+
'toUnsignedByteArray to deprecated aliases. Then delete this test.',
24+
);
25+
});
26+
});
27+
628
describe('testBit', () => {
729
it('should return value per specific bit', () => {
830
should.equal(testBit('0', 1), false);

‎test/functionality-v6-test.ts‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1528,6 +1528,132 @@ describe('v6', () => {
15281528
});
15291529
});
15301530

1531+
describe('fromByteArray', () => {
1532+
const zeros = (count: number) => Array(count).fill(0);
1533+
1534+
it('parses a valid 16-byte array', () => {
1535+
should.equal(
1536+
Address6.fromByteArray([32, 1, 13, 184, ...zeros(12)]).correctForm(),
1537+
'2001:db8::',
1538+
);
1539+
should.equal(Address6.fromByteArray([...zeros(15), 1]).correctForm(), '::1');
1540+
should.equal(
1541+
Address6.fromByteArray(Array(16).fill(255)).correctForm(),
1542+
'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff',
1543+
);
1544+
});
1545+
1546+
it('round-trips through toByteArray', () => {
1547+
const topic = new Address6('2001:db8::1');
1548+
1549+
Address6.fromByteArray(topic.toByteArray()).correctForm().should.equal(topic.correctForm());
1550+
});
1551+
1552+
it('folds signed bytes to their unsigned equivalent', () => {
1553+
// An Int8Array or a Java byte[] carries 128 to 255 as -128 to -1.
1554+
should.equal(
1555+
Address6.fromByteArray(Array(16).fill(-1)).correctForm(),
1556+
'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff',
1557+
);
1558+
should.equal(Address6.fromByteArray([-1, ...zeros(15)]).correctForm(), 'ff00::');
1559+
should.equal(Address6.fromByteArray([-128, ...zeros(15)]).correctForm(), '8000::');
1560+
});
1561+
1562+
it('throws unless given exactly 16 bytes', () => {
1563+
// RFC 4291 section 2: an IPv6 address is exactly 16 octets.
1564+
should.Throw(() => Address6.fromByteArray([]), 'IPv6 addresses require exactly 16 bytes');
1565+
should.Throw(
1566+
() => Address6.fromByteArray([1, 2, 3]),
1567+
'IPv6 addresses require exactly 16 bytes',
1568+
);
1569+
// A 17-byte array with a leading zero has the same magnitude as the
1570+
// 16-byte value, so bounds-checking the folded BigInt never catches it.
1571+
should.Throw(
1572+
() => Address6.fromByteArray([...zeros(16), 1]),
1573+
'IPv6 addresses require exactly 16 bytes',
1574+
);
1575+
});
1576+
1577+
it('throws for bytes outside -128 to 255', () => {
1578+
// 300 in the low byte stays under 2**128-1, so a result-only bound misses it.
1579+
should.Throw(
1580+
() => Address6.fromByteArray([...zeros(15), 300]),
1581+
'All bytes must be integers between -128 and 255',
1582+
);
1583+
should.Throw(
1584+
() => Address6.fromByteArray([256, ...zeros(15)]),
1585+
'All bytes must be integers between -128 and 255',
1586+
);
1587+
// -129 would fold to 127 rather than to anything the caller meant.
1588+
should.Throw(
1589+
() => Address6.fromByteArray([-129, ...zeros(15)]),
1590+
'All bytes must be integers between -128 and 255',
1591+
);
1592+
});
1593+
1594+
it('throws for non-integer bytes', () => {
1595+
should.Throw(
1596+
() => Address6.fromByteArray([1.5, ...zeros(15)]),
1597+
'All bytes must be integers between -128 and 255',
1598+
);
1599+
should.Throw(
1600+
() => Address6.fromByteArray([Number.NaN, ...zeros(15)]),
1601+
'All bytes must be integers between -128 and 255',
1602+
);
1603+
});
1604+
});
1605+
1606+
describe('fromUnsignedByteArray', () => {
1607+
const zeros = (count: number) => Array(count).fill(0);
1608+
1609+
it('parses a valid 16-byte array', () => {
1610+
Address6.fromUnsignedByteArray([32, 1, 13, 184, ...zeros(12)])
1611+
.correctForm()
1612+
.should.equal('2001:db8::');
1613+
});
1614+
1615+
it('round-trips through toUnsignedByteArray', () => {
1616+
const topic = new Address6('ffff::1');
1617+
1618+
Address6.fromUnsignedByteArray(topic.toUnsignedByteArray())
1619+
.correctForm()
1620+
.should.equal(topic.correctForm());
1621+
});
1622+
1623+
it('throws unless given exactly 16 bytes', () => {
1624+
should.Throw(
1625+
() => Address6.fromUnsignedByteArray([]),
1626+
'IPv6 addresses require exactly 16 bytes',
1627+
);
1628+
should.Throw(
1629+
() => Address6.fromUnsignedByteArray([1, 2, 3]),
1630+
'IPv6 addresses require exactly 16 bytes',
1631+
);
1632+
should.Throw(
1633+
() => Address6.fromUnsignedByteArray([...zeros(16), 1]),
1634+
'IPv6 addresses require exactly 16 bytes',
1635+
);
1636+
});
1637+
1638+
it('throws for bytes outside 0 to 255, signed values included', () => {
1639+
should.Throw(
1640+
() => Address6.fromUnsignedByteArray([...zeros(15), 300]),
1641+
'All bytes must be integers between 0 and 255',
1642+
);
1643+
should.Throw(
1644+
() => Address6.fromUnsignedByteArray([-1, ...zeros(15)]),
1645+
'All bytes must be integers between 0 and 255',
1646+
);
1647+
});
1648+
1649+
it('throws for non-integer bytes', () => {
1650+
should.Throw(
1651+
() => Address6.fromUnsignedByteArray([1.5, ...zeros(15)]),
1652+
'All bytes must be integers between 0 and 255',
1653+
);
1654+
});
1655+
});
1656+
15311657
describe('an address with more than one subnet suffix', () => {
15321658
// RE_SUBNET_STRING anchors on the end of the address, so it strips only
15331659
// the trailing suffix. Anything left over is malformed and must be

0 commit comments

Comments
 (0)