Restrict authentication tag length for GCM by default

What is the problem this feature will solve?

When using the node crypto module to decrypt data with Galois Counter Mode (GCM), the expected authentication tag length is not set by default in the Decipher class. As a consequence, code that is is safe for other modes such as CCM or algorithms like ChaChaPoly is not safe.

As an example

function decrypt(key, iv, ciphertext, tag) {
    decipher = createDecipheriv("aes-256-gcm", key, iv)
    decipher.setAuthTag(tag)
    return Buffer.concat([decipher.update(ciphertext), decipher.final()])
}

is insecure, because the correct 4-byte prefix of the actual tag will be accepted, whereas

function decrypt(key, iv, ciphertext, tag) {
    decipher = createDecipheriv("chacha20-poly1305", key, iv)
    decipher.setAuthTag(tag)
    return Buffer.concat([decipher.update(ciphertext), decipher.final()])
}

is safe, because for ChachaPoly, the expected authentication tag length defaults to 16. For CCM, the code will throw, because it requires passing an option:

function decrypt(key, iv, ciphertext, tag) {
    decipher = createDecipheriv("aes-256-ccm", key, iv, {authTagLength:16})
    decipher.setAuthTag(tag)
    return Buffer.concat([decipher.update(ciphertext), decipher.final()])
}

which is supported, but optional for GCM.

As mentioned above, crypto accepts multiple tag length values for GCM, in particular 4 bytes. 4 bytes is brute-forceable, though practical exploitability depends on the context, e.g. if user interaction is required, it won't be practical.

This isn't a security vulnerability with Node as it is documented behaviour and was explicitly decided in https://github.com/nodejs/node/issues/17523. However, this leads to many implementations in the wild being insecure. I did stumble upon this during a code review and did a small survey of open source projects and among 41 projects on GitHub with >100 stars that used the node module for actual decryption (not benchmarking, no CTFs, not test-only), only 7 implementations did fully protect against short authentication tags from being passed to the Decipher object, while 15 implementations would allow arbitrary forgeries in at most 2^32 queries. This query number might be significantly lower depending on the application based on a paper by Ferguson. The other implementations allow some form of breach of cryptographic primitives that isn't interesting in practice, i.e. brute-forcing the tag for the empty message.

What is the feature you are proposing to solve the problem?

Given this wide-spread misuse of the API (answering @tniessen s comment/question on the original issue whether people will use the API correctly in the negative) paired with NIST's recent announcement to revise SP 800-38D to disallow authentication tags of size less than 96 bits and the fact that all other authenticated modes in Node crypto either default to a reasonable expected tag size (ChachaPoly) or enforce that the expected tag size be specified (OCB, CCM), it might be reasonable to re-assess the decision from https://github.com/nodejs/node/issues/17523 to keep the expected tag length optional.

The feature I would propose would be the Chacha-way, defaulting to an expected tag length of the same size as is generated by the Cipher object, i.e. 16 bytes. This way, basically none of the applications using GCM would be affected (I encoutered none that created tags of size less than 16) in a breaking way, while fixing the security vulernability for them. This would effectively turn the API into secure-by-default, while still permitting users with other tag length needs, to specify this. Note, that this is still a breaking change.

What alternatives have you considered?

The fix could also be done in the CCM/OCB way, i.e. throwing if no expected length is passed. That would break all existing insecure implementations and even some of the secure ones that have custom length checks on the tag.
Warn on any use of a Decipher object with a GCM mode and no specified authTagLength option. While that is noisy, it still doesn't solve the current insecure-by-default state. However, it is the only improvement that isn't a breaking change, that I can come up with

nodejs / node