search

fernet / fernet-rb

Delicious HMAC Digest(if) authentication and AES-128-CBC encryption
MIT License
90 stars 22 forks source link

Encrypting an empty string is not allowed #32

Closed brandur closed 9 years ago

brandur commented 9 years ago

Contrary to the signature of Fernet.generate, encrypting an empty string is not actually allowed:

> Fernet.generate('x' * 32, '')
ArgumentError: data must not be empty
from /Users/brandur/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/fernet-2.1/lib/fernet/encryption.rb:29:in `update'

> Fernet.generate('x' * 32)
ArgumentError: data must not be empty
from /Users/brandur/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/fernet-2.1/lib/fernet/encryption.rb:29:in `update'

This is problematic because it forces the client to design and implement their own encoding scheme if they want to encrypt an empty value. For what its worth, Legacy Fernet didn't have this issue thanks to the internal JSON encoding.

hgmnz commented 9 years ago

Interesting! I think the behavior we come up with here must be added to the spec and implemented in all implementations.

@tmaher, @kr what do you think this behavior should be? I think Fernet.verifier(secret, Fernet.generate(secret, '')).message == '' should be true, any issue with that?

kr commented 9 years ago

The spec doesn't place any restrictions on the length of the plaintext message. Whatever is not explicitly forbidden is implicitly allowed, so 0 is just as good a length as 1 or 5 or a million. So in my interpretation the spec already defines the correct behavior. (This is how the Go implementation behaves.)

brandur commented 9 years ago

@tmaher, @kr what do you think this behavior should be? I think Fernet.verifier(secret, Fernet.generate(secret, '')).message == '' should be true, any issue with that?

That would actually work for my purposes. That said, from the perspective of a tinfoil hat-wearer, I could see the mere divulgence that your secret message is an empty string being considered an information leak (albeit, a minor one).

brandur commented 9 years ago

Added a patch that seems to get things running in #33.

kr commented 9 years ago

Even if everyone agrees that the spec already covers this case, I'd be ok with adding some words to explicitly state it anyway, for the sake of clarity. And even more ok with adding a test vector for an empty message.

brandur commented 9 years ago

Even if everyone agrees that the spec already covers this case, I'd be ok with adding some words to explicitly state it anyway, for the sake of clarity. And even more ok with adding a test vector for an empty message.

That would be great!

hgmnz commented 9 years ago

@brandur thanks for the patch! Merged and pushed gem 2.1.1

+1 to what @kr said too