Additional validation of tag length in AES GCM decryption

[willclarktech]

I notice here that validation of tag length is planned when using authenticated decryption:

node/src/node_crypto.cc

Line 3713 in 50ec9bf

// FIXME(bnoordhuis) Throw when buffer length is not a valid tag size.

I'm assuming this comment addresses the fact that the current implementation doesn't check whether the provided tag conforms to the valid tag lengths for GCM: 128, 120, 112, 104, or 96 (or 64 or 32 for certain applications) according to this document).

Even with such validation however, if no default minimum tag length is enforced, there is a lot of scope for insecure use of authenticated encryption, where developers authenticate decryption in a manner that accepts shorter tags than necessary (e.g. 32 bits when they only ever intend to use 128-bit tags).

A solution would be to validate tag length against not only the list of valid lengths according to the specification, but also a minimum length (128 bits by default, but configurable by users who have a good reason to allow shorter tags).

For example:

// 16-byte tag originally created using createCipheriv
const validTag = Buffer.from('11112222333344445555666677778888', 'hex');


// Using the full tag (same as now)
const decipher1 = crypto.createDecipheriv('aes-256-gcm', key, iv);
decipher1.setAuthTag(validTag);
decipher1.update(cipherText);
// Does not throw
decipher1.final();


// Using a shortened tag of valid length
const decipher2 = crypto.createDecipheriv('aes-256-gcm', key, iv);
decipher2.setAuthTag(validTag.slice(0, 4));
decipher2.update(cipherText);
// Throws "Unsupported state or unable to authenticate data"
decipher2.final();


// Specifying an invalid minimum tag length
// Throws "Invalid minimum tag bytes value. Must be one of 16, 15, 14, 13, 12, 8 or 4. Is there a good reason to set this option?"
const decipher3 = crypto.createDecipheriv('aes-256-gcm', key, iv, { minimumTagBytes: 11 }); 


// Specifying a valid minimum tag length with a shortened tag of conforming length
const decipher4 = crypto.createDecipheriv('aes-256-gcm', key, iv, { minimumTagBytes: 8 });
decipher4.setAuthTag(validTag.slice(0, 8));
decipher4.update(cipherText);
// Does not throw
decipher4.final();


// Specifying a valid minimum tag length with a shortened tag of non-conforming length
const decipher5 = crypto.createDecipheriv('aes-256-gcm', key, iv, { minimumTagBytes: 8 });
decipher5.setAuthTag(validTag.slice(0, 4));
decipher5.update(cipherText);
// Throws "Unsupported state or unable to authenticate data"
decipher5.final();


// Specifying a minimum tag length at the same time as stream.transformoptions
// { transform: myTransform, flush: myFlush } is passed as options argument to stream transform
const decipher6 = crypto.createDecipheriv('aes-256-gcm', key, iv, {
	minimumTagBytes: 8,
	transform: myTransform,
	flush: myFlush,
});

Caveats with the above proposal:

1. I'm not very familiar with Node's standards regarding where to throw errors (e.g. whether to throw when creating a decipher with an invalid minimumTagBytes value or when calling .final()) or how to word error messages.
2. I have no experience with these stream transform options so I have no idea if it's a plausible suggestion to combine both sets of options in one object. One alternative would be to have an additional options argument after the stream transform options.

[bnoordhuis]

but configurable by users who have a good reason to allow shorter tags

Can you post a concrete API proposal?

[willclarktech]

@bnoordhuis Updated the description to include an API proposal.

[bnoordhuis]

cc @nodejs/crypto - your input please.

[tniessen]

I'll try to clarify your proposal just to make sure I did not miss anything:

1. crypto.createDecipheriv should accept an option minimumTagBytes whose value must be a valid authentication tag length, otherwise the function throws. The option defaults to 128 bits (16 bytes).
2. decipher.setAuthTag(tag) should throw if the length of the tag is either invalid (not one of 128, 120, 112, 104, 96, 64, 32 bits) or smaller than minimumTagBytes.

Even though minimumTagBytes does not necessarily need to be implemented in Node.js core, the security implications of accidentally accepting small tag lengths justify it in my opinion.

If this is correct, I can probably put together a PR on the weekend. There are three relevant changes here:

• setAuthTag(tag) throws if the tag length is invalid. I would go with semver-minor or maybe even semver-patch here.
• crypto.createDecipher accepts a minimumTagBytes option and causes setAuthTag to throw when the tag is too short. This would be semver-minor.
• crypto.createDecipher uses 128 bits as the default value for minimumTagBytes. This will cause existing applications to fail if they depend on the old behavior, so this would be a semver-major change. We could add a runtime deprecation for using createCipher with tags shorter than 128 bits.

[willclarktech]

@tniessen That’s exactly what I meant and those semver assessments seem correct. In my opinion semver-patch would make sense for the first change: I was pretty surprised to discover decipher.setAuthTag(validTag.slice(0, 1)) let me decrypt without complaint!

[willclarktech]

Oh except I’ve only been talking about crypto.createDecipheriv. I suppose it should also apply to crypto.createDecipher even though people shouldn’t really be using it with GCM.

[tniessen]

I suppose it should also apply to crypto.createDecipher even though people shouldn’t really be using it with GCM.

Ref #13941

[bnoordhuis]

setAuthTag(tag) throws if the tag length is invalid. I would go with semver-minor or maybe even semver-patch here.

Right now, any 1 <= tag length <= 16 is accepted (because openssl accepts it) and there might therefore be code in the wild that uses odd sizes.

Throwing an exception must be semver-major for that reason but logging a warning for e.g. lengths < 8 can be semver-patch.

crypto.createDecipher accepts a minimumTagBytes option and causes setAuthTag to throw when the tag is too short

Call me a pessimist but I predict most usage is going to look like this...

var tag = getTagFromSomewhere();
var dec = crypto.createDecipheriv(algo, key, iv, { minimumTagBytes: tag.length });
dec.setAuthTag(tag);

More boilerplate, zero extra security.

We'll probably get better results from improving the documentation and adding big fat warnings that people must check that the tag is what they expect it to be.

[tniessen]

@bnoordhuis If we decide to set the default value of minimumTagBytes to 128 bits as a semver-major change, it might increase security considering that people will need to manually set minimumTagBytes to allow shorter tags, and at that point they are hopefully looking at our docs.

[tniessen]

I'll give a short update on the status of this proposal:

• Beginning in node 9.4.0, a warning will be emitted if an invalid authentication tag length is used (crypto: warn on invalid authentication tag length #17566).
• Beginning in node 10, a deprecation warning will be emitted if an invalid authentication tag length is used (crypto: assign deprecation code for setAuthTag/GCM #18017).
• Beginning in node 11, an error will be thrown if an invalid authentication tag length is used (crypto: throw on invalid authentication tag length #17825).

A minimum tag length has not been implemented and, according to NIST SP 800 38d, would be insufficient as the permitted tag length should be restricted to a "single, fixed value [...] associated with each key".

[tniessen]

@bnoordhuis Could you take a look at #18138? If we decide to land the proposed API, we could use the authTagLength option for GCM as well and throw in setAuthTag() if the length differs. This would make it easy for users to satisfy the NIST specification while still remaining optional (no default value).

[tniessen]

@nodejs/crypto What do you think about #17523 (comment)? The API was landed as proposed, the main question is: Will people use it, and will they do so correctly? We cannot force them to check the auth tag length, but we can help them.

[bnoordhuis]

No objections, sounds like a good idea.