Cannot parse integers greater than 2147483647

[mvduin]

BerReader fails to parse integers greater than 2147483647, e.g. this Counter32 value from a captured SNMP packet:

let r = new BerReader( Buffer.from('410500b7518b1a', 'hex') );
console.log( r.readInt(0x41) );  // throws InvalidAsn1Error

And yes I've read the comment above node-net-snmp's workaround for this, but it is simply wrong, the 5-byte encoding is in fact the correct encoding. BER integers are always interpreted as signed, and therefore a leading 00 byte must be prepended to non-negative integers whose leading byte would otherwise be in range 80-FF.

[markabrahams]

Thanks @mvduin for that - you're spot on! I've pushed a fix to master and published in version 1.1.3 of the npm.

[mvduin]

I'm personally not a fan of reducing the answer mod 2**32 (to range -2**32+1 .. 2**32-1) .. it doesn't really seem to have any benefit, it's smaller than javascript supports, and it prevents the caller from being able to validate whether the integer was inside the range it expected. My suggestion would be:

diff --git i/lib/ber/reader.js w/lib/ber/reader.js
index 6dcbd48..b1160ca 100644
--- i/lib/ber/reader.js
+++ w/lib/ber/reader.js
@@ -244,27 +244,24 @@ Reader.prototype._readTag = function(tag) {
    if (o === null)
        return null;

-   // Is it better to modulo 2**32 a large integer, throw an error, or just return it?
-   // Went with the modulo 2**32
-   // if (this.length > 5)
-   //  throw InvalidAsn1Error('Integer too long: ' + this.length);
+   if (this.length === 0)
+       throw InvalidAsn1Error('Zero-length integer');

    if (this.length > this._size - o)
        return null;
    this._offset = o;

-   var fb = this._buf[this._offset];
-   var value = 0;
+   var value = this._buf.readInt8(this._offset++);

-   for (var i = 0; i < this.length; i++) {
+   for (var i = 1; i < this.length; i++) {
        value *= 256;
-       value += (this._buf[this._offset++] & 0xff);
+       value += this._buf[this._offset++];
    }

-   if ((fb & 0x80) == 0x80)
-       value -= 2**(i * 8);
+   if ( ! Number.isSafeInteger(value) )
+       throw InvalidAsn1Error('Integer not representable as javascript number');

-   return value % (2**32);
+   return value;
 };

An argument can be made for omitting the Number.isSafeInteger check and leaving it to the caller, but it seems wiser to just throw an error instead of returning a number that isn't what was actually encoded in the data.

Parsing the first byte as signed instead of doing a fix-up after the loop is both simpler and ensures excessively overlong encodings of small negative integers are parsed correctly, which seems prudent if overlong encodings are being accepted. For example, your version would decode ff ff ff ff ff ff ff ff as 0 rather than -1.

[mvduin]

(I'm not saying accepting overlong encodings is a good thing, but it is what this library has always done so far and I do think it should either reject overlong encodings with an InvalidAsn1Error or parse them correctly no matter how severely overlong they are.)

[markabrahams]

Seems legit! Have pushed those changes to master and published in version 1.1.4 of the npm.