Opened 5 years ago

Closed 5 years ago

#16827 closed Bug (fixed)

validate CSRF token (Check length)

Reported by: jedie Owned by: Raúl Cumplido
Component: CSRF Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

I wonder that the CSRF token send from the client didn't be validated.

Don't know if a DOS attack is possible by sending many request with very long CSRF tokens?

IMHO it's a good idea to check the length before do anything with it.

See also: https://groups.google.com/group/django-developers/browse_thread/thread/9fc008d2a3735bc2

Attachments (2)

patch_16827.diff (1.9 KB) - added by Zbigniew Siciarz 5 years ago.
Provided test for the token length.
patch_16827_2.diff (2.7 KB) - added by Zbigniew Siciarz 5 years ago.
Replaced the magic number with a constant.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 5 years ago by jedie

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 5 years ago by tinodb

Component: Uncategorizedcontrib.csrf
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:3 Changed 5 years ago by Raúl Cumplido

Owner: changed from nobody to Raúl Cumplido
Status: newassigned

I will provide the tests to the pull request.

Changed 5 years ago by Zbigniew Siciarz

Attachment: patch_16827.diff added

Provided test for the token length.

Changed 5 years ago by Zbigniew Siciarz

Attachment: patch_16827_2.diff added

Replaced the magic number with a constant.

comment:4 Changed 5 years ago by Tomek Paczkowski

Triage Stage: AcceptedReady for checkin

Looks good.

comment:6 Changed 5 years ago by Ashutosh Dwivedi

from the source code i noticed that the method

django.utils.crypto.constant_time_compare

is being used for comparing the request token from the one already present in the cookie, since the comparison is already constant time is the length checking really required ?

comment:7 Changed 5 years ago by Paul McMillan

Resolution: fixed
Status: assignedclosed

aashu_dwivedi: The length check is worth fixing. It is true that the constant_time_compare will fail early if the lengths do not match, but the performance concern is in the regex applied to sanitize the string before it gets to constant_time_compare.

In [17500]:

Fixes #16827. Adds a length check to CSRF tokens before applying the santizing regex. Thanks to jedie for the report and zsiciarz for the initial patch.

Last edited 5 years ago by Paul McMillan (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top