security vulnerability: bypass throttling

[akkuman]

I sent an email a long time ago, but there was no response.

Because the throttling is not thread-safe, I can bypass the throttling via concurrency request.

this is my example code

this is my concurrency test

this is result

[tomchristie]

So, this has come up a bunch of times...

The throttling isn't intended as a security feature, and will have some fuzziness.
We probably want to document that clearly.

It's not so much "thread-safety", as it is to do with using a non-transactional approach when reading/writing the throttling history. (because the cache backends don't have any obvious way to support transactions.)

We read the throttle history from the cache, do a bit of work to determine what the new history should be, and then store that... https://github.com/encode/django-rest-framework/blob/master/rest_framework/throttling.py#L123-L132

There's plenty of scope for someone to implement an alternative throttle that does have locking, but it's not obvious to me what a good approach would be?

Looking into this a bit - Redis supports named locks, and the django-redis package has an interface onto that... https://github.com/jazzband/django-redis#locks - It's feasible that we could support locking around the throttle get/set if the cache backend that's installed supports it.

[LeOndaz]

I'm following this

[akkuman]

I don't think this is caused by non-transactional. It's that the code doesn't consider thread-safety

In https://github.com/encode/django-rest-framework/blob/master/rest_framework/throttling.py#L123-L132

At start, When requesting concurrently, it is possible for multiple threads to pass through this line of code（while self.history and self.history[-1] <= self.now - self.duration:） at the same time

Then self.history == []，so will self.throttle_success()

[tomchristie]

Each request will end up with a clean throttle instance, they're not shared between requests.

[akkuman]

But each time the value of self.history is sourced from the cache, It is equivalent to a global array.

When determining and modifying a value is not an atomic operation, it will inevitably lead to bypass problems

[tomchristie]

When determining and modifying a value is not an atomic operation, it will inevitably lead to bypass problems

Exactly, yes.

A with cache.lock("api-throttle"): ... around that block would resolve that.
None of Django's built-in cache backends support that, but django-redis does.