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

Protect against attacks. #144

Closed
wants to merge 1 commit into from
Closed

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Feb 24, 2020

An attacker can prevent new items from being admitted to the cache by
artificially raising the frequency of the victims. This PR implements an
strategy used in Caffeine to occasionnaly let items that would otherwise
be rejected.

Fixes #131


This change is Reviewable

An attacker can prevent new items from being admitted to the cache by
artificially raising the frequency of the victims. This PR implements an
strategy used in Caffeine to occasionnaly let items that would otherwise
be rejected.
// hash collision attacks, in which the score of the victims is artificially
// raised so that no new items are admitted into the cache.
// This strategy is implemented by Caffeine.
if incHits < minHits && !(rand.Int() % 127 == 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace the modulus % with a bitmask &. This was why 127 was chosen (2^n - 1) so that we could avoid the division at ~1% acceptance rate. The division vs mask was perhaps an over-optimization in this code path, though!

@ben-manes
Copy link

Due to using random eviction, you currently are not impacted by this attack. Caffeine is because LRU is a deterministic algorithm. However I think having some protection is good regardless since it is cheap and documented if you refactor away from random eviction someday.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinmr What's the status of this? Do we still need it?

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim, @karlmcguire, @manishrjain, and @martinmr)

@martinmr
Copy link
Contributor Author

Closing this ticket since it's not a required feature.
cc: @jarifibrahim

@martinmr martinmr closed this Sep 16, 2020
@martinmr martinmr deleted the martinmr/policy-randomness branch September 16, 2020 17:48
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

Successfully merging this pull request may close these issues.

Protect against hash attacks.
4 participants