Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional validation of tag length in AES GCM decryption #17523

Closed
willclarktech opened this issue Dec 7, 2017 · 13 comments
Closed

Additional validation of tag length in AES GCM decryption #17523

willclarktech opened this issue Dec 7, 2017 · 13 comments
Labels
crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security.

Comments

@willclarktech
Copy link
Contributor

willclarktech commented Dec 7, 2017

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

// 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 bnoordhuis added the crypto Issues and PRs related to the crypto subsystem. label Dec 7, 2017
@bnoordhuis
Copy link
Member

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

Can you post a concrete API proposal?

@willclarktech
Copy link
Contributor Author

@bnoordhuis Updated the description to include an API proposal.

@bnoordhuis
Copy link
Member

cc @nodejs/crypto - your input please.

@tniessen
Copy link
Member

tniessen commented Dec 7, 2017

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

tniessen commented Dec 7, 2017

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

Ref #13941

@bnoordhuis
Copy link
Member

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
Copy link
Member

tniessen commented Dec 8, 2017

@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 added a commit to tniessen/node that referenced this issue Dec 9, 2017
SaltwaterC added a commit to SaltwaterC/envelope-encryption-tools that referenced this issue Dec 15, 2017
tniessen added a commit that referenced this issue Dec 22, 2017
Using authentication tags of invalid length does not conform to NIST
standards.

PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
tniessen added a commit that referenced this issue Dec 22, 2017
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@tniessen
Copy link
Member

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

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
Copy link
Member

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

gibfahn pushed a commit that referenced this issue Jan 24, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Jan 24, 2018
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 10, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 10, 2018
Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 11, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 11, 2018
Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 12, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 12, 2018
Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 13, 2018
Using authentication tags of invalid length does not conform to NIST
standards.

Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 13, 2018
Backport-PR-URL: #18347
PR-URL: #17566
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
tniessen added a commit to tniessen/node that referenced this issue Apr 11, 2018
@tniessen
Copy link
Member

@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
Copy link
Member

No objections, sounds like a good idea.

jasnell pushed a commit that referenced this issue Apr 14, 2018
Refs: #17523

PR-URL: #17825
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 14, 2018
PR-URL: #17825
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 14, 2018
PR-URL: #17825
Refs: #17523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
tniessen added a commit to tniessen/node that referenced this issue Apr 15, 2018
This change allows users to restrict accepted GCM authentication tag
lengths to a single value.

Fixes: nodejs#17523
tniessen added a commit to tniessen/node that referenced this issue May 13, 2018
This change allows users to restrict accepted GCM authentication tag
lengths to a single value.

PR-URL: nodejs#20039
Fixes: nodejs#17523
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit that referenced this issue May 14, 2018
This change allows users to restrict accepted GCM authentication tag
lengths to a single value.

Backport-PR-URL: #20706
PR-URL: #20039
Fixes: #17523
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants