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

Document usage of encryptAES / decryptAES #4407

Closed
benmccann opened this issue May 8, 2015 · 29 comments
Closed

Document usage of encryptAES / decryptAES #4407

benmccann opened this issue May 8, 2015 · 29 comments

Comments

@benmccann
Copy link
Contributor

@benmccann benmccann commented May 8, 2015

There's nowhere in our docs where we give an example of encrypting/decrypting data. I think it could be helpful to share an example of when and how we might deal with encrypted data.

Also, something else to mention in the migration guide potentially is that in 2.4 when the encryption algorithm changed, it seems to have changed what characters can appear in the encrypted string. I ran into an issue where we assumed encrypted data would be alphanumeric, which appears to be a true of AES. In Play 2.4 when we changed to AES/CTR/NoPadding we see new characters like -, =, and +.

@benmccann benmccann added this to the 2.4.0 milestone May 8, 2015
@richdougherty
Copy link
Member

@richdougherty richdougherty commented May 11, 2015

Sounds like a good idea.

A few quick notes:

  • The old format was a simple Base64 encoding. Base64 is a-z, A-Z, 0-9 and + and /. The = character is used for padding. I'm not sure why you're not seeing + and = characters; maybe it's the data you're using?
  • The new format is formatId + - + payload.

We can detect a new format because it always includes a - character. The old format never includes it, because Base64 doesn't use -. If it's in the new format then we read the formatId by splitting on the first - and then use the formatId to work out the payload structure.

  • If the formatId is 1 then the encryption is AES without an IV. The payload is just the cipherText.
  • If the formatId is 2 then the encryption is AES with an IV. The payload is IV + cipherText.

You may want to refer to the original PR: #3595.

cc @tikurahul

@richdougherty
Copy link
Member

@richdougherty richdougherty commented May 11, 2015

  • The old format was a simple Base64 encoding. Base64 is a-z, A-Z, 0-9 and + and /. The = character is used for padding. I'm not sure why you're not seeing + and = characters; maybe it's the data you're using?

Sorry, the old format was a hex string not Base64 encoded, which explains why you're seeing new characters.

richdougherty added a commit to richdougherty/playframework that referenced this issue May 11, 2015
Add more information to the 2.4 migration docs, especially some
extra information about how the format has changed.

See playframework#4407.
richdougherty added a commit to richdougherty/playframework that referenced this issue May 11, 2015
Add more information to the 2.4 migration docs, especially some
extra information about how the format has changed.

See playframework#4407.
@richdougherty
Copy link
Member

@richdougherty richdougherty commented May 12, 2015

@benmccann, now that #4438 is merged, do you think we could remove the 2.4.0 tag from this issue? I agree that it would be good to get the docs done but I don't think they need to be there for 2.4.

@benmccann benmccann removed this from the 2.4.0 milestone May 12, 2015
@richdougherty
Copy link
Member

@richdougherty richdougherty commented May 28, 2015

The crypto section of the Play 2.4 migration docs are a good starting point for anyone who wants to work on this: https://www.playframework.com/documentation/2.4.x/Migration24#Crypto-APIs. You can find these docs in Migration24.md within the Play source code.

@v6ak
Copy link
Contributor

@v6ak v6ak commented May 29, 2015

The JavaDoc and ScalaDoc does not describe the current state. While it mentions AES/CTR/NoPadding, it describes ECB instead: “This algorithm is suitable for small amounts of data, typically less than 32 bytes, hence is useful for encrypting credit card numbers, passwords etc. For larger blocks of data, this algorithm may expose patterns and be vulnerable to repeat attacks. ”

This is present four times in the documentation: see classes play.api.libs.Crypto and play.libs.Crypto and their methods encryptAES(value: String, privateKey: String) and encryptAES(value: String), you get four

Moreover, I see two other issues.

  1. It is unclear how are IVs handled. Note that correct IVs are essential for security of stream ciphers if you want to reuse the key. (CTR mode transforms a block cipher to a stream cipher.)
  2. It does not describe another flaw: malleability. While it does not promise the cipher to be non-malleable, it would be better to mention the malleability explicitly, as stream ciphers are practically very malleable. It is often wise to also authenticate the encrypted string.

Both of them are BTW a place where ECB is actually more secure than CTR. I am not saying that Play! should have stayed with ECB, I am just saying CTR is not strictly more secure than ECB in all cases.

@benmccann
Copy link
Contributor Author

@benmccann benmccann commented Jun 2, 2015

@richdougherty
Copy link
Member

@richdougherty richdougherty commented Jun 2, 2015

@v6ak, all good points. It sounds like you've looked at our docs so far, which we know have some shortcomings. It would be great if you could review our implementation too, to let us know if there any issues there.

@v6ak
Copy link
Contributor

@v6ak v6ak commented Jun 4, 2015

@richdougherty I've looked at the implementation briefly. Some my notes about undocumented properties (e.g. the key derivation) do come from there. The implementation mostly transfers the responsibility to JCA, so the code is rather tiny and there is just a little what can be done wrong (provided that the JCA implementation is correct).

This, however, applies only for the implementation, not for the design. The design and uncertain purpose of the API was main objection.

