Default secret length should be changed to 20 bytes from 10 bytes

[nflaig]

Currently, the secret generation using the default value of 10 bytes is not compliant with the requirements mentioned in 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.

It clearly states that it MUST be at least 128 bits which means using the default value of this library is non compliant.

I suggest that the default value is at least increased to 16 bytes or better follow the recommended value of 20 bytes (160 bits) as mentioned in RFC 4226.

Related:

• Google Authenticator PAM module issue
• Google Authenticator issue
• Security Stack Exchange post

[disconnect3d]

FWIW this issue or vulnerability was introduced in 12.0.0. Old versions of otplib defaulted to 20 bytes of randomness for OTP secret generation.

See:

• https://github.com/yeojz/otplib/blob/v11.0.1/packages/otplib-authenticator/Authenticator.js#L82-L96

Vs:

• https://github.com/yeojz/otplib/blob/v12.0.0/packages/otplib-core/src/authenticator.ts#L265-L267

An interesting question to ask here is: why was this changed at all?

[D0han]

This changed exactly in b088efe
@yeojz seems that you committed that change, can you shed some light here?

[disconnect3d]

For what is worth, the old version of the lib also has a quirk/bug/vulnerability where in practice it defaults to 15 and not to 20 bytes of entropy.

Let's look how its secret key is generated:

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
	}

So it randomizes the value and then encodes it to base64 and... slices it to the length? That slicing lowers down the number of entropy bytes that one provides to the function. Let's 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 the old version defaults gives only 15 bytes of entropy which is 120 bits which is under the "MUST" value and definetely below the "RECOMMENDED" one.

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).