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

fix error with long keys in ActiveSupport::Cache::FileStore #15616

Merged
merged 1 commit into from Jun 10, 2014
Merged

fix error with long keys in ActiveSupport::Cache::FileStore #15616

merged 1 commit into from Jun 10, 2014

Conversation

@apanzerj
Copy link
Contributor

@apanzerj apanzerj commented Jun 10, 2014

@grosser @rafaelfranca

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 10, 2014

Would not this give problems with cache key clashing?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 10, 2014

Related with #10894

@grosser
Copy link
Contributor

@grosser grosser commented Jun 10, 2014

how can the md5 not be unique ?
same approach is used in memcached store.

rafaelfranca added a commit that referenced this issue Jun 10, 2014
fix error with long keys in ActiveSupport::Cache::FileStore
@rafaelfranca rafaelfranca merged commit 6cef56f into rails:master Jun 10, 2014
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 10, 2014

Sorry, missed the MD5 usage

@grosser
Copy link
Contributor

@grosser grosser commented Jun 10, 2014

💥 @apanzerj

@apanzerj
Copy link
Contributor Author

@apanzerj apanzerj commented Jun 10, 2014

thanks @grosser 😄

@nfm
Copy link

@nfm nfm commented Jun 27, 2014

I'm late to the party sorry but this seems to only solve the problem if key.size > FILEPATH_MAX_SIZE. If key.size > FILENAME_MAX_SIZE && key.size < FILEPATH_MAX_SIZE, I still get an Errno::ENAMETOOLONG.

Here's a test case that fails for me on eCryptfs after applying the patch from this pull request:

def test_medium_keys
  @cache.write("a"*500, 1)
  assert_equal 1, @cache.read("a"*10000)
end

Would it make sense to just always MD5 the key, or are there performance concerns with that approach?

By the way, if we continue to compare key.size to constants, we still need to reduce those constants to work for the lowest common denominator (probably eCryptfs, where FILENAME_MAX_SIZE seems to be 140).

/cc @apanzerj @grosser @rafaelfranca

@grosser
Copy link
Contributor

@grosser grosser commented Jun 27, 2014

already prepared a PR for you ;)
#15688

@nfm
Copy link

@nfm nfm commented Jun 27, 2014

@grosser ;) will join the discussion. Thanks for the link!

@jeremy
Copy link
Member

@jeremy jeremy commented Jun 27, 2014

how can the md5 not be unique ?

By the pigeonhole principle. Putting n items into m bins where n > m means a bin must have > 1 item: a collision. MD5 is a 128 bit hash, so it has 2^128 "bins" - meaning that, in practice, the likelihood of collision is extremely low.

@jeremy
Copy link
Member

@jeremy jeremy commented Jun 27, 2014

Even if it's very unlikely, the fact that it's possible is a major problem. If we hash cache keys, we risk major security breach. To fix that, we'd need to store the original cache key along with the cached data and verify it before returning the result.

@grosser
Copy link
Contributor

@grosser grosser commented Jun 27, 2014

Afaik memcached/dalli also uses that and nobody complained about it so far
... also 2**128 is pretty big ...
340,282,366,920,938,463,463,374,607,431,768,211,456
so yeah ...

On Thu, Jun 26, 2014 at 8:31 PM, Jeremy Kemper notifications@github.com
wrote:

Even if it's very unlikely, the fact that it's possible is a major
problem. If we hash cache keys, we risk major security breach. To fix that,
we'd need to store the original cache key along with the cached data
and verify it before returning the result.


Reply to this email directly or view it on GitHub
#15616 (comment).

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

Successfully merging this pull request may close these issues.

None yet

5 participants