Note that I welcome the attempt to simplify the rather complex JCA API, this can encourage more using cryptography, which is good. If such wrapper is done properly, it can encourage some best practices and using crypto right, which would be great. I just don't think that Play! Crypto can achieve these goals today.

Note that I haven't reviewed the IV generation so far.

@richdougherty
Copy link
Member

@richdougherty richdougherty commented Jun 18, 2015

@v6ak, I agree with all you say. We'd welcome help from you or anyone else who is interested.

Maybe we should just wrap something like Keyczar or Jasypt. I haven't used either, so I'm not sure if this a good idea.

One good thing about the new crypto code in Play 2.4 is that we have now have a versioned format, so we can evolve the implementation as needed. Of course, having multiple formats does introduce complexity; we need to make sure we don't accidentally introduce vulnerabilites through our format system!

@wsargent
Copy link
Member

@wsargent wsargent commented Jun 25, 2015

@wsargent
Copy link
Member

@wsargent wsargent commented Jun 25, 2015

@richdougherty
Copy link
Member

@richdougherty richdougherty commented Jun 25, 2015

@v6ak, If you have any thoughts on @wsargent's post (mentioned above), let us know!

@v6ak
Copy link
Contributor

@v6ak v6ak commented Jun 26, 2015

I hope I'll be able to comment it on Monday afternoon (or on Sunday if it goes well). // UTC+2

@v6ak
Copy link
Contributor

@v6ak v6ak commented Jun 30, 2015

I wanted to post it to the Google Group, but I was unable to. My join request has not been accepted so far. So I've created a gist: https://gist.github.com/v6ak/2f7f3c4f18cfe4d1be9e

@richdougherty
Copy link
Member

@richdougherty richdougherty commented Jun 30, 2015

@v6ak, thanks a lot for your comments. I've asked @jroper if he can check for your play-framework-dev join request. I'll wait to respond to your comments until you've posted them to the mailing list, because that will make it easier for the community to follow the conversation.

@wsargent
Copy link
Member

@wsargent wsargent commented Jul 1, 2015

@v6ak
Copy link
Contributor

@v6ak v6ak commented Jul 1, 2015

@richdougherty I got the access and posted it.

@wsargent I TL;DR-ed it. Is the only merit of the article the fact that hardcoding encryption algorithm is a bad practice? I agree. At the moment, it is a minor issue, but when the API is redesigned, the encryptAES and decryptAES methods may be just deprecated aliases of new methods.

@wsargent
Copy link
Member

@wsargent wsargent commented Jul 7, 2015

@v6ak more about the problems involved in dealing with crypto primitives, but yes, that's part of it.

@wsargent
Copy link
Member

@wsargent wsargent commented Jul 7, 2015

I've asked the KeyCzar team about their status and future plans on keyczar-discuss: https://groups.google.com/forum/#!topic/keyczar-discuss/wG7mnRU4F5Q

ClaraAllende pushed a commit to ClaraAllende/playframework that referenced this issue Aug 28, 2015
Add more information to the 2.4 migration docs, especially some
extra information about how the format has changed.

See playframework#4407.
@wsargent
Copy link
Member

@wsargent wsargent commented Sep 16, 2015

@wsargent
Copy link
Member

@wsargent wsargent commented Oct 16, 2015

After much discussion and research, here's the short version:

  1. Play doesn't use "encryptAES" anywhere in the internal codebase.
  2. Cryptography is hard.
  3. It's unclear who the intended audience for Crypto.encryptAES is.

Ultimately, it doesn't make sense for Play to make these methods available. So, we're going to deprecate the encryptAES/decryptAES methods, and remove them in later versions.

@v6ak
Copy link
Contributor

@v6ak v6ak commented Oct 16, 2015

OK, deprecating these methods is a correct resolution of this issue. (Maybe not the only correct one, but hopefully the most reasonable, as they are not the core of Play!.) What we might also do:

  • Add some explanation to the deprecation note. Maybe we should add just a link to explanation. Maybe just a link to https://groups.google.com/forum/#!topic/play-framework/Pao8MnADAqw or some excerpt (we might add a link to Keyczar) would be enough.
  • Wait until someone complains about the deprecation and explains the use case. (Then some similar API might reappear.)
@wsargent
Copy link
Member

@wsargent wsargent commented Oct 16, 2015

I'm going to put together a blog post and we'll write up the change and background in the migration notes.

@akkie
Copy link
Contributor

@akkie akkie commented Jan 13, 2016

Will the methods be deprecated in Play 2.5 or in Play 3? I ask because Silhouette uses this methods to encrypt authenticator related data.

@benmccann
Copy link
Contributor Author

@benmccann benmccann commented Jan 13, 2016

I feel like the deprecation here is a bit of a cop out. Just saying crypto is hard and we don't want to be responsible for it doesn't help users of Play very much. It'd be nicer to investigate the issues more thoroughly and make more concrete recommendations.

@wsargent
Copy link
Member

@wsargent wsargent commented Jan 13, 2016

@benmccann there are be recommendations and a detailed description in the document.
@akkie They'll be deprecated in 2.5.

@wsargent
Copy link
Member

@wsargent wsargent commented Feb 29, 2016

Added concrete recommendations and issues with current configuration in migration guide: https://www.playframework.com/documentation/2.5.x/CryptoMigration25

Commit is 1f22381

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.