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

SecureRandom cache_id generation #2326

Closed
t-tera opened this issue Jul 10, 2018 · 7 comments
Closed

SecureRandom cache_id generation #2326

t-tera opened this issue Jul 10, 2018 · 7 comments

Comments

@t-tera
Copy link

t-tera commented Jul 10, 2018

As a security tester, I have seen developers who assume cache_id is unpredictable, but that's not actually true in the strict sense.

Would you be able to make it truly secure random?
The following is a quick but dirty fix for generate_cache_id method.

  def self.generate_cache_id
    [Time.now.utc.to_i,
      Process.pid,
      # ->  '%06d' % Process.pid + Integer('0x' + SecureRandom.hex).to_s,
      '%04d' % (CarrierWave::CacheCounter.increment % 1000),
      # ->  Do you mean % 10000? This bug increases collision probability.

Thanks.

@mshibuya mshibuya added this to the Release v2.0.0 milestone May 1, 2019
@t-tera
Copy link
Author

t-tera commented May 16, 2019

Thank you. But a number less than 10000 is considered predictable.

@mshibuya
Copy link
Member

How much is enough?

@t-tera
Copy link
Author

t-tera commented May 16, 2019

128bit is enough.

If fixed length is desirable, how about using the following?

'%032x' % SecureRandom.random_number(1 <<128) # 128bit hex

One thing to be noted here is that when I looked into the source code of carrierwave some months ago, I found some regexps in carrierwave that assumed the random number being consisted of 4 decimal digit and that's why I suggested a weird solution in the first post.

So, to make the part longer and use hex chars, some change might be necessary somewhere else.

@t-tera
Copy link
Author

t-tera commented May 16, 2019

I'm not quite certain but maybe the regex I found was this.
https://github.com/carrierwaveuploader/carrierwave/blob/v1.3.1/lib/carrierwave/uploader/cache.rb#L193

@mshibuya
Copy link
Member

My concern is changing the format of cache id may break compatibility for existing installations. I guess it will not so, but I must think deeper before I'm certain.
I think initial implementation of generate_cache_id didn't have consideration for unpredictability, the only concern was collision avoidance.

@mshibuya mshibuya reopened this May 17, 2019
@mshibuya
Copy link
Member

I've decided not to change cache id format because of compatibility concern.
A random number up to 10^(15+4) is almost equivalent to 64 bits of entropy, it should be hard enough for this use case...

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

No branches or pull requests

2 participants