markabrahams / node-asn1-ber

Generate and parse ASN.1 BER objects
7 stars 8 forks source link

Integers greater than 2147483647 are encoded incorrectly #21

Closed mvduin closed 2 years ago

mvduin commented 2 years ago

The writeInt instance method of BerWriter erroneously encodes integers in range 2147483648 (0x80000000) to 4294967295 (0xFFFFFFFF) as if they were in range -2147483648 to -1 instead:

let w = new BerWriter;
w.writeInt( 2147483647 );
w.writeInt( 2147483648 );
w.writeInt( 4294967295 );
let r = new BerReader( w.buffer );
console.log( r.readInt() );  // prints 2147483647
console.log( r.readInt() );  // prints -2147483648
console.log( r.readInt() );  // prints -1

The correct encoding for integers in this range uses 5 bytes (with the first byte being 00).

markabrahams commented 2 years ago

Hi @mvduin - the fix for this is in master now and published in version 1.1.3 of the npm.

mvduin commented 2 years ago

Here's a much much simpler version of writeInt:

Writer.prototype.writeInt = function(i, tag) {
    if (!Number.isInteger(i))
        throw new TypeError('argument must be an integer');
    if (typeof(tag) !== 'number')
        tag = ASN1.Integer;

    let bytes = [];
    while (i < -0x80 || i >= 0x80) {
        bytes.push(i & 0xff);
        i = Math.floor(i / 0x100);
    }
    bytes.push(i & 0xff);

    this._ensure(2 + bytes.length);
    this._buf[this._offset++] = tag;
    this._buf[this._offset++] = bytes.length;

    while (bytes.length)
        this._buf[this._offset++] = bytes.pop();
};

An argument could be made for strengthening the Number.isInteger(i) check to Number.isSafeInteger(i) but there's no strict need, the method will in fact correctly encode any integer value that's representable as a javascript number, e.g. it will correctly pass:

let writer = new BerWriter();
writer.writeInt(-6641203978126788000);
assert.deepStrictEqual(writer.buffer, Buffer.from('0208a3d5b42b22d77c00', 'hex'));

The most important reason to not merely check the typeof is because ±Infinity would cause an infinite loop until node runs out of memory.

markabrahams commented 2 years ago

Yep - that's better! Have replaced writeInt() with this now in master and published in version 1.2.2 of the npm.