Template fragment cache key collision for vary_on values containing ":"

[Jacob Walls]

The ​documented example for template fragment caching demonstrates using request.user.username as a vary_on argument. If that username contained :, and another vary_on argument was present, then two cache keys might collide, and the wrong content could be served.

See this PoC provided to the Security Team:

from django.core.cache.utils import make_template_fragment_key
a = make_template_fragment_key("frag", ["alice", "b:c"])
b = make_template_fragment_key("frag", ["alice:b", "c"])
assert a == b   # same key

We decided against accepting this as a security issue given the unlikelihood of colons in the data most important to vary on from a security perspective, e.g. usernames in a username + language code vary_on pair, but there is a correctness issue to fix here.

One fix strategy would involve incorporating the lengths of the arguments into the cache key.

Since this will cause cache busting, we should probably document in the release note something similar to the note from 5cb3ed187b283059589cb442c56a66a795800cac.

[Natalia Bidart]

Thanks!

[Jacob Walls]

Amar, this one is almost identical to #37101, so if you don't mind, I'm going to fix them both in the same PR to aid review.

[Amar]

No worries at all, Jacob! You are welcome to take it. A combined PR makes perfect sense here.

[Jacob Walls]

​PR

[Juan Pedro Roldán]

Hi,

I was reviewing this ticket while looking for a small code-related contribution to understand Django’s contribution workflow. I noticed that the ticket is already assigned, so I won’t work on it directly.

I tried to reproduce the issue locally using the example from the ticket description:

make_template_fragment_key("frag", ["alice", "b:c"])

and

make_template_fragment_key("frag", ["alice:b", "c"])

Both calls generated the same cache key:

template.cache.frag.486f15e693cb74d155ff00eb53e7e306

I also added a small local regression test in tests/cache/tests.py, inside TestMakeTemplateFragmentKey, to check that these two keys should be different. As expected, the test fails on the current branch, so the reported behavior is reproduced.

I hope this helps with testing or triage. I’d be happy to test a proposed patch if needed.

[Jacob Walls]

Hi there. The attached PR is almost mergable, awaiting re-review on the release note. Any comments welcome!

[Jacob Walls <jacobtylerwalls@…>]

In 8acd000:

Fixed #37174 -- Used netstring delimiter between vary on arguments for cached template fragments.

This algorithm used a delimiter, but because it didn't use a length,
argument values containing the delimiter could still cause collisions.

[Jacob Walls <jacobtylerwalls@…>]

In 02f94d2:

Refs #37101, #37174 -- Added release note for cache misses when varying on arguments.

Thanks Natalia Bidart for the review.

[Jacob Walls <jacobtylerwalls@…>]

In 0383d12:

Refs #37174 -- Doc'd that args to the {% cache %} tag are stringified.

[Jacob Walls <jacobtylerwalls@…>]

In 0317b61:

[6.1.x] Fixed #37174 -- Used netstring delimiter between vary on arguments for cached template fragments.

This algorithm used a delimiter, but because it didn't use a length,
argument values containing the delimiter could still cause collisions.

Backport of 8acd000725838f73c5e6781114f11866f6674c46 from main.

[Jacob Walls <jacobtylerwalls@…>]

In e4cda2e:

[6.1.x] Refs #37101, #37174 -- Added release note for cache misses when varying on arguments.

Thanks Natalia Bidart for the review.

Backport of 02f94d2a899b6573f90dee5417afb8a763ee7f25 from main.

[Jacob Walls <jacobtylerwalls@…>]

In d5ff018:

[6.1.x] Refs #37174 -- Doc'd that args to the {% cache %} tag are stringified.

Backport of 0383d12ea3451a75be184bd1783f11031903e7ee from main.