Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Conversation

KiNgMaR
Copy link

@KiNgMaR KiNgMaR commented Oct 7, 2013

This adds support for the authenticated encryption mode GCM to node-crypto.

Before: using aes-128-gcm, aes-192-gcm aes-256-gcm with createCipheriv would work, however decryption with createDecipheriv would always throw in final.

With patch: aforementioned modes are now supported. Users have to use buf = getAuthTag() after Final, transmit the tag with the ciphertext, then use setAuthTag(buf) when decrypting. If the data has been tampered with or if no tag is provided during decryption, final will throw.

The exception thrown from final is currently generic. I can make it more verbose/clear if wanted.

Limitations: it's not possible to add any AAD (additional authenticated data) into the mix (or verify it). This could be achieved by adding another method to Cipheriv, e.g. setAdditionalAuthenticatedData(buf).

Other AEAD modes: The AES-CCM mode cannot be used with the current API because it does not support streaming (the total length of the plain / cipher text has to be known when the first update call is made). Unless limiting users to 1 call of update was ok, of course.
Composite ciphers such as AES-128-CBC-HMAC-SHA1 use a different OpenSSL API, which is rather poorly documented (and only used in OpenSSL's TLS implementation as far as I can tell), so supporting these is not currently in scope of this pull request either.

Thanks for considering.

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit KiNgMaR/node-1@bd44373656a78399f9461b0ac5624bb9a1bc1835 has the following error(s):

  • First line of commit message must be no longer than 50 characters
  • Commit message must indicate the subsystem this commit changes

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@KiNgMaR KiNgMaR closed this Oct 7, 2013
@KiNgMaR KiNgMaR reopened this Oct 7, 2013
@KiNgMaR KiNgMaR closed this Oct 7, 2013
@KiNgMaR KiNgMaR reopened this Oct 7, 2013
@rlidwka
Copy link

rlidwka commented Oct 7, 2013

Unless limiting users to 1 call of update was ok, of course.

It's ok if it's documented and second update() call throws with a good error message. It's odd though that seemingly stream cipher don't support streaming.

can't say anything about the rest of PR due to lack of knowledge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocate the buffer first and copy the tag data directly into it? Saves an allocation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that Buffer uses malloc, so to make valgrind happy you'll want to free().

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, wait. Use:

Local<Object> buf = Buffer::Use(env, out, out_len);
args.GetReturnValue().Set(buf);

Unless there's some reason you can't hand over control of the memory to Buffer.

@bnoordhuis
Copy link
Member

Calling @indutny.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a lint rule put in for this. split it to two lines.

@KiNgMaR
Copy link
Author

KiNgMaR commented Oct 17, 2013

Hey everyone, I changed the things you commented on - except for the int -> unsigned int thing. This way, it's consistent with Update and Final.

Will add documentation update in the next few days.

@RomanMinkin
Copy link

👍 for this issue.

@esco
Copy link

esco commented Nov 19, 2013

👍

@ghost ghost assigned indutny Nov 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to return here. No logic following the if(). Also, remove the brackets. It's a one liner and you're not using them above.

@wamoyo
Copy link

wamoyo commented Nov 19, 2013

👍 for this issue.

@KiNgMaR
Copy link
Author

KiNgMaR commented Nov 19, 2013

Rebased, updated, tested -- let me know if there's something else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final method.

@indutny
Copy link
Member

indutny commented Nov 21, 2013

Small style nits, otherwise LGTM.

@KiNgMaR
Copy link
Author

KiNgMaR commented Dec 7, 2013

Sorry about the delay, nits fixed up :-)

This adds two new member functions getAuthTag and setAuthTag that
are useful for AES-GCM encryption modes. Use getAuthTag after
Cipheriv.final, transmit the tag along with the data and use
Decipheriv.setAuthTag to have the encrypted data verified.
@indutny
Copy link
Member

indutny commented Dec 7, 2013

Thank you, landed in e0d31ea

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants