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

attr_encrypted is completely unsafe #32

Closed
danpal opened this issue Dec 19, 2011 · 45 comments

Comments

Projects
None yet
@danpal
Copy link

commented Dec 19, 2011

You are using cbc mode with the same :iv over an over.
see this discussion:
danpal/encryptor@c1b7e39

This gem is quite downloaded(36,520 total downloads). It should be fixed, and users should at least be warned.

@ivanoats

This comment has been minimized.

Copy link

commented Jan 6, 2012

wow @shuber any comments? @danpal gotta patch?

@danpal

This comment has been minimized.

Copy link
Author

commented Jan 6, 2012

I am using attr_encrypted I wrote, it's stable enough for me but it needs some refactoring. It is safe though:
https://github.com/danpal/attr_encrypted

@daemonsy

This comment has been minimized.

Copy link

commented Jan 31, 2012

Sorry, just for the convenience of future users, @danpal renamed the gem.

https://github.com/danpal/attr_encryptor

@mrgcohen

This comment has been minimized.

Copy link

commented Sep 18, 2012

what's the status of this? has it been updated?

@sbfaulkner

This comment has been minimized.

Copy link
Member

commented Sep 18, 2012

having just joined on the "official" team for maintaining this, we'll take a look. I suspect we'll be integrating some or all of dan's work in an upcoming release

cheers

@maletor

This comment has been minimized.

Copy link

commented Sep 20, 2012

Nice @sbfaulkner, looking forward to it.

@dvdplm

This comment has been minimized.

Copy link

commented Jan 24, 2013

@sbfaulkner any updates on this? I'm trying to pick the right library to use when I ran into this and now I have doubts... ;)

@jjasonclark

This comment has been minimized.

Copy link

commented Jan 25, 2013

I'm also looking at this issue for my company.

I think I have a summary of the issue. Please correct me if I'm wrong.

  • The default encryption :algorithm is 'aes-256-cbc'.
  • The examples (and most usage) sets a :key value.
  • Block encryptors (which aes-256-cbc is) need to have an IV (initialization vector) that changes with each write to be secure.

I have not tried this yet, but I think I know a possible solution. Make a proc be your :iv value. This proc could return a value created at the time the encrypted attribute was changed.

Only issue I see left is storing the IV value.

@danpal did create a fork of the gem that added creation of a '_iv' and '_salt' versions of the encrypted attribute. I still haven't finished the complete look at the code. He does add a requirement for the salt value in addition to the IV value. This isn't required to be secure, but I think he includes it because it cannot hurt. I am looking at the code now to see if you can use a default salt value and only store the IV value.

@dvdplm

This comment has been minimized.

Copy link

commented Jan 25, 2013

In my case, the encrypted string is not updated by users. Ever. If I'm reading this right, we should be good without setting the IV (and avoid storing it). @jjasonclark: thanks for the info! :)

@danpal

This comment has been minimized.

Copy link
Author

commented Jan 25, 2013

@dvdplm no you are not, you have to have an IV onc CVC.
@jjasonclark tl;dr but the IV doesn't need to change on each write. You can store the IV on the DB.

Just use this i'll do everything for you: https://github.com/danpal/attr_encryptor

@jjasonclark

This comment has been minimized.

Copy link

commented Jan 25, 2013

@danpal Your gem looks good and might meet our needs. Have you thought about prepending the IV value to the encrypted value instead of having another DB column?

Also, I'm seeing on the web that doing CBC-MAC would require an IV of all 0s. Do you know if this is correct?

Do you have any advise for converting from attr_encrypted to attr_encryptor? I assume we need to do something to the data so we can generate the other 2 values.

@jjasonclark

This comment has been minimized.

Copy link

commented Feb 14, 2013

@maletor

This comment has been minimized.

Copy link

commented Feb 14, 2013

I was hoping for something a little more thorough, but this will have to do.

I'm kidding. Awesome work and thanks.

@jasonmp85

This comment has been minimized.

Copy link

commented May 15, 2013

So what's the status of this pull request? Can this fork be merged back?

@rcook

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

Daniel,

I would like to volunteer to take on the task of merging your work back into attr_encrypted. Do you still have your original fork of attr_encrypted lying around somewhere? This would be helpful for me to reconstruct the missing history between attr_encrypted and your (now-unrelated) attr_encryptor project.

-Richard

@mrgcohen

This comment has been minimized.

Copy link

commented Jul 13, 2013

Go Richard go Richard go.

Tmnt

-Michael

On Jul 12, 2013, at 7:35 PM, Richard Cook notifications@github.com wrote:

Daniel,

I would like to volunteer to take on the task of merging your work back
into attr_encrypted. Do you still have your original fork of attr_encrypted
lying around somewhere? This would be helpful for me to reconstruct the
missing history between attr_encrypted and your (now-unrelated)
attr_encryptor project.

-Richard


Reply to this email directly or view it on
GitHubhttps://github.com//issues/32#issuecomment-20910109
.

@danpal

This comment has been minimized.

Copy link
Author

commented Jul 16, 2013

My Fork is here: https://github.com/danpal/attr_encryptor

I wish there was someone who could take care of this project. There are a number of improvements I wish to do on them. If someone want's to maintain it I would be happy to help them with the Crypto and secure design. It could be much better

@cheynewallace

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2013

Would love if someone could help make attr_encryptor Ruby 2 compatible. Then I could continue using attr_encrypted. Issue has been raised here #60

@rcook

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2013

Cheyne: Please see my comment at #60 about your compatibility issue.

So, this turns out to be complicated by the fact that danpal/attr_encryptor is dependent upon a forked version of encryptor at danpal/encryptor. However, I have a plan. I intend that the end result is a new version of attr_encrypted that supports Daniel's salt/IV functionality as well as backwards compatibility for users of the existing attr_encrypted gem through an API option. I'm hoping that this will also provide a migration path for users with existing databases based on attr_encrypted - this should be an in-built mechanism similar to Jason Clark's migration process.

Here's my road map:

  1. Fork shuber/encryptor and merge in changes from danpal/encryptor.
  2. Modify encryptor's crypt API and upstream callers to use an additional option to allow the caller to switch between the old behaviour and the new behaviour.
  3. Fork shuber/attr_encrypted and merge in changes from danpal/encryptor into a feature branch.
  4. In the feature branch I will massage attr_encryptor's API back to its original attr_encrypted form.
  5. I will modify attr_encryptor's API to use an additional option to allow the caller to switch between the old behaviour and new behaviour.

Until my modifications to shuber/encryptor are merged back to the main repository, the new version of attr_encrypted in the feature branch will be temporarily modified to refer to my version of this gem.

Sounds like fun, eh?!

@cheynewallace

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2013

Glad someone is back on the case! Love this gem, id hate to see it die out.

@jjasonclark

This comment has been minimized.

Copy link

commented Jul 16, 2013

Sounds like a plan. Let us know when your feature branch is testable.

@rcook

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2013

So, at the risk of giving a running commentary, I can report that the merge is going pretty well. It should be done some time over the next week, time permitting. I've already submitted a pull request for the encryptor gem at https://github.com/shuber/encryptor. This small change is required to support attr_encryptor's functionality:

attr-encrypted/encryptor#9

The first hurdle is getting this pull request merged. In the meantime, I'm proceeding with the attr_encrypted merge based on my locally built version of encryptor.

@maletor

This comment has been minimized.

Copy link

commented Jul 18, 2013

Awesome!

On Wednesday, July 17, 2013, Richard Cook wrote:

So, at the risk of giving a running commentary, I can report that the
merge is going pretty well. It should be done some time over the next week,
time permitting. I've already submitted a pull request for the encryptorgem at
https://github.com/shuber/encryptor. This small change is required to
support attr_encryptor's functionality:

attr-encrypted/encryptor#9 attr-encrypted/encryptor#9

The first hurdle is getting this pull request merged. In the meantime, I'm
proceeding with the attr_encrypted merge based on my locally built
version of encryptor.


Reply to this email directly or view it on GitHubhttps://github.com//issues/32#issuecomment-21163460
.

@rcook

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2013

Pull request submitted:

#62

Note that this depends on my pull request to the Encryptor project:

attr-encrypted/encryptor#9

@rcook

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2013

I submitted my pull request approximately three weeks ago and I'm still waiting to see if anything will happen with it.

@brianlucas

This comment has been minimized.

Copy link

commented Aug 13, 2013

I'm also waiting on Richard's integration so I can move away from the danpal fork.

On Aug 12, 2013, at 11:25 PM, Richard Cook notifications@github.com wrote:

I submitted my pull request approximately three weeks ago and I'm still waiting to see if anything will happen with it.


Reply to this email directly or view it on GitHub.

@amacneil

This comment has been minimized.

Copy link

commented Aug 20, 2013

👍

@saghaulor

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2013

@sbfaulkner are you reviewing the pull request from @rcook ? A lot of people are hoping this will be resolved so that we can (continue) to use this gem. Please advise.

