Different keys can be used to decrypt the same message

[kv109]

See https://gist.github.com/kv109/42289aa65f81e819910005f4773215a1

[Zeragamba]

@kv109 and your write up on Medium (https://blog.elpassion.com/simple-and-terrifying-encryption-story-c1f1d6707c07#.py588ygyh)

[StephenWeber]

Saw your post on Medium on the issue, and I'm of two minds.

First off, the documented use of the library is to use the keys that the library generates for you, as shown in the README:

# Generate a random key
key = AES.key
 => "290c3c5d812a4ba7ce33adf09598a462"

# Encrypt a string.  Default output is base_64 encoded, init_vector and cipher_text are joined with "$"
b64 = AES.encrypt("A super secret message", key)
 => "IJjbgbv/OvPIAf4R5qAWyg==$fy0v7JwRX4kyAWflgouQlt9XGmiDKvbQMRHmQ+vy1fA="

Your post indicates you were setting up some repeatable testing, perhaps you could generate a valid key like above and place it in your test code.

Second, 0x00 is a valid key, albeit a useless one. Truncating non-hex characters to zero seems like a valid design decision, albeit something the user should be made aware of. The documentation is thin on anything outside the expected use, so perhaps that could be expanded. As this is a security package, being explicit about the behavior of the methods would be a useful part of the library.

If you want the behavior of the library to change, what behavior would you prefer to see?

[antstorm]

@StephenWeber I think that it would make sense to at least raise an error if the key contains any non-HEX symbols, which would be the simple fix.

I guess the expected behaviour is instead of assuming the key is a HEX number (since it's a String and can be anything), assume it's a string and get the byte-code for each of the characters instead.

Happy to help with implementing this.

[chicks]

Hey gang!

This is an AMAZING catch, and frankly a bit embarrassing! This gem has a great namespace, and it deserves a more active maintainer - my ruby coding days are few and far between.

Does anyone want to take this over? I'm happy to pass along responsibility in whatever way makes sense.

Loved the writeup on medium, btw!

[drewblas]

I have a PR incoming that will properly error out if given an invalid input for the key (a string that is not a valid hex representation)

[drewblas]

I should mention though that even a cursory security audit reveals several other major problems:

• The keys being generated are only HALF as long as they need to be. They should be 32 bytes, which is 64 hex characters. But the hex is being unpacked and then trimmed to 32 characters (16 bytes). So all keys generated by the AES.key method have the second half all 0x00.

• The _random_seed method has a fallback if it can't use OpenSSL::Random to use a cryptographically insecure method of generating bytes.

Should I move to try and fix them all?

[stouset]

@chicks With all due respect, this gem does not need a new maintainer. It needs to be retired. There are plenty of good cryptographic libraries out there with good Ruby implementations written by people with a solid background in crypto (full disclosure: the author, Tony, is a former coworker of mine on the Information Security team at Square).

Nobody should be using this gem in its current state. Any changes that would bring it up to modern standards would inevitably break the existing API heavily.

In my opinion, a 1.0 release of this gem should be rolled out that removes all preexisting modules and methods, warns anyone installing it that the gem is no longer recommended, and points to a suitable replacement (e.g., cryptosphere/rbnacl). If someone is currently relying on it, they can pin to the previous version using a Gemfile.

[chicks]

Totally agree - I was more thinking if there's a better gem that wants the "aes" name we can probably work that out.

[Zeragamba]

a good options would be point them to the official ruby implementations : https://ruby-doc.org/stdlib-2.0.0/libdoc/openssl/rdoc/OpenSSL/Cipher.html

[mtrpcic]

Or, release a new version of this gem that is just a wrapper around the built-in OpenSSL methods. This way anybody using this library can update and not need to worry about refactoring code. Then this repository can be marked as deprecated, and deprecation warnings can be added to the wrapper code.

[chicks]

@mtrpcic this was the original intent - the AES encryption is delegated to OpenSSL and the gem provides a nice wrapper to make it easier to use. Most everything comes down to: https://github.com/chicks/aes/blob/master/lib/aes/aes.rb#L151

It's also worth noting that the other goal was an AES lib that didn't require a lot of setup or dependent libraries. FasterAES was the standard at the time, but it required the compilation of ruby extensions which turned a 5 minute gem install into a much longer activity as everyone scrambled to compile and package the dependencies properly. It's not as performant, but the heavy lifting is still handled by OpenSSL.

That being said, the way I wrote the key handling routines wasn't vetted thoroughly enough and is severely damaging the credibility of the whole approach :)