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: https://github.com/indutny/bn.js/blob/db57519421f0c47c9f68c05fa6fc12273dcca2c2/lib/bn.js#L21-L41

• _init function: https://github.com/indutny/bn.js/blob/db57519421f0c47c9f68c05fa6fc12273dcca2c2/lib/bn.js#L80-L111

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: https://github.com/indutny/bn.js/blob/db57519421f0c47c9f68c05fa6fc12273dcca2c2/lib/bn.js#L514-L532

This is because the isZero performs the length check of 1: https://github.com/indutny/bn.js/blob/db57519421f0c47c9f68c05fa6fc12273dcca2c2/lib/bn.js#L2834-L2836

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.