@brianlucas

This comment has been minimized.

Copy link

commented Aug 23, 2013

Agreed, any update would be appreciated.

@sbfaulkner

This comment has been minimized.

Copy link
Member

commented Aug 24, 2013

yes, we had some client work to deal with ... thanks for the patience

@saghaulor

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2013

Has there been any movement on this issue?

@sbfaulkner

This comment has been minimized.

Copy link
Member

commented Nov 10, 2013

pretty sure we have merged it, but have not yet pushed a new gem version... @billymonk - am I correct about this?

@billymonk

This comment has been minimized.

Copy link
Member

commented Nov 10, 2013

@sbfaulkner, @saghaulor - It has been merged in. Check out 3f7de29.

@sbfaulkner

This comment has been minimized.

Copy link
Member

commented Nov 11, 2013

thanks for confirming @billymonk

@saghaulor I'll attempt to push an updated gem this week

@saghaulor

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2013

@sbfaulkner @billymonk thanks for the update guys. Really looking forward to these improvements. I hope to see them this week.

@brianlucas

This comment has been minimized.

Copy link

commented Nov 11, 2013

If you want to use it now (which is what I did), you can of course do:

#gemfile

gem 'attr_encrypted', :git=>'https://github.com/attr-encrypted/attr_encrypted.git' # used to encrypt active record models

inside your model.rb

self.attr_encrypted_options[:mode] = :per_attribute_iv_and_salt
attr_encrypted :data,
:key => proc { Encryptor.encrypt(:value => SALT, :key => KEY) },
:marshal => true

On Nov 11, 2013, at 8:33 AM, Stephen Aghaulor notifications@github.com wrote:

@sbfaulkner @billymonk thanks for the update guys. Really looking forward to these improvements. I hope to see them this week.


Reply to this email directly or view it on GitHub.

@sbfaulkner

This comment has been minimized.

Copy link
Member

commented Nov 15, 2013

v1.3.0 of both the encryptor gem and attr_encrypted have been pushed

@sbfaulkner sbfaulkner closed this Nov 15, 2013

@amacneil

This comment has been minimized.

Copy link

commented Nov 15, 2013

Thank you! Glad this has finally been merged. 😄

@jonathan-upstart

This comment has been minimized.

Copy link

commented Jun 11, 2014

Does this fix apply retroactively? As in: if we upgraded attr_encrypted to v1.3.0+, do we now need to do anything to existing data that was previously encrypted using an older version of attr_encrypted to ensure that it is safely encrypted?

@rcook

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2014

@jonathan-upstart: Unfortunately the fix does not apply retroactively since the enhanced functionality requires separate IV and salt columns per encrypted attribute: i.e. you need to add new columns to your database to accommodate it. Furthermore, you'd need to re-encrypt the encrypted values with the new encryption scheme.

@jonathan-upstart

This comment has been minimized.

Copy link

commented Jun 11, 2014

Thanks @rcook! Would you mind pointing me to instructions for how to accommodate these new columns? I don't see anything related to salts on the attr-encrypted github page: https://github.com/attr-encrypted/attr_encrypted. Thank you!

@rcook

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2014

@jonathan-upstart: You're quite right: there is absolutely no documentation for this. You should follow the instructions at https://github.com/danpal/attr_encryptor for the time being. I'll take on the task of merging this documentation into attr_encrypted since this is a significant oversight on my part.

@rcook

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2014

I've opened issue #118 to track this.

@mhuggins

This comment has been minimized.

Copy link

commented Jan 22, 2015

Apologies for my naivety, but is it safe to store the salt value on the same record as the encrypted data, per the sample code that @brianlucas linked to?

create_table :users do |t|
  t.string :email, null: false
  t.string :encrypted_ssn, null: false
  t.string :encrypted_ssn_iv, null: false
  t.string :encrypted_ssn_salt, null: false
  t.string :salt, null: false
  t.timestamps null: false
end

class User < ActiveRecord::Base
  attr_encrypted :ssn,
      mode: :per_attribute_iv_and_salt,
      key: ->(record) { Encryptor.encrypt(value: record.salt, key: KEY) }

  after_initialize :assign_salt

  private

  def assign_salt
    self.salt ||= Digest::SHA256.hexdigest((Time.now.to_i * rand(1000)).to_s)[0..15]
  end
end
@jjasonclark

This comment has been minimized.

Copy link

commented Jan 23, 2015

Yes, it is safe for the salt. It doesn't even need to be secret. https://en.wikipedia.org/wiki/Salt_(cryptography)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.