new BN(null).toString() causes an infinite loop (cashes the browser)

[adipascu]

new BN(null).toString() causes an infinite loop (cashes the browser)

It looks like

• new BN(null) creates an empty BN
• .toString() causes the VM interpreter to go into a loop

I think the solution is to make .toString() crash on empty instances.
A better fix could be to no longer accept new BN(null), remove empty instance feature or make it accessible with a different api (0 param constructor or something else).

[begelundmuller]

Also bumped into this. Running

> (new BN(null)).imuln(0).toString(10)

Crashes Node with

#
# Fatal error in , line 0
# API fatal error handler returned after process out of memory
#
Illegal instruction: 4

[ahpaleus]

Could you please resolve this issue and patch the codebase?

We figured out that it is possible to trigger this bug by passing the following arguments:

new BN(" ").toString()
new BN("-").toString()
new BN(null).toString()
new BN(undefined).toString()

As the bn.js has nearly 37 million weekly downloads and is used by nearly 3k projects, this bug (leading to Denial of Service) can be impactful.

[disconnect3d]

cc: @indutny

Below is what we diagnosed with @ahpaleus. The crash occurs due to infinite loop which allocates memory on each loop iteration and so it exhausts the NodeJS or web browser's tab memory.

Related sources:

• BN constructor:
  

  bn.js/lib/bn.js

  Lines 21 to 41 in db57519

  	function BN (number, base, endian) {
  	if (BN.isBN(number)) {
  	return number;
  	}
  	this.negative = 0;
  	this.words = null;
  	this.length = 0;
  	// Reduction context
  	this.red = null;
  	if (number !== null) {
  	if (base === 'le' || base === 'be') {
  	endian = base;
  	base = 10;
  	}
  	this._init(number || 0, base || 10, endian || 'be');
  	}
  	}

• _init function:
  

  bn.js/lib/bn.js

  Lines 80 to 111 in db57519

  	BN.prototype._init = function init (number, base, endian) {
  	if (typeof number === 'number') {
  	return this._initNumber(number, base, endian);
  	}
  	if (typeof number === 'object') {
  	return this._initArray(number, base, endian);
  	}
  	if (base === 'hex') {
  	base = 16;
  	}
  	assert(base === (base | 0) && base >= 2 && base <= 36);
  	number = number.toString().replace(/\s+/g, '');
  	var start = 0;
  	if (number[0] === '-') {
  	start++;
  	this.negative = 1;
  	}
  	if (start < number.length) {
  	if (base === 16) {
  	this._parseHex(number, start, endian);
  	} else {
  	this._parseBase(number, base, start);
  	if (endian === 'le') {
  	this._initArray(this.toArray(), base, endian);
  	}
  	}
  	}
  	};

Cases:

• When " " is passed, the _init ends up using an empty string due to number = number.toString().replace(/\s+/g, ''); line and so later neither of the if (number[0] === '-') or the if (start < number.length) conditions trigger
• When "-" is passed, in the _init the if (number[0] === '-') check passes, so we end up having start=1 but number.length=1so that we still do not parse any numbers from string as theif (start < number.length)` does not pass (and well, there are no numbers to parse)
• When null or undefined are passed, I believe the _init is never called as it is called only when if (number !== null) condition passes

All those cases result in having a BN instance where BN.length is 0 and so in the .toString() we loop infinitely in:

bn.js/lib/bn.js

Lines 514 to 532 in db57519

	while (!c.isZero()) {
	var r = c.modrn(groupBase).toString(base);
	c = c.idivn(groupBase);
	if (!c.isZero()) {
	out = zeros[groupSize - r.length] + r + out;
	} else {
	out = r + out;
	}
	}
	if (this.isZero()) {
	out = '0' + out;
	}
	while (out.length % padding !== 0) {
	out = '0' + out;
	}
	if (this.negative !== 0) {
	out = '-' + out;
	}

This is because the isZero performs the length check of 1:

bn.js/lib/bn.js

Lines 2834 to 2836 in db57519

	BN.prototype.isZero = function isZero () {
	return this.length === 1 && this.words[0] === 0;
	};

Also FWIW when "" (empty string) is passed, the number || 0 expression in the this._init(number || 0, base || 10, endian || 'be'); line makes it so that a number 0 is passed to the _init and so an empty string returns a big num value of 0.

All those could be fixed by additional assertions in the BN constructor. That could be e.g. this.length !== 0. You may also want to cover the case of empty string, but since it works and return 0, maybe its good to not change this behavior.