Repository navigation
Additional validation of tag length in AES GCM decryption #17523
Description
I notice here that validation of tag length is planned when using authenticated decryption:
Line 3713 in 50ec9bf
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:
- 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
minimumTagBytesvalue or when calling.final()) or how to word error messages. - 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.
Activity
bnoordhuis commentedon Dec 7, 2017
Can you post a concrete API proposal?
willclarktech commentedon Dec 7, 2017
@bnoordhuis Updated the description to include an API proposal.
bnoordhuis commentedon Dec 7, 2017
cc @nodejs/crypto - your input please.
tniessen commentedon Dec 7, 2017
I'll try to clarify your proposal just to make sure I did not miss anything:
crypto.createDecipherivshould accept an optionminimumTagByteswhose value must be a valid authentication tag length, otherwise the function throws. The option defaults to 128 bits (16 bytes).decipher.setAuthTag(tag)should throw if the length of thetagis either invalid (not one of 128, 120, 112, 104, 96, 64, 32 bits) or smaller thanminimumTagBytes.Even though
minimumTagBytesdoes 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.createDecipheraccepts aminimumTagBytesoption and causessetAuthTagto throw when the tag is too short. This would be semver-minor.crypto.createDecipheruses 128 bits as the default value forminimumTagBytes. 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 usingcreateCipherwith tags shorter than 128 bits.willclarktech commentedon Dec 7, 2017
@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 commentedon Dec 7, 2017
Oh except I’ve only been talking about
crypto.createDecipheriv. I suppose it should also apply tocrypto.createDeciphereven though people shouldn’t really be using it with GCM.tniessen commentedon Dec 7, 2017
Ref #13941
bnoordhuis commentedon Dec 7, 2017
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.
Call me a pessimist but I predict most usage is going to look like this...
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 commentedon Dec 8, 2017
@bnoordhuis If we decide to set the default value of
minimumTagBytesto 128 bits as a semver-major change, it might increase security considering that people will need to manually setminimumTagBytesto allow shorter tags, and at that point they are hopefully looking at our docs.crypto: warn on invalid authentication tag length
Mitigate nodejs/node#17523.
test: use valid authentication tag length
crypto: warn on invalid authentication tag length
48 remaining items
crypto: allow to restrict valid GCM tag length
doc: warn about short GCM tags visibly
crypto: move DEP0182 to End-of-Life
crypto: move DEP0182 to End-of-Life
doc: warn about short GCM tags visibly
doc: warn about short GCM tags visibly
doc: warn about short GCM tags visibly
doc: warn about short GCM tags visibly
crypto: allow to restrict valid GCM tag length