Otplib before 12.0.0 defaults to 15 bytes of entropy for OTP secret key generation which is too low

[disconnect3d]

I have described this issue already in #671 (comment) but I think it should have a separate bug report here.

The old versions of otplib, before 12.0.0, have a bug where in practice they default to 15 and not 20 bytes of entropy as the default argument of the generateSecret(length) function may suggest. Generally, the function lowers down/truncate the number of entropy bytes that are provided to it.

The generateSecret function uses a secretKey(len, ...) function for key generation:

otplib/packages/otplib-authenticator/Authenticator.js

Lines 77 to 91 in 5ffa22a

	/**
	* Generates and encodes a secret key
	*
	* @param {number} length - secret key length (not encoded key length)
	* @return {string}
	* @see {@link module:impl/authenticator/secretKey}
	* @see {@link module:impl/authenticator/encodeKey}
	*/
	generateSecret(len = 20) {
	if (!len) {
	return '';
	}
	const secret = secretKey(len, this.options);
	return encodeKey(secret);
	}

Which can be found here:

otplib/packages/otplib-utils/secretKey.js

Lines 10 to 23 in dd7dc73

	function secretKey(length, options = {}) {
	if (!length || length < 1) {
	return '';
	}
	if (!options.crypto || typeof options.crypto.randomBytes !== 'function') {
	throw new Error('Expecting options.crypto to have a randomBytes function');
	}
	return options.crypto
	.randomBytes(length)
	.toString('base64') // convert format
	.slice(0, length); // return required number of characters
	}

And which gets the length number of CSPRNG bytes and then encode them to base64 and slice the result to the length bytes. This slicing lowers down the number of entropy bytes that one provides to the function. We can see how this works for the default length of 20 bytes in a node REPL session:

> entropy = crypto.randomBytes(20)
<Buffer 45 07 a2 bc 59 e7 cc 29 be 06 0b 16 d1 b8 bb 8e 5c ed a7 92>
> encoded = entropy.toString('base64')
'RQeivFnnzCm+BgsW0bi7jlztp5I='
> encoded.length
28
> sliced = encoded.slice(0, 20)
'RQeivFnnzCm+BgsW0bi7'
> sliced.length
20
> buffer = Buffer.from(sliced, 'base64')
<Buffer 45 07 a2 bc 59 e7 cc 29 be 06 0b 16 d1 b8 bb>
> buffer.length
15

So this gives only 15 bytes of entropy which is 120 bits which is below the recommended value from the RFC 4226 section 4:

R6 - The algorithm MUST use a strong shared secret. The length of
the shared secret MUST be at least 128 bits. This document
RECOMMENDs a shared secret length of 160 bits.

The new versions of the library (12.0.0, 12.0.1) does not perform that .slice(0, length) operation when they randomize the buffer. This can be seen here, here and here (fwiw I don't know why there are 3 functions createRandomBytes).

However, I am writing this up here as this should get a proper attention and either a fix for older versions should be released, or a security advisory should be released so that projects that depend on otplib would update its version to a safer one.

Additionally, please note that the newer versions: 12.0.0 and 12.0.1 also have another issue with the generateSecret function where they changed the default length argument value to 10. So calling just generateSecret() is even less secure in those versions than in the older ones. This is reported separately in #671 (comment)